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
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,11 @@ internal static SyntaxToken FirstOrDefault(this SyntaxTokenList list, SyntaxKind
int index = list.IndexOf(kind);
return (index >= 0) ? list[index] : default(SyntaxToken);
}

internal static CSharpSyntaxNode GetCSharpSyntax(this SyntaxReference syntaxReference, CancellationToken cancellationToken = default)
{
return (CSharpSyntaxNode)syntaxReference.GetSyntax(cancellationToken);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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, CancellationToken cancellationToken)
{
var csnode = (CSharpSyntaxNode)node;
CheckSyntaxNode(csnode);
return this.GetOperationWorker(csnode, GetOperationOptions.Lowest, cancellationToken);
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.

CheckSymbol(csmethod);
Copy link
Contributor

Choose a reason for hiding this comment

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

CheckSymbol(csmethod); [](start = 12, length = 22)

It looks like this helper is using "symbol" as parameter name, I would expect "method" to be used for this call site instead.

var csbody = csmethod.GetBodySyntax();
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.

return GetOperationWorker(csmethod, csbody, cancellationToken);
}

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

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

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

internal virtual IOperation GetOperationWorker(CSharpSyntaxNode node, GetOperationOptions options, CancellationToken cancellationToken)
{
return null;
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -983,11 +994,54 @@ 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)
{
const int methodOrdinal = -1;
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 loweredBody = LocalRewriter.Rewrite(
_compilation,
method,
methodOrdinal,
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))
Expand Down Expand Up @@ -1724,7 +1778,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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,20 +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 = GetMemberModel(node);
return model?.GetOperationWorker(method, node, cancellationToken);
}

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)
node = SyntaxFactory.GetStandaloneNode(node);

var model = this.GetMemberModel(node);
if (model != null)
{
return model.GetOperationWorker(node, options, cancellationToken);
}
else
{
return null;
}
var model = GetMemberModel(node);
return model?.GetOperationWorker(node, options, cancellationToken);
}

internal override SymbolInfo GetSymbolInfoWorker(CSharpSyntaxNode node, SymbolInfoOptions options, CancellationToken cancellationToken = default(CancellationToken))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 ISequenceExpression 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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using System.Linq;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
Expand Down Expand Up @@ -316,5 +317,13 @@ internal static CSharpSyntaxNode ExtractReturnTypeSyntax(this MethodSymbol metho

return (CSharpSyntaxNode)CSharpSyntaxTree.Dummy.GetRoot();
}

internal static CSharpSyntaxNode GetBodySyntax(this MethodSymbol method)
{
var syntaxReference = method.DeclaringSyntaxReferences.FirstOrDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

var [](start = 12, length = 3)

Please spell out the type.

Copy link
Contributor

@AlekseyTs AlekseyTs Aug 29, 2017

Choose a reason for hiding this comment

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

FirstOrDefault [](start = 67, length = 14)

It is not clear why FirstOrDefault is the right thing to do. It is not clear why are we doing this through DeclaringSyntaxReferences at all. I think that we should only handle top-level methods explicitly declared in source (not lambdas, not local functions, etc.) It'll probably make sense to do what MethodCompiler.BindMethodBody does rather than inventing some new way of getting the bound body, perhaps share some code with that method.

var node = syntaxReference.GetCSharpSyntax();
Copy link
Contributor

Choose a reason for hiding this comment

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

syntaxReference [](start = 23, length = 15)

Could syntaxReference be null here? Would it be safe to call GetCSharpSyntax in this case?

node = node.GetCodeBlockSyntax();
return node;
}
}
}
Loading