Fix Use Auto Property handling of VB's As New syntax#26592
Fix Use Auto Property handling of VB's As New syntax#26592sharwell merged 4 commits intodotnet:masterfrom
Conversation
| end class", | ||
| "class Class1 | ||
| readonly property P as Guid | ||
| = new Guid(""{00000000-0000-0000-0000-000000000000}"") |
There was a problem hiding this comment.
why do we need to change the syntax? i think this should be:
readonly property P as new Guid(""{00000000-0000-0000-0000-000000000000}"")There was a problem hiding this comment.
➡️ The language-agnostic code generator cannot currently distinguish between these two cases.
There was a problem hiding this comment.
if the user prefers that, we shouldn't change their code unnecessarily
There was a problem hiding this comment.
This request cannot be implemented without a substantial modification to the code generation infrastructure. As a bug fix for a situation capable of unexpectedly deleting important code, I do not believe the limitation should block progress on this fix.
There was a problem hiding this comment.
looking at the code above, I see VB specific code above GetFieldInitializer, not a language agnostic generator
There was a problem hiding this comment.
specifically this line:
statement = statement.WithInitializer(SyntaxFactory.EqualsValue(initializer))there should be something like WithAsClause
There was a problem hiding this comment.
Looking at this another way: prior to this pull request the following code is generated:
readonly property P as Guid
The form used in this pull request is objectively better than the previous implementation in both behavior and style, even if it does not represent the ideal end goal.
There was a problem hiding this comment.
➡️ This is now corrected per #26592 (comment)
| Dim identifier = TryCast(Await fieldSymbol.DeclaringSyntaxReferences(0).GetSyntaxAsync(cancellationToken).ConfigureAwait(False), ModifiedIdentifierSyntax) | ||
| Dim declarator = TryCast(identifier?.Parent, VariableDeclaratorSyntax) | ||
| Return declarator?.Initializer?.Value | ||
| Dim initializer = declarator?.Initializer?.Value |
There was a problem hiding this comment.
this seems a little unnecessary too - we're getting the expression from the initializer only to then construct a new initializer above - we could just return the initializer itself, together with the as clause as a tuple maybe?
There was a problem hiding this comment.
then a few lines above the code would be changed to
statement = statement.WithInitializer(initializer).WithAsClause(asClause)There was a problem hiding this comment.
➡️ This change could be implemented as a follow-up improvement, but is outside the scope of the bug fix. The code changes for this pull request were intentionally kept to a minimum and to match d2dc228.
There was a problem hiding this comment.
well it's sort of a bug fix that introduces another small bug :-/ i think the proper fix should be really easy and probably add even less lines of code than this one
There was a problem hiding this comment.
📝 If I don't make a change here, I'll file a new issue in dotnet/roslyn so this feedback is not lost. The request is certainly reasonable.
There was a problem hiding this comment.
➡️ I went ahead and made this change since, unlike the commit referenced above, this code fix does not use the language-agnostic syntax generator for this part.
| Dim declarator = TryCast(identifier?.Parent, VariableDeclaratorSyntax) | ||
| Return declarator?.Initializer?.Value | ||
| Dim initializer = declarator?.Initializer | ||
| Dim asNewClause = TryCast(declarator.AsClause, AsNewClauseSyntax) |
There was a problem hiding this comment.
why is the trycast necessary?
There was a problem hiding this comment.
ah, i see why.might be worth a comment.
|
@jinujoseph For approval |
|
Approved to merge for 15.8.preview2 |
Fixes #26256
Ask Mode template not completed
Customer scenario
What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)
Bugs this fixes
(either VSO or GitHub links)
Workarounds, if any
Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW
Risk
This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code
Performance impact
(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")
Is this a regression from a previous update?
Root cause analysis
How did we miss it? What tests are we adding to guard against it in the future?
How was the bug found?
(E.g. customer reported it vs. ad hoc testing)
Test documentation updated?
If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.