added a way to specify ordering of Document/ProjectDiagnosticAnalyzers #24673
Conversation
this is to let partner team such as typescript and F# to be able to order their analyzers
|
@jinujoseph @Pilchie approval for 15.7? |
|
tagging @minestarks as well. |
| /// Priority is ascending order and this only works on HostDiagnosticAnalyzer meaning Vsix installed analyzers in VS. | ||
| /// This is to support partner teams (such as typescript and F#) who want to order their analyzer's execution order. | ||
| /// </summary> | ||
| public virtual int Priority { get; } = DefaultPriority; |
There was a problem hiding this comment.
💡 The second backing field here isn't needed. You can use => instead of { get; } =.
|
ping? can I get a code review? |
| return Math.Max(0, analyzer.Priority); | ||
| default: | ||
| // regular analyzer get next priority | ||
| return -1; |
There was a problem hiding this comment.
❓ This means all user analyzers will have priority over all DocumentDiagnosticAnalyzer and ProjectDiagnosticAnalyzer instances. Is this the intent?
There was a problem hiding this comment.
Document/ProjectDiagnosticAnalyzer will come after all other kinds of analyzers. Document/ProjectDiagnosticAnalyzer is special kind of analyzers that want to use Workspace not compilation for diagnostics. I dont see any issue it being last. and no third party can use it.
| return Owner.GetOnAnalyzerException(projectId, DiagnosticLogAggregator); | ||
| } | ||
|
|
||
| internal IEnumerable<DiagnosticAnalyzer> GetAnalyzersTestOnly(Project project) |
There was a problem hiding this comment.
💡 It would be good to create an enforced [TestOnly] attribute. Outside the scope of this pull request, but something to think about if this is a common pattern.
There was a problem hiding this comment.
sure. we could create enforcer once we have call graph I believe with attribute.
| _compilerAnalyzer = analyzerManager.GetCompilerDiagnosticAnalyzer(language); | ||
| _map = analyzerMap; | ||
|
|
||
| var compilerAnalyzer = analyzerManager.GetCompilerDiagnosticAnalyzer(language); |
There was a problem hiding this comment.
I really don't like that we are tracking four different things now for analyzer prioritization.
- The compiler analyzers
- Implementations of
DocumentDiagnosticAnalyzer - Implementations of
ProjectDiagnosticAnalyzer - Everything else
Each of these four uses a different prioritization mechanism, and none of them leverage the functionality which users come to expect with MEF/VS (OrderAttribute / ExtensionOrderAttribute).
❓ What are our alternatives to unify and simplify this behavior?
There was a problem hiding this comment.
DiagnosticAnalyzer doesn't use MEF. and I dont like OrderAttribute which require one to explicitly specify what comes before and after. that is just overkill for what is asked and get pretty complex fast. we used them and probably still use in some cases such as command handler, but we just hated them I believe. since whenever new one is added, one had to change multiple ones to adjust order. it is only suitable for ones that order must be declared quite clearly. this is not the case.
...
we have 4 different groups of analyzers (C#/VB compiler analyzer which is special, regular diagnostic analyzer, Document/ProjectDiagnosticAnalyzer, analyzer that implements IBuiltInAnalyzer). Engine treats them differently. reason is simple. since we want to allow special ability third party analyzer (regular diagnostic analyzer) can't use.
sure, we can have an issue tracking whether we should expose this special ability to third party or not. but that is not something this PR is about.
This is not a reason¹ to avoid allowing users to prioritize analyzers. Historically, the members working on dotnet/roslyn are just as likely, if not more so, to implement analyzers that are slower than third-party analyzers. ¹ To clarify, this is not even a weak reason. If anything, this serves as a reason to allow third-party analyzers to declare priorities because it explicitly acknowledges the user benefits of running analyzers in specified orders. 💡 If you ever find yourself thinking "people maybe shouldn't do this", it's almost certainly the case that 'people' includes our own team. 😄 |
|
letting users set priority is I believe what we want. not authors. we currently disallow author from setting it. .. yep, we should allow users to set priority. BUT NEVER let author to set it. (sure, except first party including F# and typescript. especially F# and typescript since they don't have concept of third party analyzers) I don't agree we should be treated exactly same as third party. if we don't have requirement to maintain API backward compatibility/perf or quality, then sure. we can innovate/experiment and breaks people as we go along. but since we need to maintain all those, I don't see anyway we expose everything we do to third party such as supporting special treatment on compiler diagnostic analyzers which get participate in build/live error duplication, Document/ProjectDiagnosticAnalyzer that use Solution directly rather than compilation. we do have something we try to expose to third-party such as Options (currently only first party can use options) through editorconfig. there are some we want to expose such as IBuiltInAnalyzer.GetAnalyzerCategory which can improve perf on demand analysis on span. also there are something we want to get rid of "OpenFlieOnly". all these, I dont think we can expose them as we introduce before we internally playing with them. once we have such thing, engine must treat first party special. |
|
@sharwell not sure which you want to change. |
|
ping? @sharwell and @Pilchie @jinujoseph ? |
I did mean I believe authors should be able to control it. |
There was a problem hiding this comment.
Accepting as an interim solution while we work on blame and overall driver performance. With additional work going into the analyzer driver for performance, expect two things to occur in the future:
- The current API to change
- The eventual API to expose the final option uniformly, allowing for reordering by analyzer authors and/or users without limitations to origin
|
I am against letting author to set priority so I guess that is design question? tagging @jinujoseph and @dotnet/roslyn-analysis @dotnet/roslyn-ide |
| if (state == _compilerStateSet) | ||
| { | ||
| return _compilerStateSet; | ||
| return -2; |
There was a problem hiding this comment.
Can we extract these into constant fields?
|
I am against exposing any analyzer priority based public API or mechanism, until there is strong request from community (analyzer authors or users) with a valid reason how this will help them.
I think ordering comes into picture only when we start putting expensive/slow analyzers into the pool of analyzers getting executed. Instead of enabling users/authors to push these analyzers to the end of the queue, we should instead focus on a better blame story so end users can disable expensive analyzers and help analyzer authors improve their analyzers. |
|
retest windows_debug_unit64_prtest please |
|
@Pilchie can you take a look? |
|
I approve of the scenario, but I'd like confirmation from at least one of F#/TS that this is sufficient to alleviate their issues. @dsyme or @minestarks - how can we work to validate that this will be enough for your use cases? |
|
Also tagging @brettfo - maybe we could create a VAL build containing this change and a corresponding change to F# to supply priorities and let people try that out? |
|
cc @TIHan @Pilchie I looked at the code and it appears to be exactly what Visual F# Tools need. It would be good to do a validation via an evaluation build though. To do that we'd need to
|
|
@Pilchie I can't recall whether we had a specific demand around this. We did have broad discussions about priority in the past, so I checked to see whether this change would help. In our case, we have a mechanism to prioritize by document type (library files vs. user files) rather than analyzer type. I don't think this change directly allows us to control that, but I can see us maybe splitting our current So yes, potentially helpful, but I don't think we'll be consuming this immediately. |
|
@heejaechang Ugh, I'm having trouble building the dev15.7.x branch.
|
|
/cc @jaredpar for error ⬆️ |
|
In a normal directory I get this: I get this in the roslyn directory: |
|
Trying a |
|
Same problem |
|
@dsyme The Roslyn repository is locked to a specific version of the dotnet tool and the .NET SDK. The former is automatically installed by the Restore script in the repository, but the latter requires manual installation. If the correct version of the SDK isn't already located on the system (e.g. as part of a prior Visual Studio install that uses the same version), you'll have to follow the link provided in the error to install the specific version. The error will then be resolved until the next time we need to upgrade our SDK reference. |
|
OK, but it downloaded and installed the dotnet SDK pre-release version during restore. But then fails to detect it during build. Also the offered link http://go.microsoft.com/fwlink/?LinkID=798306&clcid=0x409 doesn't give me an option to manually install the specific pre-release 2.2.0 version requested The log for the Restore & build after a git clean is below: |
|
@dsyme A direct link is given in the red text from your error screenshot: |
|
AH thanks, didn't see that one |
|
Quick question: which Roslyn.sln startup project should I use to F5 debug into a new RoslynDev-hive (in order to exercise the new code above)? |
VisualStudioSetup.Next |
|
@sharwell To pick up the new Roslyn binaries do I have to HintPath to binary-by-binary from the VIsualFSharp.sln? If so, which specific binaries are affected by the above fix? |
Quick question: how do I confirm if this is running in a RoslynDev registry hive? It seems to know all my settings history when I launch which surprises me (e.g. different to when I launch into using "Roslyn" project as the startup project) |
I'm not sure on this one (new area for me).
If you used F5 (or Ctrl+F5), it's almost certainly in the RoslynDev hive. You can use Process Explorer to see the command line arguments passed to the process if you want to conficb. |
|
Looks like only Microsoft.CodeAnalysis.Features.dll |
You can also verify the location of the loaded Microsoft.CodeAnalysis assemblies using the debugger modules window. |
|
@heejaechang Just to check - Priority 1 = go first, Priority 1000 = go last? The docs just say "in ascending order" which I'm still not clear about |
Yes, priority 1 goes first for the configurable analyzers. The default value is 50. |
|
OK, I confirm this works! See the notes on the corresponding Visual F# pull request dotnet/fsharp#4341 |
|
Thanks for the confirmation @dsyme. @heejaechang Approved to merge for 15.7. |
|
That's really odd. From the command line Restore and Build should always work. We bootstrap everything. Indeed it is going through the bootstrap phase for CLI here but the latter msbuild call is not picking up the installation. That's pretty straight forward as all we need to do is manipulate @dsyme is this a simple cmd window (not a developer command prompt)? Can you run |
|
VS roams setting between hives as well. if you don't want that, you need to turn this off
|
8 participants
Customer scenario
Users that use F# or Typescript should see errors that are considered cheaper to calculate much faster than before.
Bugs this fixes
#20390 (for F#)
Workarounds, if any
each have put their own workaround for it which they want to remove.
Risk
I don't see any risk
Performance impact
time to run all analyzers are same. just order of they run can be changed. but for users, since they will see the first error faster than before if there is one, perception-wise, it could feel faster.
Is this a regression from a previous update?
No
Root cause analysis
Roslyn analyzer engine doesn't allow diagnostic author to specific order of analyzers since it is open platform (third party analyzers). otherwise, there can be an abuser that think theirs are most important and put theirs at the head. so we don't give this control to author. for built in analyzers, we do have control over the order. for example, we run compiler analyzers first so not a problem.
Issue occurred once partner team start to adapt Roslyn for diagnostics (typescript, F#). they want to control order of their builtin analyzers (like running their compiler analyzers run first), but to us, they are still not first party, so they don't have such control. this expose a way for them to control their analyzers order. but for now, this is specific to vsix installed analyzers and only for partner team (Document/ProjectDiagnosticAnalyzers). we still don't give it to pure third party.
How was the bug found?
Dogfooding