Skip to content
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.

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

IOperation API extensions to expose low-level operations #21705

Closed
Changes from 8 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -465,11 +465,18 @@ private int CheckAndAdjustPositionForSpeculativeAttribute(int position)

#endregion Helpers for speculative binding

protected override IOperation GetOperationCore(SyntaxNode node, CancellationToken cancellationToken)
protected override IOperation GetOperationCore(IMethodSymbol method, SyntaxNode body, CancellationToken cancellationToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we consider changing the API to take only one of the symbol OR body parameters? Alternatively, please file a separate issue to track removal of one of these parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

{
var csnode = (CSharpSyntaxNode)node;
CheckSyntaxNode(csnode);
return this.GetOperationWorker(csnode, GetOperationOptions.Lowest, cancellationToken);
var csbody = (CSharpSyntaxNode)body;
var csmethod = (MethodSymbol)method;
Copy link
Contributor

Choose a reason for hiding this comment

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

(MethodSymbol)method [](start = 27, length = 20)

Please use EnsureCSharpSymbolOrNull helper.

CheckSyntaxNode(csbody);
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckSyntaxNode(csbody); [](start = 12, length = 24)

I think consumer might be confused by an exception thrown by this helper because syntax is not an input for the GetOperation API.

CheckSymbol(csmethod);
return this.GetOperationWorker(csmethod, csbody, cancellationToken);
}

internal virtual IOperation GetOperationWorker(MethodSymbol method, CSharpSyntaxNode body, CancellationToken cancellationToken)
{
return null;
}

internal enum GetOperationOptions
@@ -479,6 +486,13 @@ internal enum GetOperationOptions
Parent
}

protected override IOperation GetOperationCore(SyntaxNode node, CancellationToken cancellationToken)
{
var csnode = (CSharpSyntaxNode)node;
CheckSyntaxNode(csnode);
return this.GetOperationWorker(csnode, GetOperationOptions.Lowest, cancellationToken);
}

internal virtual IOperation GetOperationWorker(CSharpSyntaxNode node, GetOperationOptions options, CancellationToken cancellationToken)
{
return null;
@@ -1246,6 +1260,14 @@ protected void AssertPositionAdjusted(int position)
Debug.Assert(position == CheckAndAdjustPosition(position), "Expected adjusted position");
}

protected void CheckSymbol(Symbol symbol)
{
if (symbol == null)
{
throw new ArgumentNullException(nameof(symbol));
}
}

protected void CheckSyntaxNode(CSharpSyntaxNode syntax)
{
if (syntax == null)
Original file line number Diff line number Diff line change
@@ -960,6 +960,17 @@ public override SyntaxTree SyntaxTree
}

internal override IOperation GetOperationWorker(CSharpSyntaxNode node, GetOperationOptions options, CancellationToken cancellationToken)
{
var result = GetBoundNode(node, options);

// The CSharp operation factory assumes that UnboundLambda will be bound for error recovery and never be passed to the factory
// as the start of a tree to get operations for. This is guaranteed by the builder that populates the node map, as it will call
// UnboundLambda.BindForErrorRecovery() when it encounters an UnboundLambda node.
Debug.Assert(result.Kind != BoundKind.UnboundLambda);
return _operationFactory.Create(result);
}

private BoundNode GetBoundNode(CSharpSyntaxNode node, GetOperationOptions options)
{
CSharpSyntaxNode bindableNode;

@@ -983,11 +994,55 @@ internal override IOperation GetOperationWorker(CSharpSyntaxNode node, GetOperat
break;
}

// The CSharp operation factory assumes that UnboundLambda will be bound for error recovery and never be passed to the factory
// as the start of a tree to get operations for. This is guaranteed by the builder that populates the node map, as it will call
// UnboundLambda.BindForErrorRecovery() when it encounters an UnboundLambda node.
Debug.Assert(result.Kind != BoundKind.UnboundLambda);
return _operationFactory.Create(result);
return result;
}

