Handle indexers in nested object initializers #48168
Conversation
…they are object initializers. This only has visible impact on property accesses with arguments on the getter, meaning class indexers.
333fred
force-pushed
the
iop-crash-indexers
branch
from
43da26b
to
ce06e42
Compare
|
@333fred It looks like there are legitimate CI failures. #Closed |
|
@AlekseyTs should be addressed now. @dotnet/roslyn-compiler for reviews please. |
src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory_Methods.cs
Show resolved
Hide resolved
| MethodSymbol accessor = isObjectOrCollectionInitializer ? property.GetOwnOrInheritedGetMethod() : property.GetOwnOrInheritedSetMethod(); | ||
| var property = (PropertySymbol?)boundObjectInitializerMember.MemberSymbol; | ||
| MethodSymbol? accessor = isObjectOrCollectionInitializer ? property.GetOwnOrInheritedGetMethod() : property.GetOwnOrInheritedSetMethod(); | ||
| Debug.Assert(property is not null); |
There was a problem hiding this comment.
Debug.Assert(property is not null); [](start = 24, length = 35)
Should the assert be moved right after the declaration of property? It looks like it is dereferenced in the previous statement. #Closed
There was a problem hiding this comment.
No. Those are extension methods that accept a null input. I could certainly move it, but it doesn't particularly matter.
There was a problem hiding this comment.
Those are extension methods that accept a null input. I could certainly move it, but it doesn't particularly matter.
I think that would benefit the reader.
In reply to: 497941233 [](ancestors = 497941233)
| { | ||
| while ((object)property != null) | ||
| while (property is not null) |
There was a problem hiding this comment.
is not [](start = 28, length = 6)
It looks like this PR doesn't follow the general guideline to keep unrelated refactorings out of PRs that change product behavior. Also, in general we prefer avoiding making stylistic changes to existing code. Everybody has personal preferences and if analyzer doesn't flag the code, it is in a good state style/formatting wise. #Closed
There was a problem hiding this comment.
Since this is an ask mode fix, I would prefer changes like that reverted and keeping this PR to a necessary minimum to address the issue.
In reply to: 497930116 [](ancestors = 497930116)
There was a problem hiding this comment.
These changes are what allowed me to figure out the bug here. My strategy was to nullable enable the callstack, which allowed me to figure out where the bug was coming from. I'd prefer to keep them.
There was a problem hiding this comment.
These changes are what allowed me to figure out the bug here. My strategy was to nullable enable the callstack, which allowed me to figure out where the bug was coming from. I'd prefer to keep them.
Changes to comparisons to null and the like are unnecessary. I think they should be reverted and any other changes that are not meant to change product behavior. Nullable annotations and asserts are fine, of course.
In reply to: 497940563 [](ancestors = 497940563)
|
Done with review pass (iteration 3) #Closed |
|
Hello @333fred! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
When I removed the usage of
Lazy<T>from IOperation, I mishandled setting whether a property initializer in a nested object initializer expression is a object initializer itself. This meant that during the build of the parent node, we'd correctly check the setter for the property to ensure that it was valid, but then when handling deriving the arguments for the child node we'd use the wrong method (the getter, instead of the setter). Practically, this only had observably bad results from indexers: indexers without a get method would cause the code to null ref. This is because when deriving the arguments for the getter for non-indexed properties, we'd use the getter, and then immediately bail out because the getter had no arguments. Fixes #45692.Commit-by-commit review is recommended. Commit 1 nullable enables code so that I could figure out what the bug was. The second commit actually fixes the bug.