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 (v2) #21810

Closed
63 changes: 63 additions & 0 deletions src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1753,6 +1753,69 @@ internal PointerTypeSymbol CreatePointerTypeSymbol(TypeSymbol elementType)

#endregion

#region Operations

/// <summary>
/// Gets the low-level operation corresponding to the method's body.
/// </summary>
/// <param name="method">The method symbol.</param>
/// <param name="cancellationToken">An optional cancellation token.</param>
/// <returns>The low-level operation corresponding to the method's body.</returns>
protected override IOperation GetOperationCore(IMethodSymbol method, CancellationToken cancellationToken = default)
{
IOperation result = null;
var csmethod = method.EnsureCSharpSymbolOrNull<IMethodSymbol, MethodSymbol>(nameof(method));

if (csmethod != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

csmethod [](start = 16, length = 8)

Please add cast to object because symbols have user-defined comparison operator and we don't want it to be called here.

Copy link
Contributor

Choose a reason for hiding this comment

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

if (csmethod != null) [](start = 12, length = 21)

We also need to make sure the method is actually part of the source module of this compilation.

{
var body = LowerMethodBody(csmethod);

if (body != null)
{
var operationFactory = new Semantics.CSharpOperationFactory(null);
result = operationFactory.Create(body);
}
}

return result;
}

private BoundStatement LowerMethodBody(MethodSymbol method)
{
BoundStatement result = null;
var compilationState = new TypeCompilationState(method.ContainingType, this, null);
var diagnostics = DiagnosticBag.GetInstance();
var body = MethodCompiler.BindMethodBody(method, compilationState, diagnostics);
Copy link
Contributor

Choose a reason for hiding this comment

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

MethodCompiler.BindMethodBody [](start = 23, length = 29)

I think we should call this method only if the method symbol is SourceMemberMethodSymbol and BodySyntax is not null for it. For example, we probably don't want to return any tree for synthesized bodies (for auto-property accessors, etc.).


if (body != null && !body.HasErrors)
Copy link
Contributor

Choose a reason for hiding this comment

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

(body != null && !body.HasErrors) [](start = 15, length = 33)

We should also check if there are any errors in the diagnostics bag.

{
const int methodOrdinal = -1;
var dynamicAnalysisSpans = ImmutableArray<SourceSpan>.Empty;

result = LocalRewriter.Rewrite(
this,
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);
}

diagnostics.Free();
return result;
}

#endregion

#region Binding

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,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);
bool isImplicit = boundNode.WasCompilerGenerated;
Expand Down Expand Up @@ -426,7 +441,7 @@ private IEventAssignmentExpression CreateBoundEventAssignmentOperatorOperation(B
return new LazyEventAssignmentExpression(eventReference, handlerValue, adds, _semanticModel, syntax, type, constantValue, isImplicit);
}

private IParameterReferenceExpression CreateBoundParameterOperation(BoundParameter boundParameter)
private IParameterReferenceExpression CreateBoundParameterOperation(BoundParameter boundParameter)
{
IParameterSymbol parameter = boundParameter.ParameterSymbol;
SyntaxNode syntax = boundParameter.Syntax;
Expand Down Expand Up @@ -1064,7 +1079,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());
Copy link
Member

Choose a reason for hiding this comment

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

Where are we returning null for a BoundNode? This seems like a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

The CSharpOperationFactory.Create(BoundNode boundNode) method returns null if the boundNode parameter is null. This could happen when there is an optional operation child, like Instance property of IMemberReferenceExpression.


ImmutableArray<ILocalSymbol> locals = boundBlock.Locals.As<ILocalSymbol>();
SyntaxNode syntax = boundBlock.Syntax;
Expand Down Expand Up @@ -1506,5 +1521,59 @@ private IIsPatternExpression CreateBoundIsPatternExpressionOperation(BoundIsPatt
bool isImplicit = boundIsPatternExpression.WasCompilerGenerated;
return new LazyIsPatternExpression(expression, pattern, _semanticModel, syntax, type, constantValue, isImplicit);
}

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)

I don't think we should be filtering out None nodes.


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

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>);
bool isImplicit = boundConditionalGoto.WasCompilerGenerated;
return new LazyConditionalGotoStatement(condition, target, jumpIfTrue, _semanticModel, syntax, type, constantValue, isImplicit);
}

private ISequenceExpression CreateBoundSequenceOperation(BoundSequence boundSequence)
Copy link
Contributor

Choose a reason for hiding this comment

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

CreateBoundSequenceOperation [](start = 36, length = 28)

It looks like there are no tests for this node.

