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.

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

Add IOperation support for tuple expressions #19636

Closed
wants to merge 1 commit into from
Closed

Add IOperation support for tuple expressions #19636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 0 additions & 10 deletions src/Compilers/CSharp/Portable/BoundTree/Expression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,9 @@
// 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.Collections.Immutable;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.Semantics;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
{
internal partial class BoundTupleExpression
{
protected override ImmutableArray<BoundNode> Children => StaticCast<BoundNode>.From(this.Arguments);
}

internal partial class BoundDelegateCreationExpression
{
protected override ImmutableArray<BoundNode> Children => ImmutableArray.Create<BoundNode>(this.Argument);
Expand Down
13 changes: 13 additions & 0 deletions src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ private static IOperation CreateInternal(BoundNode boundNode)
return CreateBoundLabeledStatementOperation((BoundLabeledStatement)boundNode);
case BoundKind.ExpressionStatement:
return CreateBoundExpressionStatementOperation((BoundExpressionStatement)boundNode);
case BoundKind.TupleLiteral:
case BoundKind.ConvertedTupleLiteral:
return CreateBoundTupleExpressionOperation((BoundTupleExpression)boundNode);
default:
var constantValue = ConvertToOptional((boundNode as BoundExpression)?.ConstantValue);
return Operation.CreateOperationNone(boundNode.HasErrors, boundNode.Syntax, constantValue, getChildren: () => GetIOperationChildren(boundNode));
Expand Down Expand Up @@ -985,5 +988,15 @@ private static IExpressionStatement CreateBoundExpressionStatementOperation(Boun
Optional<object> constantValue = default(Optional<object>);
return new LazyExpressionStatement(expression, isInvalid, syntax, type, constantValue);
}

private static ITupleExpression CreateBoundTupleExpressionOperation(BoundTupleExpression boundTupleExpression)
{
Lazy<ImmutableArray<IOperation>> elements = new Lazy<ImmutableArray<IOperation>>(() => GetTupleElements(boundTupleExpression));
Copy link
Member

@jcouv jcouv May 23, 2017 •
edited by mavasani
Loading

Choose a reason for hiding this comment

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

Nit: you could omit the type (use var) since type is spelled out on the right-hand-side. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have followed the existing pattern used in this file @heejaechang probably we can clean this up for the entire file in one of your refactorings?

Copy link
Contributor

Choose a reason for hiding this comment

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

Use of var is not a requirement. It is the other way around, we forbid use of var in some cases.

Copy link
Member

Choose a reason for hiding this comment

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

This lambda (and all others above) are creating closures. The compiler generally tries to avoid this, so I'm surprised to see this a recurring pattern in this file. Maybe something to consider for a follow-up refactoring as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, certainly. Let me file an issue to track all your review comments here.. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #19720

bool isInvalid = boundTupleExpression.HasErrors;
SyntaxNode syntax = boundTupleExpression.Syntax;
ITypeSymbol type = boundTupleExpression.Type;
Optional<object> constantValue = ConvertToOptional(boundTupleExpression.ConstantValue);
Copy link
Member

@jcouv jcouv May 23, 2017 •
edited by mavasani
Loading

Choose a reason for hiding this comment

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

I don't think tuple can ever have a constant value. #WontFix

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think tuple can ever have a constant value.

The bound node will reflect that.

Copy link
Member

Choose a reason for hiding this comment

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

I would keep this as is. If tuples could ever become constnat, then this code will continue to work.

return new LazyTupleExpression(elements, isInvalid, syntax, type, constantValue);
}
}
}
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory_Methods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,15 @@ private static ImmutableArray<IVariableDeclaration> GetVariableDeclarationStatem
OperationFactory.CreateVariableDeclaration(declaration.LocalSymbol, Create(declaration.InitializerOpt), declaration.Syntax)));
}

private static readonly ConditionalWeakTable<BoundTupleExpression, object> s_tupleElementsMappings =
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to have specific type (as opposed to object) for the values stored in the table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heejaechang? Comment tracked in #19720