internal override IOperation GetOperationWorker(MethodSymbol method, CSharpSyntaxNode bodySyntax, CancellationToken cancellationToken)
{
IOperation result = null;
var node = GetBoundNode(bodySyntax, GetOperationOptions.Highest);

if (!node.HasErrors)
{
var body = (BoundStatement)node;
var loweredBody = LowerMethodBody(method, body);

// The CSharp operation factory assumes that UnboundLambda will be bound for error recovery and never be passed to the factory
// as the start of a tree to get operations for. This is guaranteed by the builder that populates the node map, as it will call
// UnboundLambda.BindForErrorRecovery() when it encounters an UnboundLambda node.
Debug.Assert(loweredBody.Kind != BoundKind.UnboundLambda);
result = _operationFactory.Create(loweredBody);
Copy link
Contributor

Choose a reason for hiding this comment

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

result = _operationFactory.Create(loweredBody); [](start = 16, length = 47)

It feels wrong to tie lowered tree to any SemanticModel. SemanticModel deals with un-lowered bound trees, who knows what negative effects will this operation have, how will it affect the caches, etc. I think there should be no interaction between SemanticModel and the tree built of off lowered bound nodes.

}

return result;
}

private BoundStatement LowerMethodBody(MethodSymbol method, BoundStatement body)
{
var diagnostics = DiagnosticBag.GetInstance();
Copy link
Contributor

Choose a reason for hiding this comment

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

DiagnosticBag.GetInstance() [](start = 30, length = 27)

Are we leaking diagnostics bag from the pool?

var compilationState = new TypeCompilationState(method.ContainingType, _compilation, null);

var dynamicAnalysisSpans = ImmutableArray<CodeAnalysis.CodeGen.SourceSpan>.Empty;
//var synthesizedSubmissionFields = method.ContainingType.IsSubmissionClass ? new SynthesizedSubmissionFields(_compilation, method.ContainingType) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented-out code.


var loweredBody = LocalRewriter.Rewrite(
_compilation,
method,
-1,
Copy link
Member

Choose a reason for hiding this comment

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

Magic number

method.ContainingType,
body,
compilationState,
previousSubmissionFields: null,
allowOmissionOfConditionalCalls: true,
instrumentForDynamicAnalysis: false,
debugDocumentProvider: null,
dynamicAnalysisSpans: ref dynamicAnalysisSpans,
diagnostics: diagnostics,
sawLambdas: out bool sawLambdas,
sawLocalFunctions: out bool sawLocalFunctions,
sawAwaitInExceptionHandler: out bool sawAwaitInExceptionHandler);

return loweredBody;
}

internal override SymbolInfo GetSymbolInfoWorker(CSharpSyntaxNode node, SymbolInfoOptions options, CancellationToken cancellationToken = default(CancellationToken))
@@ -1724,7 +1779,9 @@ internal ImmutableArray<BoundNode> GetBoundNodes(CSharpSyntaxNode node)
return ImmutableArray<BoundNode>.Empty;
}

// some nodes don't have direct semantic meaning by themselves and so we need to bind a different node that does
/// <summary>
/// Some nodes don't have direct semantic meaning by themselves and so we need to bind a different node that does.
/// </summary>
internal protected virtual CSharpSyntaxNode GetBindableSyntaxNode(CSharpSyntaxNode node)
{
switch (node.Kind())
Original file line number Diff line number Diff line change
@@ -161,6 +161,22 @@ internal override Binder GetEnclosingBinderInternal(int position)
return _binderFactory.GetBinder((CSharpSyntaxNode)token.Parent, position).WithAdditionalFlags(GetSemanticModelBinderFlags());
}