{
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.

Please fix the offset.

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 = 116, length = 28)

I don't think we should be filtering out None nodes.


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>);
bool isImplicit = boundSequence.WasCompilerGenerated;
return new LazySequenceExpression(expressions, value, locals, _semanticModel, syntax, type, constantValue, isImplicit);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Diagnostics;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Semantics;
using Microsoft.CodeAnalysis.Test.Utilities;
using Roslyn.Test.Utilities;
using Xunit;

namespace Microsoft.CodeAnalysis.CSharp.UnitTests
{
public partial class IOperationTests : SemanticModelTestBase
{
[Fact]
public void IConditionalGotoStatement_FromIf()
{
string source = @"
class C
{
/*<bind>*/
static void Method(int p)
{
if (p < 0) p = 0;
}
/*</bind>*/
}
";
string expectedOperationTree = @"
IBlockStatement (1 statements) (OperationKind.BlockStatement) (Syntax: '{ ... }')
IBlockStatement (3 statements) (OperationKind.BlockStatement) (Syntax: 'if (p < 0) p = 0;')
IConditionalGotoStatement (JumpIfTrue: False) (OperationKind.ConditionalGotoStatement) (Syntax: 'p < 0')
Copy link
Contributor

Choose a reason for hiding this comment

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

IConditionalGotoStatement (JumpIfTrue: False) (OperationKind.ConditionalGotoStatement) (Syntax: 'p < 0') [](start = 4, length = 104)

I cannot tell where this jumps to. Either the output should be adjusted so that could be obvious, or the test should verify relationship between labels by using other means.

Copy link
Contributor

Choose a reason for hiding this comment

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

JumpIfTrue: False [](start = 31, length = 17)

Please add tests for scenarios when JumpIfTrue is true.

Condition: IBinaryOperatorExpression (BinaryOperatorKind.LessThan) (OperationKind.BinaryOperatorExpression, Type: System.Boolean) (Syntax: 'p < 0')
Left: IParameterReferenceExpression: p (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'p')
Right: ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 0) (Syntax: '0')
IExpressionStatement (OperationKind.ExpressionStatement) (Syntax: 'p = 0;')
Expression: ISimpleAssignmentExpression (OperationKind.SimpleAssignmentExpression, Type: System.Int32) (Syntax: 'p = 0')
Left: IParameterReferenceExpression: p (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'p')
Right: ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 0) (Syntax: '0')
ILabeledStatement (OperationKind.LabeledStatement) (Syntax: 'if (p < 0) p = 0;')
Statement: null
";
var expectedDiagnostics = DiagnosticDescription.None;
VerifyOperationTreeAndDiagnosticsForTest<BaseMethodDeclarationSyntax>(source, expectedOperationTree, expectedDiagnostics, highLevelOperation: false);
}

[Fact]
public void IConditionalGotoStatement_FromIfElse()
{
string source = @"
class C
{
/*<bind>*/
static void Method(int p)
{
if (p < 0) p = 0;
else p = 1;
}
/*</bind>*/
}
";
string expectedOperationTree = @"
IBlockStatement (1 statements) (OperationKind.BlockStatement) (Syntax: '{ ... }')
IBlockStatement (6 statements) (OperationKind.BlockStatement) (Syntax: 'if (p < 0) ... else p = 1;')
IConditionalGotoStatement (JumpIfTrue: False) (OperationKind.ConditionalGotoStatement) (Syntax: 'p < 0')
Copy link
Member

Choose a reason for hiding this comment

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

Not entirely related to this PR, but perhaps we should be printing the Label regardless of whether it was implicitly generated or not. It would make this test easier to follow, as right now I can't see what will actually happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that the labels of IConditionalGotoStatement are automatically generated by the compiler when lowering the method body, so they have different names depending on the order in which the unit tests run.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should find a solution for this. For example, the visitor that prints the tree can track the synthesized label symbols it runs into in the process of visiting the tree and assign unique ids to them in a deterministic way. That id can be printed. In fact, we might want to assign ids like that to all label symbols because there could be labels with duplicate names and we should be able to tell them apart.

Condition: IBinaryOperatorExpression (BinaryOperatorKind.LessThan) (OperationKind.BinaryOperatorExpression, Type: System.Boolean) (Syntax: 'p < 0')
Left: IParameterReferenceExpression: p (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'p')
Right: ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 0) (Syntax: '0')
IExpressionStatement (OperationKind.ExpressionStatement) (Syntax: 'p = 0;')
Expression: ISimpleAssignmentExpression (OperationKind.SimpleAssignmentExpression, Type: System.Int32) (Syntax: 'p = 0')
Left: IParameterReferenceExpression: p (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'p')
Right: ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 0) (Syntax: '0')
IBranchStatement (BranchKind.GoTo) (OperationKind.BranchStatement) (Syntax: 'if (p < 0) ... else p = 1;')
Copy link
Contributor

