Switch semantic fact helpers for detecting read/write/ref/out etc. to #30123
Conversation
use the newly added GetValueUsageInfo
|
retest this please |
|
@dotnet-bot retest this please |
|
retest roslyn-CI please |
| } | ||
|
|
||
| return false; | ||
| } |
| // has an IInvalidOperation for the invocation and argument syntax nodes, and | ||
| // we cannot correctly determine the RefKind of the argument from the operation tree. | ||
| // So we workaround this scenario by performing syntactic checks. | ||
| if (isInErrorContext && node is ExpressionSyntax expressionNode) |
There was a problem hiding this comment.
ick....
I'd prefer to wait on 18722 being addressed. Otherwise, we have two entirely different codepaths to ask the same question. Ensuring they're in sync and don't have problems is not something i'd prefer us to do.
There was a problem hiding this comment.
I agree that this is not an ideal workaround, but note that the remaining syntactic checks are purely around ref/out/in arguments, which is still not large amount of code in error path. We will discuss costing around IInvalidInvocationOperation next week, and I am going to push for us addressing it soon. If we end up identifying that this is too costly to fit into preview2 or dev16 timeframe, I will discuss with @jinujoseph on the best path ahead here.
|
|
||
| // Once https://github.com/dotnet/roslyn/issues/30116 is implemented, we can switch this API | ||
| // and other extension methods in this file that use this API to be language-agnostic extensions. | ||
| // TODO: File a tracking a track bug for this API cleanup. |
There was a problem hiding this comment.
unlike below, this is minor enough that i'm ok with us going in even before 30116 happening.
There was a problem hiding this comment.
However, thinking about it more... it's definitely a bit worrying to me. The problem here is that people have to know that depending on if you go node->op->value versus node->value, you can get different results... seems easy to make mistakes.
| => node.GetValueUsageInfo(semanticModel, cancellationToken).IsInRefOrOutContext(); | ||
|
|
||
| public static bool IsInInContext(this SyntaxNode node, SemanticModel semanticModel, CancellationToken cancellationToken) | ||
| => node.GetValueUsageInfo(semanticModel, cancellationToken).IsInInContext(); |
There was a problem hiding this comment.
i'm on the fence if these extensions have any value. Since we have the helpers on ValueUsageInfo, all this is buying is not having to say GetValueUsageInfo. However, i don't mind GetValueUsageInfo, it's like calling GEtSymbolInfo/GetTypeInfo/GetConstantValue/etc.
I feel like we don't need such a thin wrapper here.
There was a problem hiding this comment.
Oh, i see. This wraps both getting the IOperation and getting the ValueUsageInfo. Ideally, it feels like this would just be exposed off of hte SemanticModel.
There was a problem hiding this comment.
Yes, this thin wrapper makes it easier for clients to get this information without worrying about the underlying implementation details (IOperation/ValueUsageInfo).
There was a problem hiding this comment.
@mavasani Would it be worth considering exposing GetValueUsageInfo off of SemanticModel? it would fit very cleanly in with the shape of SemanticModel and how people can use it, or IOperation, depending on which is most natural for their use case.
There was a problem hiding this comment.
Yes, that sounds like a better API. I will do this change when we decide to move this PR forward.
There was a problem hiding this comment.
SGTM. Thanks for looking into this. I like that this can help simplify the IDE, and help improve the overall compiler APIs!
| return ValueUsageInfo.Write; | ||
| } | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| return ValueUsageInfo.Read; |
There was a problem hiding this comment.
is this the right default fall-out case? I'd almost prefer an assert here so we catch this and have to explicitly define all the cases.
| bool IsOnlyWrittenTo(SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken); | ||
| bool IsInOutContext(SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken); | ||
| bool IsInRefContext(SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken); | ||
| bool IsInInContext(SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken); |
There was a problem hiding this comment.
Overall, i'm on hte fence here. The changes are great. But i'm not happy wiht the inability to remove code due to the limitation in IOperation that we are basically hacking a bit around. I think i'd prefer us to fix at the least the IOp issue around invalid code first before moving here. Otherwise, i don't see this actually buying us much.
|
We also need to revert the |
|
Closing this very old stale PR. |
use the newly added GetValueUsageInfo helper, which uses semantics instead of just syntax.
Move code in Find-refs to use the new extension method GetValueUsageInfo