internal override IOperation GetOperationWorker(MethodSymbol method, CSharpSyntaxNode node, CancellationToken cancellationToken)
{
// in case this is right side of a qualified name or member access (or part of a cref)
node = SyntaxFactory.GetStandaloneNode(node);

var model = this.GetMemberModel(node);
if (model != null)
Copy link
Member

Choose a reason for hiding this comment

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

return model?.GetOperationWorker(method, node, cancellationToken)

{
return model.GetOperationWorker(method, node, cancellationToken);
}
else
{
return null;
}
}

internal override IOperation GetOperationWorker(CSharpSyntaxNode node, GetOperationOptions options, CancellationToken cancellationToken)
{
// in case this is right side of a qualified name or member access (or part of a cref)
Original file line number Diff line number Diff line change
@@ -218,6 +218,21 @@ private IOperation CreateInternal(BoundNode boundNode)
return CreateBoundPatternSwitchLabelOperation((BoundPatternSwitchLabel)boundNode);
case BoundKind.IsPatternExpression:
return CreateBoundIsPatternExpressionOperation((BoundIsPatternExpression)boundNode);

// To support BoundNodes after lowering phase.
case BoundKind.SequencePoint:
return CreateBoundSequencePointOperation((BoundSequencePoint)boundNode);
case BoundKind.SequencePointWithSpan:
return CreateBoundSequencePointWithSpanOperation((BoundSequencePointWithSpan)boundNode);
case BoundKind.SequencePointExpression:
return CreateBoundSequencePointExpressionOperation((BoundSequencePointExpression)boundNode);
case BoundKind.StatementList:
return CreateBoundStatementListOperation((BoundStatementList)boundNode);
case BoundKind.ConditionalGoto:
return CreateBoundConditionalGotoOperation((BoundConditionalGoto)boundNode);
case BoundKind.Sequence:
return CreateBoundSequenceOperation((BoundSequence)boundNode);

default:
var constantValue = ConvertToOptional((boundNode as BoundExpression)?.ConstantValue);
return Operation.CreateOperationNone(boundNode.Syntax, constantValue, getChildren: () => GetIOperationChildren(boundNode));
@@ -938,7 +953,7 @@ private IParameterInitializer CreateBoundParameterEqualsValueOperation(BoundPara
private IBlockStatement CreateBoundBlockOperation(BoundBlock boundBlock)
{
Lazy<ImmutableArray<IOperation>> statements =
new Lazy<ImmutableArray<IOperation>>(() => boundBlock.Statements.Select(s => Create(s)).Where(s => s.Kind != OperationKind.None).ToImmutableArray());
new Lazy<ImmutableArray<IOperation>>(() => boundBlock.Statements.Select(s => Create(s)).Where(s => s != null && s.Kind != OperationKind.None).ToImmutableArray());

ImmutableArray<ILocalSymbol> locals = boundBlock.Locals.As<ILocalSymbol>();
SyntaxNode syntax = boundBlock.Syntax;
@@ -1335,5 +1350,56 @@ private IIsPatternExpression CreateBoundIsPatternExpressionOperation(BoundIsPatt
Optional<object> constantValue = ConvertToOptional(boundIsPatternExpression.ConstantValue);
return new LazyIsPatternExpression(expression, pattern, syntax, type, constantValue);
}

private IOperation CreateBoundSequencePointOperation(BoundSequencePoint boundSequencePoint)
{
return Create(boundSequencePoint.StatementOpt);
}

private IOperation CreateBoundSequencePointWithSpanOperation(BoundSequencePointWithSpan boundSequencePointWithSpan)
{
return Create(boundSequencePointWithSpan.StatementOpt);
}

private IOperation CreateBoundSequencePointExpressionOperation(BoundSequencePointExpression boundSequencePointExpression)
{
return Create(boundSequencePointExpression.Expression);
}

private IBlockStatement CreateBoundStatementListOperation(BoundStatementList boundStatementList)
{
Lazy<ImmutableArray<IOperation>> statements =
new Lazy<ImmutableArray<IOperation>>(() => boundStatementList.Statements.Select(s => Create(s)).Where(s => s != null && s.Kind != OperationKind.None).ToImmutableArray());
Copy link
Contributor

Choose a reason for hiding this comment

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

s.Kind != OperationKind.None [](start = 136, length = 28)

Why is this the right condition? Isn't this potentially going to remove significant portion of the tree?


ImmutableArray<ILocalSymbol> locals = ImmutableArray<ILocalSymbol>.Empty;
SyntaxNode syntax = boundStatementList.Syntax;
ITypeSymbol type = null;
Optional<object> constantValue = default(Optional<object>);
return new LazyBlockStatement(statements, locals, syntax, type, constantValue);
}

private IConditionalGotoStatement CreateBoundConditionalGotoOperation(BoundConditionalGoto boundConditionalGoto)
{
Lazy<IOperation> condition = new Lazy<IOperation>(() => Create(boundConditionalGoto.Condition));
ILabelSymbol target = boundConditionalGoto.Label;
bool jumpIfTrue = boundConditionalGoto.JumpIfTrue;
SyntaxNode syntax = boundConditionalGoto.Syntax;
ITypeSymbol type = null;
Optional<object> constantValue = default(Optional<object>);
return new LazyConditionalGotoStatement(condition, target, jumpIfTrue, syntax, type, constantValue);
}

private IOperation CreateBoundSequenceOperation(BoundSequence boundSequence)
{
Lazy<ImmutableArray<IOperation>> expressions =
new Lazy<ImmutableArray<IOperation>>(() => boundSequence.SideEffects.Select(s => Create(s)).Where(s => s != null && s.Kind != OperationKind.None).ToImmutableArray());
Copy link
Contributor

Choose a reason for hiding this comment

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

s.Kind != OperationKind.None [](start = 132, length = 28)

Same question.


Lazy<IOperation> value = new Lazy<IOperation>(() => Create(boundSequence.Value));
ImmutableArray<ILocalSymbol> locals = boundSequence.Locals.As<ILocalSymbol>();
SyntaxNode syntax = boundSequence.Syntax;
ITypeSymbol type = null;
Optional<object> constantValue = default(Optional<object>);
return new LazySequenceExpression(expressions, value, locals, syntax, type, constantValue);
}
}
}
34 changes: 34 additions & 0 deletions src/Compilers/Core/Portable/Compilation/SemanticModel.cs
Original file line number Diff line number Diff line change
@@ -63,6 +63,40 @@ public SyntaxTree SyntaxTree
/// </summary>
protected abstract SyntaxTree SyntaxTreeCore { get; }

/// <summary>
/// Gets the operation corresponding to the method body.
/// </summary>
/// <param name="method">The method symbol.</param>
/// <param name="body">The method body syntax node</param>
/// <param name="cancellationToken">An optional cancellation token.</param>
/// <returns></returns>
public IOperation GetOperation(IMethodSymbol method, SyntaxNode body, CancellationToken cancellationToken = default(CancellationToken))
{
if (!this.Compilation.IsIOperationFeatureEnabled())
{
throw new InvalidOperationException(CodeAnalysisResources.IOperationFeatureDisabled);
}

return GetOperationInternal(method, body, cancellationToken);
}

internal IOperation GetOperationInternal(IMethodSymbol method, SyntaxNode body, CancellationToken cancellationToken = default(CancellationToken))
{
try
{
return GetOperationCore(method, body, cancellationToken);
}
catch (Exception e) when (FatalError.ReportWithoutCrashUnlessCanceled(e))
{
// Log a Non-fatal-watson and then ignore the crash in the attempt of getting operation
Debug.Assert(false);
}

return null;
}

protected abstract IOperation GetOperationCore(IMethodSymbol method, SyntaxNode body, CancellationToken cancellationToken);

/// <summary>
/// Gets the operation corresponding to the expression or statement syntax node.
/// </summary>
Loading