Skip to content

Fix error position for switch-statement condition and for-statement initializer #7305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 2 commits into from
Jul 25, 2018
Merged

Fix error position for switch-statement condition and for-statement initializer #7305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b4bae4
[Feature] Fix error position for switch-statement condition and for-s…
daxian-dbw Jul 4, 2018
755153e
[Feature] Correct sequence point update for while/do-while/do-until
daxian-dbw Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 77 additions & 26 deletions src/System.Management.Automation/engine/parser/Compiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,14 +743,16 @@ static Compiler()
s_builtinAttributeGenerator.Add(typeof(ValidateNotNullOrEmptyAttribute), NewValidateNotNullOrEmptyAttribute);
}

private Compiler(List<IScriptExtent> sequencePoints)
private Compiler(List<IScriptExtent> sequencePoints, Dictionary<IScriptExtent, int> sequencePointIndexMap)
{
_sequencePoints = sequencePoints;
_sequencePointIndexMap = sequencePointIndexMap;
}

internal Compiler()
{
_sequencePoints = new List<IScriptExtent>();
_sequencePointIndexMap = new Dictionary<IScriptExtent, int>();
}

internal bool CompilingConstantExpression { get; set; }
Expand All @@ -766,6 +768,7 @@ internal Compiler()
private int _switchTupleIndex = VariableAnalysis.Unanalyzed;
private int _foreachTupleIndex = VariableAnalysis.Unanalyzed;
private readonly List<IScriptExtent> _sequencePoints;
private readonly Dictionary<IScriptExtent, int> _sequencePointIndexMap;
private int _stmtCount;

internal bool CompilingMemberFunction { get; set; }
Expand Down Expand Up @@ -903,17 +906,31 @@ internal static Expression IsStrictMode(int version, Expression executionContext
ExpressionCache.Constant(version));
}

internal Expression UpdatePosition(Ast ast)
private int AddSequencePoint(IScriptExtent extent)
Copy link
Member Author

@daxian-dbw daxian-dbw Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this method is added to make sure that one extent can only be added to the sequence list once.

{
_sequencePoints.Add(ast.Extent);
// Make sure we don't add the same extent to the sequence point list twice.
if (!_sequencePointIndexMap.TryGetValue(extent, out int index))
{
_sequencePoints.Add(extent);
index = _sequencePoints.Count - 1;
_sequencePointIndexMap.Add(extent, index);
}

return index;
}

private Expression UpdatePosition(Ast ast)
{
IScriptExtent extent = ast.Extent;
int index = AddSequencePoint(extent);

// If we just added the first sequence point, then we don't want to check for breakpoints - we'll do that
// in EnterScriptFunction.
// Except for while/do loops, in this case we want to check breakpoints on the first sequence point since it
// will be executed multiple times.
return ((_sequencePoints.Count == 1) && !_generatingWhileOrDoLoop)
return (index == 0 && !_generatingWhileOrDoLoop)
? ExpressionCache.Empty
: new UpdatePositionExpr(ast.Extent, _sequencePoints.Count - 1, _debugSymbolDocument, !_compilingSingleExpression);
: new UpdatePositionExpr(extent, index, _debugSymbolDocument, !_compilingSingleExpression);
}

private int _tempCounter;
Expand Down Expand Up @@ -1683,7 +1700,7 @@ internal void Compile(CompiledScriptBlockData scriptBlock, bool optimize)
// on this sequence point, but it makes it safe to access the CurrentPosition
// property in FunctionContext (which can happen if there are exceptions
// defining the functions.)
_sequencePoints.Add(ast.Extent);
AddSequencePoint(ast.Extent);
}