Choose a reason for hiding this comment

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

IBranchStatement (BranchKind.GoTo) (OperationKind.BranchStatement) (Syntax: 'if (p < 0) ... else p = 1;') [](start = 4, length = 106)

I cannot tell where this jumps to.

ILabeledStatement (OperationKind.LabeledStatement) (Syntax: 'if (p < 0) ... else p = 1;')
Statement: null
IExpressionStatement (OperationKind.ExpressionStatement) (Syntax: 'p = 1;')
Expression: ISimpleAssignmentExpression (OperationKind.SimpleAssignmentExpression, Type: System.Int32) (Syntax: 'p = 1')
Left: IParameterReferenceExpression: p (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'p')
Right: ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 1) (Syntax: '1')
ILabeledStatement (OperationKind.LabeledStatement) (Syntax: 'if (p < 0) ... else p = 1;')
Statement: null
";
var expectedDiagnostics = DiagnosticDescription.None;
VerifyOperationTreeAndDiagnosticsForTest<BaseMethodDeclarationSyntax>(source, expectedOperationTree, expectedDiagnostics, highLevelOperation: false);
}
}
}
23 changes: 23 additions & 0 deletions src/Compilers/Core/Portable/Compilation/Compilation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1092,6 +1092,29 @@ protected abstract INamedTypeSymbol CommonCreateAnonymousTypeSymbol(

#endregion

#region Operations

/// <summary>
/// Gets the low-level operation corresponding to the method's body.
/// </summary>
/// <param name="method">The method symbol.</param>
/// <param name="cancellationToken">An optional cancellation token.</param>
/// <returns>The low-level operation corresponding to the method's body.</returns>
internal IOperation GetOperation(IMethodSymbol method, CancellationToken cancellationToken = default)
Copy link
Contributor

Choose a reason for hiding this comment

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

GetOperation [](start = 28, length = 12)

Should the low level nature of the result be reflected in the name of the API?

Copy link
Contributor

Choose a reason for hiding this comment

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

GetOperation [](start = 28, length = 12)

We should test this API for all different kinds of methods that can be declared in source: constructors, destructors, accessors, partial methods, etc. Also should test it for methods that have their bodies synthesized by compilers: default constructors, auto-property accessors, etc. For both languages.

Copy link
Contributor

Choose a reason for hiding this comment

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

GetOperation [](start = 28, length = 12)

Should also test with members of compiler synthesized types: anonymous types (both public and private anonymous type symbols), tuple types, etc.

{
return GetOperationCore(method, cancellationToken);
}

/// <summary>
/// Gets the low-level operation corresponding to the method's body.
/// </summary>
/// <param name="method">The method symbol.</param>
/// <param name="cancellationToken">An optional cancellation token.</param>
/// <returns>The low-level operation corresponding to the method's body.</returns>
protected abstract IOperation GetOperationCore(IMethodSymbol method, CancellationToken cancellationToken = default);

#endregion

#region Diagnostics

internal const CompilationStage DefaultDiagnosticsStage = CompilationStage.Compile;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
using System;
using System.Collections.Generic;
using System.Text;
using System.Threading;

namespace Microsoft.CodeAnalysis
{
public static class CompilationExtensions
{
/// <summary>
/// Gets the low-level operation corresponding to the method's body.
/// </summary>
/// <param name="compilation">The compilation containing the method symbol.</param>
/// <param name="method">The method symbol.</param>
/// <param name="cancellationToken">An optional cancellation token.</param>
/// <returns>The low-level operation corresponding to the method's body.</returns>
public static IOperation GetOperation(this Compilation compilation, IMethodSymbol method, CancellationToken cancellationToken = default)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMethodSymbol method [](start = 76, length = 20)

I think we should require this symbol to be a definition. I.e. should return nothing for a member of a constructed type, or for a constructed generic method. Please add tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively we could throw instead of returning null in this case.


In reply to: 136207763 [](ancestors = 136207763)

{
if (compilation == null)
{
throw new ArgumentNullException(nameof(compilation));
}

if (method == null)
{
throw new ArgumentNullException(nameof(method));
}

if (!compilation.IsIOperationFeatureEnabled())
{
throw new InvalidOperationException(CodeAnalysisResources.IOperationFeatureDisabled);
}

return compilation.GetOperation(method, cancellationToken);
}
}
}
Loading