Remove IBuiltInAnalyzer.OpenFileOnly#26590
Conversation
768c4a9 to
59f5b85
Compare
|
Help me understand the behavior change here? |
|
When it returned true, the
This pull request recognizes that an analyzer containing only diagnostics with severity Hidden also provides these guarantees. During review, I found that all analyzers returning
|
|
Note: with this change, the only purpose of IBuiltInAnalyzer is to support the 'GetAnalyzerCategory' call. It would be very good if we could remove that last call somehow (or make it public). It seems to serve a very important purpose for performance, but at hte same time is kept completely internal preventing 3rd party analyzers from benefitting from that perf entrypoint. I reviewed our existing use of this API and even discovered a bunch of cases where we get it wrong. I opened up #26671 to address this. |
There was a problem hiding this comment.
Does this mean users can never enforce squiggles in the IDE for unnecessary usings? I presume the project system requires this behavior. They configure RemoveUnnecessaryImportsDiagnosticId to be an error over here: <Rule Id="IDE0005" Action="Error" /> <!-- Using directive is unnecessary using System.Text;
There was a problem hiding this comment.
Oh, why is this tied to this change?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I read that and still don't understand context. Assume I know nothing about how this works; why is it broken? It works in my limited testing.
There was a problem hiding this comment.
@mavasani @davkean IDE0005 needs two changes before it can be configured:
- Needs to avoid reporting the errors in generated code (but keep reporting a hidden diagnostic for fading unnecessary code)
- Needs to be able to run OOP (it may already, but this needs to be confirmed)
In the meantime the configuration will be silently ignored, and then when the diagnostic is properly enabled it will become active again.
|
@sharwell I like the intent of the PR, thanks for starting cleanup in this area. Finally we are working towards getting rid of this internal (hacky) interface!! However, I am not sure about the |
No, that is not a correct representation of our position. I want the Error List to always show the correct state, I don't care if I have a file open or not. Showing errors only when a file is open is very confusing. |
Ah, sorry to have misinterpreted. Regardless, I feel the current change will mean unused usings can never be configured to be an error/warning. @sharwell Is it necessary to add these |
|
@mavasani IDE0005 is impacted by this, but there is definitely intent to fix. I moved #22579 back into the 15.8 milestone to accompany this. There is one other scenario impacted: For users with Full Solution Analysis enabled, the error list will now show Suggestion messages for diagnostics at severity Info for a subset of our diagnostics that enabled Open File Only mode for severities less than Warning (with Info being the only impacted one). This is arguably the correct behavior anyway, especially since it aligns with the behavior of all third-party analyzers. |
|
@sharwell I am still slightly uncomfortable about tying 2 concepts here (severity and scope of execution) to one data point (severity). For example, if someone now bumps up the severity of any diagnostic ID reported by I personally prefer keeping these two things separate, possibly by implementing #12713 or by introducing a new IDE specific API to |
It already does this. In addition, the default severity of this analyzer is Hidden, so it will not be impacted by this change unless users have customized the severity specifically to Info and have FSA enabled.
I'm not fundamentally opposed to expanding the API for this. However, I do not believe we are currently using |
This internal method overlaps the behavior of DiagnosticSeverity.Hidden. The the few analyzers relying on it for correctness already reported Hidden diagnostics by default. These analyzers are updated to include NotConfigurable, ensuring that Hidden severity will be used for the diagnostics in all configurations. Fixes dotnet#15760 Fixes dotnet#16026 Closes dotnet#16700
|
well, I like things to be explicit since it is easy to check. when things are implicitly implied, things get fragile and require knowledge on implementation detail. for example, right now only closed file analysis is happening on OOP, so hidden has kind of same meaning of Open file only. but that's just how we implemented it and our current optimization. unless this remove some issue, not sure why we want to do this.. especially if IBuildInAnalyzer is still there, and there are more issue we need to resolve to remove this. |
There was a problem hiding this comment.
so at some point, we want to move open file only analyzers to OOP as well. but this one shouldn't move even at that point since it require rename service which will not exist in OOP side.
There was a problem hiding this comment.
➡️ This one will not run OOP even after this pull request.
There was a problem hiding this comment.
How is that controlled? Thanks!
There was a problem hiding this comment.
@CyrusNajmabadi It's described in the initial comment for this pull request.
59f5b85 to
b2d9e9b
Compare
| public override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticSpanAnalysis; | ||
|
|
||
| public override bool OpenFileOnly(Workspace workspace) | ||
| { |
There was a problem hiding this comment.
code style analyzer is weird one I believe, since engine is specifically rewritten to ignore code style analyzer's descriptor. it is special ability grant to only IDE code style analyzer which we want to fix at some point. since they return dynamic descriptor that are not allowed to any other analyzers.
basically they can return diagnostic that is not from descriptor they return.
so, some of these analyzer used to not run on OOP when this return true. will all these run on OOP now? or never. I think code style analyzer always return fake hidden descriptor? (since it doesn't matter for them)
|
|
||
| public bool OpenFileOnly(Workspace workspace) | ||
| { | ||
| var preferTypeKeywordInDeclarationOption = workspace.Options.GetOption( |
There was a problem hiding this comment.
all these analyzers are special. can't I think their descriptor has no meaning.
|
|
||
| public bool OpenFileOnly(Workspace workspace) | ||
| { | ||
| var preferTypeKeywordInDeclarationOption = workspace.Options.GetOption( |
There was a problem hiding this comment.
same here. not sure what the reason was that IDE refuse to use regular diagnostic system but use this system. @mavasani do you remember? was it due to naming convention analyzer? which they say they don't know what diagnostic they have either since user can configure those?
| var existingData = await ProjectAnalysisData.CreateAsync(project, stateSets, avoidLoadingData, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // we can't return here if we have open file only analyzers sine saved data for open file only analyzer | ||
| // is wrong. (since it only contains info on open files rather than whole project) |
There was a problem hiding this comment.
you should do something here I believe. otherwise, fix all on open file only analyzer will be broken. not sure whether there is a test cover this or not though.
There was a problem hiding this comment.
I see why you removed it here now.
| { | ||
| if (existing.TryGetValue(analyzer, out var analysisResult) && | ||
| analysisResult.Version == version && | ||
| !analyzer.IsOpenFileOnly(project.Solution.Workspace)) |
There was a problem hiding this comment.
hmmm.. we might filter out all hidden analyzer for closed file one, so this basically assume it will never be here I guess.
There was a problem hiding this comment.
we might filter out all hidden analyzer for closed file one
No, hidden diagnostics work for Fix All even if the documents are closed.
| } | ||
|
|
||
| // due to OpenFileOnly analyzer, we need to run inproc as well for such analyzers | ||
| var inProcResultTask = AnalyzeInProcAsync(CreateAnalyzerDriver(analyzerDriver, a => a.IsOpenFileOnly(project.Solution.Workspace)), project, remoteHostClient, cancellationToken); |
There was a problem hiding this comment.
I doubt this is okay due to style analyzers. you want this, probably need to fix code style analyzers to behave same as regular analyzers first.
|
what is driving force of this PR? having issue with OpenFiles or just clean up? I think if It wants to remove OpenFiles, it should first fix the reason it was added at the first place. it should first make all IDE analyzers to behave same as third party analyzers that don't require information that third party analyzer can't access. (which we wanted but didn't have time to do so) if I am remembering correctly. first is options, (which was one of reason we started editorconfig in compiler) |
This is broken before and after the change in this PR (i.e. this PR doesn't change behavior for this).
We already run these analyzers OOP, so I'm assuming they work in this scenario. I can do some more manual validation if it would help.
The only two analyzers potentially impacted are unnecessary imports and the rename tracking analyzers. This PR forces both of these to run in process so they will not break. |
What IDE services does this need? N/M. I see now. It accesses the workspace to find all linked documents to decide what is ok to remove or not. |
I don't know of any, but I haven't conclusively ruled it out either. |
|
Closing this for now; will file an issue to revisit. |
This internal method overlaps the behavior of
DiagnosticSeverity.Hidden. The the few analyzers relying on it for correctness already reported Hidden diagnostics by default. These analyzers are updated to includeNotConfigurable, ensuring that Hidden severity will be used for the diagnostics in all configurations.Fixes #15760
Fixes #16026
Closes #16700
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.