Copy link
Member

Choose a reason for hiding this comment

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

The problem with CWT is that it has a 'class' constraint on the values. But we're storing ImmutableArrays. So we end up needing to box things into object to satisfy the CWT.

new ConditionalWeakTable<BoundTupleExpression, object>();

private static ImmutableArray<IOperation> GetTupleElements(BoundTupleExpression boundTupleExpression)
{
return (ImmutableArray<IOperation>)s_tupleElementsMappings.GetValue(boundTupleExpression,
tupleExpr => tupleExpr.Arguments.SelectAsArray(element => Create(element)));
}

// TODO: We need to reuse the logic in `LocalRewriter.MakeArguments` instead of using private implementation.
// Also. this implementation here was for the (now removed) API `ArgumentsInParameter`, which doesn't fulfill
// the contract of `ArgumentsInEvaluationOrder` plus it doesn't handle various scenarios correctly even for parameter order,
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Test/Semantic/CSharpCompilerSemanticTest.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
<Compile Include="Diagnostics\DiagnosticAnalyzerTests.cs" />
<Compile Include="Diagnostics\GetDiagnosticsTests.cs" />
<Compile Include="IOperation\IOperationTests_IArgument.cs" />
<Compile Include="IOperation\IOperationTests_ITupleExpression.cs" />
<Compile Include="IOperation\IOperationTests_IIfStatement.cs" />
<Compile Include="IOperation\IOperationTests_IFieldReferenceExpression.cs" />
<Compile Include="IOperation\IOperationTests_IObjectCreationExpression.cs" />
Expand Down
8 changes: 4 additions & 4 deletions ...ompilers/CSharp/Test/Semantic/IOperation/IOperationTests_IParameterReferenceExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ public void M(int x, int y)
}
";
string expectedOperationTree = @"
IOperation: (OperationKind.None) (Syntax: '(x, x + y)')
Children(2): IParameterReferenceExpression: x (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'x')
ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32 x, System.Int32)) (Syntax: '(x, x + y)')
Elements(2): IParameterReferenceExpression: x (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'x')
Copy link
Member

@jcouv jcouv May 23, 2017 •
edited by mavasani
Loading

Choose a reason for hiding this comment

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

In terms of formatting, maybe IParameterReferenceExpression: x ... should be on the next line, indented with IBinaryOperatorExpression ...? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we hope to make that change in the dump format, but it would require updating almost all the IOperation tests. We have it in our backlog though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #19866

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

IBinaryOperatorExpression (BinaryOperationKind.IntegerAdd) (OperationKind.BinaryOperatorExpression, Type: System.Int32) (Syntax: 'x + y')
Left: IParameterReferenceExpression: x (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'x')
Right: IParameterReferenceExpression: y (OperationKind.ParameterReferenceExpression, Type: System.Int32) (Syntax: 'y')
Expand Down Expand Up @@ -66,8 +66,8 @@ public void M(Point point)
";
string expectedOperationTree = @"
IOperation: (OperationKind.None) (Syntax: 'var (x, y) = point')
Children(2): IOperation: (OperationKind.None) (Syntax: 'var (x, y)')
Children(2): ILocalReferenceExpression: x (OperationKind.LocalReferenceExpression, Type: System.Int32) (Syntax: 'x')
Children(2): ITupleExpression (OperationKind.TupleExpression, Type: (System.Int32 x, System.Int32 y)) (Syntax: 'var (x, y)')
Elements(2): ILocalReferenceExpression: x (OperationKind.LocalReferenceExpression, Type: System.Int32) (Syntax: 'x')
ILocalReferenceExpression: y (OperationKind.LocalReferenceExpression, Type: System.Int32) (Syntax: 'y')
IConversionExpression (ConversionKind.Invalid, Implicit) (OperationKind.ConversionExpression, Type: (System.Int32 x, System.Int32 y)) (Syntax: 'point')
IParameterReferenceExpression: point (OperationKind.ParameterReferenceExpression, Type: Point) (Syntax: 'point')
Expand Down
Loading