Add IOperation support for anonymous object creation expressions. #20364
Conversation
| @@ -95,6 +96,33 @@ internal static IArgument CreateArgumentOperation(ArgumentKind kind, IParameterS | |||
| invokedAsExtensionMethod: invokedAsExtensionMethod); | |||
| } | |||
|
|
|||
| private ImmutableArray<IOperation> GetAnonymousObjectCreationInitializers(BoundAnonymousObjectCreationExpression expression) | |||
| { | |||
| // For error cases, the binder generates only the argument. | |||
There was a problem hiding this comment.
See #20338 for additional gotchas here. #Resolved
| @@ -705,6 +705,68 @@ public static MetadataReference CreateMetadataReferenceFromIlSource(string ilSou | |||
| return CompileAndVerify(comp); | |||
| } | |||
|
|
|||
| protected List<SyntaxNode> GetSyntaxNodeList(SyntaxTree syntaxTree) | |||
There was a problem hiding this comment.
Code just moved from SemanticModelTestBase to CSharpTestBase. #Resolved
| @@ -788,4 +788,59 @@ Public MustInherit Class BasicTestBaseBase | |||
| End Function | |||
| End Class | |||
|
|
|||
| #Region "IOperation tree validation" | |||
There was a problem hiding this comment.
Code just moved from SemanticModelTestBase to BasicTestBase.
|
Tagging @dotnet/analyzer-ioperation @dotnet/roslyn-compiler for review. #Resolved |
| /// <summary> | ||
| /// Represents a C# or VB new/New anonymous object creation expression. | ||
| /// </summary> | ||
| internal sealed partial class AnonymousObjectCreationExpression : BaseAnonymousObjectCreationExpression, IAnonymousObjectCreationExpression |
There was a problem hiding this comment.
AnonymousObjectCreationExpression [](start = 34, length = 33)
I didn't see where AnonymousObjectCreationExpression is used. Can you clarify why declare three types: base, lazy, and non-lazy anonymous object creation? #Pending
There was a problem hiding this comment.
Tagging @heejaechang again for more context, HeeJae can correct me if I am wrong - I think the idea here is to have a definition for both the Lazy and non-lazy versions of each Operation node, with the latter being useful for creating a fully populated tree, which is not being used currently, but we plan to use in future.
There was a problem hiding this comment.
If they are not used, I don't think they should be in the PR.
In reply to: 123576594 [](ancestors = 123576594)
There was a problem hiding this comment.
The base class is used, so ok to keep. The one that is not used, if I understand correctly, is LazyAnonymousObjectCreationExpression.
In reply to: 124861675 [](ancestors = 124861675,123576594)
There was a problem hiding this comment.
There was a problem hiding this comment.
Julien, I have added comment to #20204 to address your feedback. Tagging @heejaechang to confirm the above reasoning.
In reply to: 125327101 [](ancestors = 125327101,124862019,124861675,123576594)
| /// <summary> | ||
| /// Explicitly-specified member initializers. | ||
| /// </summary> | ||
| public override ImmutableArray<IOperation> Initializers { get; } |
There was a problem hiding this comment.
Should the Initializers use a more specific type/interface? I see this was considered in the design notes, but all the tests show IAssignmentExpression goes there. Is it possible to get anything else? #Closed
There was a problem hiding this comment.
Nevermind, I found one example of some other type (in test AnonymousTypeSymbols_BaseAccessInMembers_OperationTree).
In reply to: 123607488 [](ancestors = 123607488)
There was a problem hiding this comment.
Yes, I initially had IAssignmentExpression, but there are scenarios where we can get different IOperation for error cases. #Resolved
|
Ping any more feedback? #Resolved |
| } | ||
|
|
||
| IOperation target = Create(expression.Declarations[i]); | ||
| bool isInvalid = target.IsInvalid || value.IsInvalid; |
There was a problem hiding this comment.
Is it necessary to consider the entire assignment invalid if either target or value are invalid? Will that mean analyzers will typically skip the valid parts of the assignment? #Pending
There was a problem hiding this comment.
@heejaechang Is actually working on a PR to remove IOperation.IsInvalid flag. It is currently a very fragile heuristic, and we are replacing it with an extension method on IOperation that determines the result by computing if the span has syntax or semantic diagnostics within it.
| var o = (Action)(() => (/*<bind>*/new { x = 1, y = new { } }/*</bind>*/).ToString()); ; | ||
| } | ||
| } | ||
| "; |
There was a problem hiding this comment.
What is different about anonymous types created in a lambda?
That said, perhaps reference locals or parameters from the containing method, if there are not already tests that cover captured variables, although captured variables can be tested separately from anonymous types.
There was a problem hiding this comment.
Nothing really, I was just creating cloned IOperation tests for existing tests in this source files.
In reply to: 123879628 [](ancestors = 123879628)
| } | ||
| "; | ||
| string expectedOperationTree = @" | ||
| IBlockStatement (2 statements, 2 locals) (OperationKind.BlockStatement, IsInvalid) (Syntax: '{ ... }') |
There was a problem hiding this comment.
Why is the block marked invalid? #Pending
There was a problem hiding this comment.
Same issue as others - this flag is current based off BoundNode.HasErrors. @heejaechang is planning to replace this with an IsInValid extension method.
In reply to: 123879687 [](ancestors = 123879687)
| IBlockStatement (2 statements, 2 locals) (OperationKind.BlockStatement, IsInvalid) (Syntax: '{ ... }') | ||
| Locals: Local_1: System.Object v1 | ||
| Local_2: System.Object v2 | ||
| IVariableDeclarationStatement (1 declarations) (OperationKind.VariableDeclarationStatement, IsInvalid) (Syntax: 'object v1 = ... };') |
There was a problem hiding this comment.
Why is the variable declaration invalid? #Pending
There was a problem hiding this comment.
Same issue as above - IsInvalid needs to be changed.
In reply to: 123879697 [](ancestors = 123879697)
| { | ||
| public static void Test1(int x) | ||
| { | ||
| using (var v1 = /*<bind>*/new { }/*</bind>*/) |
There was a problem hiding this comment.
Consider <bind> around entire declaration rather than around anonymous type expression since the expression is not invalid.
There was a problem hiding this comment.
| Left: IPropertyReferenceExpression: System.Double <anonymous type: System.Int32 aa, System.String $1, System.Double bb>.bb { get; } (Static) (OperationKind.PropertyReferenceExpression, Type: System.Double) (Syntax: 'bb') | ||
| Right: IFieldReferenceExpression: System.String ClassA.aa (Static) (OperationKind.FieldReferenceExpression, Type: System.String) (Syntax: 'ClassA.aa') | ||
| ILiteralExpression (Text: 1.2) (OperationKind.LiteralExpression, Type: System.Double, Constant: 1.2) (Syntax: '1.2') | ||
| "; |
There was a problem hiding this comment.
I may be misreading the expected result, but it looks like the string represents { aa = 1, bb = ClassA.aa, 1.2 }. #Resolved
| @@ -705,6 +705,68 @@ public static MetadataReference CreateMetadataReferenceFromIlSource(string ilSou | |||
| return CompileAndVerify(comp); | |||
| } | |||
|
|
|||
| protected List<SyntaxNode> GetSyntaxNodeList(SyntaxTree syntaxTree) | |||
There was a problem hiding this comment.
Can all of these members be static? #Resolved
There was a problem hiding this comment.
|
@cston addressed the feedback. #Resolved |
|
LGTM |
|
@dotnet/roslyn-infrastructure Seems like ubuntu_16_debug_prtest failed due to an unrelated build error: |
|
@dotnet-bot retest ubuntu_16_debug_prtest please |
|
@jcouv Can you please take another look? #Resolved |
|
Ping, I need one more compiler sign off. #Resolved |
|
cc @dotnet/roslyn-compiler |
| private IPropertyReferenceExpression CreateBoundAnonymousPropertyDeclarationOperation(BoundAnonymousPropertyDeclaration boundAnonymousPropertyDeclaration) | ||
| { | ||
| PropertySymbol property = boundAnonymousPropertyDeclaration.Property; | ||
| Lazy<IOperation> instance = new Lazy<IOperation>(() => null); |
There was a problem hiding this comment.
Why have a lazy that always returns null? Seems like null coudl just be used directly here. #Resolved
There was a problem hiding this comment.
I have followed the pattern used in this file. @heejaechang Will having a null child node be handled by the operation factory?
I have added a comment to #20204 to track this feedback for the entire file. #Resolved
| case BoundKind.ConstantPattern: | ||
| return CreateBoundConstantPatternOperation((BoundConstantPattern)boundNode); | ||
| case BoundKind.DeclarationPattern: | ||
| return CreateBoundDeclarationPatternOperation((BoundDeclarationPattern)boundNode); | ||
| case BoundKind.WildcardPattern: | ||
| return null; |
There was a problem hiding this comment.
I didn't understand this change. How do you know this is unreachable? #Closed
There was a problem hiding this comment.
@jcouv - This is addressing feedback from #20276 (comment). We handle this bound node while operation on it's parent here: https://github.com/dotnet/roslyn/pull/20276/files#diff-df80689b64fbe29db28a562eb0dac8caR1154 #Resolved
| } | ||
|
|
||
| protected const string startString = "/*<bind>*/"; | ||
| protected const string endString = "/*</bind>*/"; |
There was a problem hiding this comment.
Accessible members should be PascalCased not camelCased. #Resolved
There was a problem hiding this comment.
This is not new code. I am moving the existing code from SemanticModelTestBase to CSharpTestBase. I'll create a separate compiler test bug to track this work. #Resolved
There was a problem hiding this comment.
Filed #20611 #Resolved
There was a problem hiding this comment.
I went ahead and addressed the test feedback in the next commit. #Resolved
|
|
||
| if (exprFullText.StartsWith(startString, StringComparison.Ordinal)) | ||
| { | ||
| if (exprFullText.Contains(endString)) |
There was a problem hiding this comment.
Braces are required for anything other than single line if statements. Please add them here. #Resolved
There was a problem hiding this comment.
Ditto, existing compiler test code being moved around. #Resolved
|
|
||
| if (exprFullText.EndsWith(endString, StringComparison.Ordinal)) | ||
| { | ||
| if (exprFullText.Contains(startString)) |
There was a problem hiding this comment.
Existing compiler test code being moved around #Resolved
mavasani
force-pushed
the
IAnonymousObjectCreation
branch
from
0b3bd6e to
9bd67cc
Compare
| SyntaxNode syntax = boundAnonymousPropertyDeclaration.Syntax; | ||
| ITypeSymbol type = boundAnonymousPropertyDeclaration.Type; | ||
| Optional<object> constantValue = ConvertToOptional(boundAnonymousPropertyDeclaration.ConstantValue); | ||
| return new LazyPropertyReferenceExpression(property, instance, member, argumentsInEvaluationOrder, isInvalid, syntax, type, constantValue); |
There was a problem hiding this comment.
Is this change (adding argumentsInEvaluationOrder) covered by test?
There was a problem hiding this comment.
Currently the dumper doesn't print out child operation arrays which are empty, hence we cannot cover it in tests. @jinujoseph is working on updating the operation tree dumper to print a header with 0 element count for this case, at which point the unit tests will cover it.
mavasani
force-pushed
the
IAnonymousObjectCreation
branch
from
0b15671 to
f49daed
Compare
Fixes #19926