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

Handle unspecified anonymous type property in VisualBasicOperationFactory #74091

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

cston
Copy link
Member

@cston cston commented Jun 20, 2024

Handle unspecified anonymous type property in VisualBasicOperationFactory.

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2063559

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 20, 2024
@cston cston marked this pull request as ready for review June 21, 2024 00:53
@cston cston requested a review from a team as a code owner June 21, 2024 00:53
@cston cston requested a review from a team June 21, 2024 18:22
Dim item = New C() With {.}
~
]]>.Value
Dim expectedFlowGraph = <![CDATA[
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 24, 2024

Choose a reason for hiding this comment

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

expectedFlowGraph

Please assert original IOperation tree for all added tests #Closed

Dim [property] As IPropertySymbol = DirectCast(boundAnonymousTypePropertyAccess.ExpressionSymbol, IPropertySymbol)
Return CreateBoundAnonymousTypePropertyAccessOperation(boundAnonymousTypePropertyAccess, [property])
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 24, 2024

Choose a reason for hiding this comment

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

Return CreateBoundAnonymousTypePropertyAccessOperation(boundAnonymousTypePropertyAccess, [property])

Alternatively, consider creating InvalidOperation when [property] Is Nothing and reverting changes in GetAnonymousTypeCreationInitializers. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that initially. However, TestOperationVisitor.VisitAnonymousObjectCreation() expects the initializer target to be an IPropertyReferenceOperation, and I assumed there might be consumers other than test code that also expect the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should simply change the assertion in TestOperationVisitor.VisitAnonymousObjectCreation, what it is doing is mostly driven by implementation. When we are fixing bugs, we are changing shape of IOperation tree even for success scenarios, not to mention error scenarios. The binder says, it doesn't know what the property is (that is what the bound node returns) and I think we should stick with that rather than trying to re-guess what that property might be. Otherwise, we probably should fix what the bound node returns, so that SemanticModel and IOperation tree produce consistent result.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Based on our offline discussion, I've left this as is, and added a check of the SemanticModel to the tests.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

Initializers(1):
ISimpleAssignmentOperation (OperationKind.SimpleAssignment, Type: ?, IsInvalid) (Syntax: '.')
Left:
IPropertyReferenceOperation: Property <anonymous type: $0 As ?>.$0 As ? (OperationKind.PropertyReference, Type: ?, IsInvalid) (Syntax: '')
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 25, 2024

Choose a reason for hiding this comment

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

Property <anonymous type: $0 As ?>.$0 As ?

I assume this is not consistent with symbol information returned by SemanticModel for the node. Let's confirm that and assert what SemanticModel API returns for the syntax node. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 3)

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 4)

@cston cston merged commit 602687a into dotnet:main Jun 25, 2024
24 checks passed
@cston cston deleted the with-missing-member branch June 25, 2024 21:11
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jun 25, 2024
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants