Fix issue where a write wasn't detected in make-readonly #2#26303
Fix issue where a write wasn't detected in make-readonly #2#26303jinujoseph merged 4 commits intodotnet:masterfrom
Conversation
jcouv
added
Area-IDE
Community
|
@mavasani @heejaechang could you take a look? |
| return false; | ||
| } | ||
|
|
||
| public static bool IsAttributeNamedArgumentIdentifier(this ExpressionSyntax expression) |
There was a problem hiding this comment.
i moved this function down as it was accidentally added between IsWrittenTo and IsOnlyWrittenTo.
| /// | ||
| /// copied from SyntaxExtensions.GetContainingDeconstruction | ||
| /// </summary> | ||
| private static bool IsExpressionOfArgumentInDeconstruction(ExpressionSyntax expr) |
There was a problem hiding this comment.
talked to @jcouv and he says this is hte right way to determine if you're on the assignment part of a deconstruction.
There was a problem hiding this comment.
@jcouv Wasn't there some "get me the deconstruction info" API we could call instead? Even if we don't use it, just call it and check if the result is null? My worry being this is subtle and probably subject to change as the compiler does new things.
There was a problem hiding this comment.
There is a semantic model API has a GetDeconstructionInfo. Looking at it, it seems I didn't add compiler tests (filed #26352 just now) :-S
I think it will only return something if you pass in an assignment or foreach syntax nodes.
That means you could indeed just check that you're on the left of an assignment (unwrapping tuples) and that the assignment is a deconstruction (using GetDeconstructionInfo).
I'm not sure that buys much (you still need to walk up the node's ancestors to find an assignment or foreach), and that's doing some binding work, so I'd recommend sticking with a purely syntactic check, as currently implemented in the PR.
There was a problem hiding this comment.
@jcouv How does this appear through IOperatoin? is is possible it's better? or would we still be lacking the info that this is a write and not a read?
|
@jinujoseph @dotnet/roslyn-ide can i get another pair of eyes on this simple PR? |
|
@jcouv Feel free to merge. @jasonmalinowski Thanks for the review! |
4 participants
Followup to #26301
Fixes #26264