Enable "prefer 'this.' qualifier" on methods.#26188
Enable "prefer 'this.' qualifier" on methods.#26188jinujoseph merged 2 commits intodotnet:masterfrom
Conversation
|
|
||
| protected override void InitializeWorker(AnalysisContext context) | ||
| => context.RegisterOperationAction(AnalyzeOperation, OperationKind.FieldReference, OperationKind.PropertyReference, OperationKind.MethodReference); | ||
| => context.RegisterOperationAction(AnalyzeOperation, OperationKind.FieldReference, OperationKind.PropertyReference, OperationKind.MethodReference, OperationKind.Invocation); |
There was a problem hiding this comment.
- wrap plz.
- this seems odd. why do you need to explicitly handle invocation operations? an invocation will be of the form "expr(...)". why is it not sufficient to just process hte "expr" part and handle it if it is some sort of member reference?
There was a problem hiding this comment.
Yes, this is unfortunately a bug that creeped into IOperation where IInvocationOperation directly exposes the instance instead of the IMethodReference for the method being invoked. @AlekseyTs @333fred as FYI. I am not sure if we can fix this without a breaking change.
There was a problem hiding this comment.
Hrm.. in the case of things just being buggy, i think it might be good to just fix, document as a change in behavior and bite the bullet early before IOperation spreads out further and further.
There was a problem hiding this comment.
Also tagging @jinujoseph. I will file a bug to track this issue.
This also needs an API change as we probably have to replace IInvocationOperation.Instance with IMethodReferenceOperation MemberReference { get; }. Note this will break the existing analyzers based on 2.6 APIs that either use IInvocationOperation.Instance and/or rely on the tree shape for invocation operations.
There was a problem hiding this comment.
@CyrusNajmabadi should I close it and wait for #26206?
There was a problem hiding this comment.
No no. Please leave open. Let's see what the decision is. If we decide to not really change the shape of IOperation, we'll need this PR where you ergister for OperationKind.Invocation. if the shape is changed, then we'll still want this PR so we can unskip the tests. In either event, let's just put this in a holding pattern for now until other decisions are made.
There was a problem hiding this comment.
I agree with @CyrusNajmabadi - this is a correct fix for the current IOperation API/tree shape and no fix to #26206 should (ideally) break this implementation.
There was a problem hiding this comment.
@mavasani I assume you made a mistake with the issue number?
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
LGTM. But i'm surprised this was necessary. @mavasani does this make sense to you? Do you really hve to go through IInvocationOperatoin to figure out what members you are referncing, instead of just using the expression part of the invocation?
|
@zaytsev-victor Can you please resolve merge conflicts? Tagging @jinujoseph for ask mode approval. |
|
Approved to merge to 15.8.Preview3 once conflicts are resolved and test being green |
# Conflicts: # src/Features/Core/Portable/QualifyMemberAccess/AbstractQualifyMemberAccessDiagnosticAnalyzer.cs
|
retest windows_debug_spanish_unit32_prtest please |
|
retest windows_release_vs-integration_prtest please |
Fixed #25981
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.