var compileInterpretChoice = (_stmtCount > 300) ? CompileInterpretChoice.NeverCompile : CompileInterpretChoice.CompileOnDemand;
Expand Down Expand Up @@ -1853,8 +1870,8 @@ private Func<FunctionContext, object> CompileSingleExpression(ExpressionAst expr
var exprs = new List<Expression>();
var temps = new List<ParameterExpression> { _executionContextParameter, LocalVariablesParameter };
GenerateFunctionProlog(exprs, temps, null);
_sequencePoints.Add(expressionAst.Extent);
exprs.Add(new UpdatePositionExpr(expressionAst.Extent, _sequencePoints.Count - 1, _debugSymbolDocument, checkBreakpoints: true));
int index = AddSequencePoint(expressionAst.Extent);
exprs.Add(new UpdatePositionExpr(expressionAst.Extent, index, _debugSymbolDocument, checkBreakpoints: true));
var result = Compile(expressionAst).Cast(typeof(object));
exprs.Add(Expression.Label(_returnTarget, result));
var body = Expression.Block(new[] { _executionContextParameter, LocalVariablesParameter }, exprs);
Expand Down Expand Up @@ -2143,7 +2160,7 @@ private Expression<Action<FunctionContext>> CompileNamedBlock(NamedBlockAst name

private Tuple<Action<FunctionContext>, Type> CompileTrap(TrapStatementAst trap)
{
var compiler = new Compiler(_sequencePoints) { _compilingTrap = true };
var compiler = new Compiler(_sequencePoints, _sequencePointIndexMap) { _compilingTrap = true };
string funcName = _currentFunctionName + "<trap>";
if (trap.TrapType != null)
{
Expand Down Expand Up @@ -2517,8 +2534,8 @@ private void GenerateFunctionProlog(List<Expression> exprs, List<ParameterExpres

if (entryExtent != null)
{
_sequencePoints.Add(entryExtent);
exprs.Add(new UpdatePositionExpr(entryExtent, _sequencePoints.Count - 1, _debugSymbolDocument, checkBreakpoints: false));
int index = AddSequencePoint(entryExtent);
exprs.Add(new UpdatePositionExpr(entryExtent, index, _debugSymbolDocument, checkBreakpoints: false));
}

exprs.Add(
Expand Down Expand Up @@ -2568,8 +2585,8 @@ private Expression InitializeMemberProperties(Expression ourThis)
}

var extent = propertyMember.InitialValue.Extent;
_sequencePoints.Add(extent);
exprs.Add(new UpdatePositionExpr(extent, _sequencePoints.Count - 1, _debugSymbolDocument, checkBreakpoints: false));
int index = AddSequencePoint(extent);
exprs.Add(new UpdatePositionExpr(extent, index, _debugSymbolDocument, checkBreakpoints: false));
var property = _memberFunctionType.Type.GetProperty(propertyMember.Name, bindingFlags);
exprs.Add(
Expression.Assign(
Expand Down Expand Up @@ -2952,12 +2969,12 @@ public object VisitIfStatement(IfStatementAst ifStmtAst)
{
int clauseCount = ifStmtAst.Clauses.Count;

var exprs = new Tuple<BlockExpression, Expression>[clauseCount];
var exprs = new Tuple<Expression, Expression>[clauseCount];
for (int i = 0; i < clauseCount; ++i)
{
IfClause ifClause = ifStmtAst.Clauses[i];
var cond = Expression.Block(
UpdatePosition(ifClause.Item1),
var cond = UpdatePositionForInitializerOrCondition(
ifClause.Item1,
CaptureStatementResults(ifClause.Item1, CaptureAstContext.Condition).Convert(typeof(bool)));
var body = Compile(ifClause.Item2);
exprs[i] = Tuple.Create(cond, body);
Expand Down Expand Up @@ -3022,7 +3039,7 @@ private Expression CompileAssignment(
}

var exprs = new List<Expression>
{
{
// Set current position in case of errors.
UpdatePosition(assignmentStatementAst),
ReduceAssignment((ISupportsAssignment)assignmentStatementAst.Left,
Expand Down Expand Up @@ -4174,7 +4191,7 @@ private Expression GenerateDoLoop(LoopStatementAst loopStatement)
_loopTargets.Add(new LoopGotoTargets(loopLabel ?? string.Empty, breakLabel, continueLabel));
_generatingWhileOrDoLoop = true;
var loopBodyExprs = new List<Expression>
{
{
s_callCheckForInterrupts,
Compile(loopStatement.Body),
ExpressionCache.Empty
Expand All @@ -4190,6 +4207,7 @@ private Expression GenerateDoLoop(LoopStatementAst loopStatement)
{
test = Expression.Not(test);
}
test = UpdatePositionForInitializerOrCondition(loopStatement.Condition, test);
exprs.Add(Expression.IfThen(test, Expression.Goto(repeatLabel)));
exprs.Add(Expression.Label(breakLabel));

Expand Down Expand Up @@ -4240,10 +4258,26 @@ private Expression GenerateIteratorStatement(VariablePath iteratorVariablePath,
// $foreach/$switch = GetEnumerator $enumerable
var enumerable = NewTemp(typeof(object), "enumerable");
temps.Add(enumerable);

// Update position to make it safe to access 'CurrentPosition' property in FunctionContext in case
// that the evaluation of 'stmt.Condition' throws exception.
if (generatingForeach)
{
// For foreach statement, we want the debugger to stop at 'stmt.Condition' before evaluating it.
// The debugger will stop at 'stmt.Condition' only once. The following enumeration will stop at
// the foreach variable.
exprs.Add(UpdatePosition(stmt.Condition));
}
else
{
// For switch statement, we don't want the debugger to stop at 'stmt.Condition' before evaluating it.
Copy link
Contributor

@PaulHigin PaulHigin Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this comment it is not clear how the code prevents these two undesirable behaviors. Can you clarify?

Copy link
Member Author

@daxian-dbw daxian-dbw Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument checkBreakpoints: false when calling UpdatePositionExpr means not checking breaking point for this sequence point update.

// The following enumeration will stop at 'stmt.Condition' again, and we don't want the debugger to
// stop at 'stmt.Condition' twice before getting into one of its case clauses.
var extent = stmt.Condition.Extent;
int index = AddSequencePoint(extent);
exprs.Add(new UpdatePositionExpr(extent, index, _debugSymbolDocument, checkBreakpoints: false));
}

exprs.Add(
Expression.Assign(enumerable,
GetRangeEnumerator(stmt.Condition.GetPureExpression())
Expand Down Expand Up @@ -4354,6 +4388,19 @@ private Expression GetRangeEnumerator(ExpressionAst condExpr)
return null;
}

private Expression UpdatePositionForInitializerOrCondition(PipelineBaseAst pipelineBaseAst, Expression initializerOrCondition)
{
if (pipelineBaseAst is PipelineAst pipelineAst && !pipelineAst.Background && pipelineAst.GetPureExpression() != null)
{
// If the initializer or condition is a pure expression (CommandExpressionAst without redirection),
// then we need to add a sequence point. If it's an AssignmentStatementAst, we don't need to add
// sequence point here because one will be added when processing the AssignmentStatementAst.
initializerOrCondition = Expression.Block(UpdatePosition(pipelineBaseAst), initializerOrCondition);
}

return initializerOrCondition;
}

public object VisitDoWhileStatement(DoWhileStatementAst doWhileStatementAst)
{
return GenerateDoLoop(doWhileStatementAst);
Expand All @@ -4367,13 +4414,17 @@ public object VisitDoUntilStatement(DoUntilStatementAst doUntilStatementAst)
public object VisitForStatement(ForStatementAst forStatementAst)
{
// We should not preserve the partial output if exception is thrown when evaluating the initializer.
var init = (forStatementAst.Initializer != null)
? CaptureStatementResults(forStatementAst.Initializer, CaptureAstContext.AssignmentWithoutResultPreservation)
: null;
Expression init = null;
PipelineBaseAst initializer = forStatementAst.Initializer;
if (initializer != null)
{
init = CaptureStatementResults(initializer, CaptureAstContext.AssignmentWithoutResultPreservation);
init = UpdatePositionForInitializerOrCondition(initializer, init);
}

var generateCondition = forStatementAst.Condition != null
? () => Expression.Block(UpdatePosition(forStatementAst.Condition),
CaptureStatementResults(forStatementAst.Condition, CaptureAstContext.Condition))
PipelineBaseAst condition = forStatementAst.Condition;
var generateCondition = condition != null
? () => UpdatePositionForInitializerOrCondition(condition, CaptureStatementResults(condition, CaptureAstContext.Condition))
: (Func<Expression>)null;

var loop = GenerateWhileLoop(forStatementAst.Label, generateCondition,
Expand All @@ -4389,9 +4440,9 @@ public object VisitForStatement(ForStatementAst forStatementAst)

public object VisitWhileStatement(WhileStatementAst whileStatementAst)
{
PipelineBaseAst condition = whileStatementAst.Condition;
return GenerateWhileLoop(whileStatementAst.Label,
() => Expression.Block(UpdatePosition(whileStatementAst.Condition),
CaptureStatementResults(whileStatementAst.Condition, CaptureAstContext.Condition)),
() => UpdatePositionForInitializerOrCondition(condition, CaptureStatementResults(condition, CaptureAstContext.Condition)),
(loopBody, breakTarget, continueTarget) => loopBody.Add(Compile(whileStatementAst.Body)));
}

Expand Down
Loading