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 field reference from semantic model in non-field-backed property #75904

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

cston
Copy link
Member

@cston cston commented Nov 13, 2024

Fixes #75893

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 13, 2024
@cston cston marked this pull request as ready for review November 13, 2024 23:31
@cston cston requested a review from a team as a code owner November 13, 2024 23:31
@cston cston requested a review from a team November 14, 2024 00:20
[WorkItem("https://github.com/dotnet/roslyn/issues/75893")]
[Theory]
[CombinatorialData]
public void SpeculativeSemanticModel(bool includeLocal)
Copy link
Member

@Youssef1313 Youssef1313 Nov 14, 2024

Choose a reason for hiding this comment

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

Just FYI, doesn't have to be done in this PR (or even done at all), but when I previously worked on the feature, I have written relatively large amount of tests which also include SpeculativeSemanticModel. I haven't checked if the exact case in this test is one of those I wrote, but generally I think it may be a good idea to copy those tests. Some test outcomes may be different from the now-expected behavior and may require adjustments though.

The tests are here: https://github.com/dotnet/roslyn/blob/32d6c1db46fd613888d08970908257fbacf328ba/src/Compilers/CSharp/Test/Emit2/Semantics/PropertyFieldKeywordTests.cs

There was also this test:

public void TestWinMdExpData_SemiAutoProperty()

And this:

https://github.com/dotnet/roslyn/blob/32d6c1db46fd613888d08970908257fbacf328ba/src/Compilers/CSharp/Test/Semantic/Semantics/InteractiveSemanticModelTests.cs#L99C21-L99C37

And

public void TestGetSpeculativeSemanticModelInAutoPropInitializer1(string property)
, and finally https://github.com/dotnet/roslyn/blob/32d6c1db46fd613888d08970908257fbacf328ba/src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs#L6959C21-L6959C52

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 @Youssef1313 for links to your tests from the earlier branch. I had previously referenced the tests in PropertyFieldKeywordTests in #75398, and I've added a link to your comment there. I'll investigate the tests as part of that issue. In the meantime, this particular issue seemed straightforward enough to handle directly.

@@ -1465,9 +1465,12 @@ private BoundExpression BindFieldExpression(FieldExpressionSyntax node, BindingD
}
}

// Field will be null when binding a field expression in a speculative
// semantic model when the property does not have a backing field.
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 14, 2024

Choose a reason for hiding this comment

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

in a speculative semantic model when the property does not have a backing field.

Consider if we could assert the fact. At least partially. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't obvious what to assert here since the Binder is not specific to the semantic model (IsSemanticModelBinder == false), even when called from that code path.

[WorkItem("https://github.com/dotnet/roslyn/issues/75893")]
[Theory]
[CombinatorialData]
public void SpeculativeSemanticModel(bool includeLocal)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 14, 2024

Choose a reason for hiding this comment

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

SpeculativeSemanticModel

Consider adding a test for a success scenario (speculating with field works in a field-backed property), if we don't have one already. #Resolved

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 2)

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.

visual studio provider errors with use of field keyword - when typing
6 participants