-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fix symbol info in identifier name in object initializer #74582
Fix symbol info in identifier name in object initializer #74582
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general approach looks fine to me, but I have a few comments. I don't like the new NoneOperation
in the tree here; it doesn't seem like it should be there.
src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/Semantics/ObjectAndCollectionInitializerTests.cs
Outdated
Show resolved
Hide resolved
bool isRef = refKind == RefKind.Ref; | ||
var rhsKind = isRef ? GetRequiredRHSValueKindForRefAssignment(boundLeft) : BindValueKind.RValue; | ||
// We fall back on simply binding the name as an expression for proper recovery | ||
// and also report a diagnostic about this being an invalid value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to explain why this is beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an explanation, lmk if it's okay
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>
@dotnet/roslyn-compiler for a second review. |
@dotnet/roslyn-compiler for a second review |
@Rekkonnect Could you resolve merge conflicts? |
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
Done with review pass (commit 9) |
@@ -5267,6 +5267,380 @@ void F(int x = 2, = 3) { } | |||
Assert.Same(symbol1.ContainingSymbol, symbol2.ContainingSymbol); | |||
} | |||
|
|||
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/74348")] | |||
public void ObjectInitializerIncompletePropertyValueDeclaration01() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR only improves handling incorrect object initializers in C#, leaving VB unaffected. Do we want to add tests showing the current state of similar cases in VB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add tests showing the current state of similar cases in VB?
I think we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the tests, lmk
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs
Outdated
Show resolved
Hide resolved
Done with review pass (commit 15) |
Done with review pass (commit 17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 18)
@333fred For the second review |
Closes #74348
We now bind simple identifier name expressions as
BoundObjectInitializerMember
when inside an object initializer. Additionally, we also handleBoundObjectInitializerMember
instances in the nullable walker and consider them as non null.This approach follows the suggestion by @CyrusNajmabadi to handle this fix in the binder instead of the parser to avoid breaking too many parsing tests in the process.