Give a specific message when analyzer references a more-recent compiler version

[RikkiGibson]

Closes #61252

https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1569042

[RikkiGibson]

we're skipping error codes which we know are present in 17.4.

[RikkiGibson]

The idea here was to prevent breaking the signature of the constructor, since it is public API. I guess we could use 'internal init' here as well.

Instead of adding this property, we could consider "smuggling through" a string representation of the referenced version through the 'message' property. But that is very hacky.

Ideally the public API would be much more limited here, i.e. the constructor would be internal and perhaps the entire type as well. It's not clear to me why external users would need to dig through why analyzers are failing to load instead of just surfacing the diagnostics.

[RikkiGibson]

@mavasani do you know whether/how the IDE is directly using new AnalyzerLoadFailureEventArgs and related APIs such as AnalyzerLoadFailureEventArgs.FailureErrorCode?

[jaredpar]

The one point I would raise is whether the increase in complexity in the API is justified by how much clearer it makes the error. Basically is the error message without this good enough? Does knowing the exact version referenced by the analyzer help the customer enough to make this API worth it? Lacking this data the customer would still have the version of the compiler and the version of the analyzer (usually it's the full path to the NuPkg).

Leaned both ways thinking about this in last 5 minutes.

[RikkiGibson]

Lacking this data the customer would still have the version of the compiler and the version of the analyzer

The user would have the running version of the compiler, to be clear. They could be referencing an analyzer version which just happens to require a later SDK than the one they're running, and the process of figuring out manually which compiler version the analyzer is referencing may not be something we expect users to know how to do.

I'm happy to look at any way to pass along this information in a way that's found to be more palatable. But I do think the information is worth passing along.

[mavasani]

@mavasani do you know whether/how the IDE is directly using new AnalyzerLoadFailureEventArgs and related APIs such as AnalyzerLoadFailureEventArgs.FailureErrorCode?

@RikkiGibson AnalyzerLoadFailureEventArgs is required to be exposed for all analyzer hosts that might require loading the analyzer assembly in contexts outside analyzer execution. For example, in the IDE, we need to load the analyzer assembly to populate the Analyzers node in the solution explorer, and any failure to load the analyzer assembly needs to be reported as a diagnostic in the error list. We do so by hooking up to the event handler AnalyzerFileReference.AnalyzerLoadFailed:

roslyn/src/VisualStudio/Core/Def/ProjectSystem/VisualStudioAnalyzer.cs

Lines 52 to 53 in ec6da66

	var analyzerFileReference = new AnalyzerFileReference(FullPath, s_analyzerAssemblyLoader);
	analyzerFileReference.AnalyzerLoadFailed += OnAnalyzerLoadError;

In the hooked up event handler, we create the appropriate diagnostic using the failure code and report it in the error list:

roslyn/src/VisualStudio/Core/Def/ProjectSystem/VisualStudioAnalyzer.cs

Lines 61 to 70 in ec6da66

	private void OnAnalyzerLoadError(object sender, AnalyzerLoadFailureEventArgs e)
	{
	var data = DocumentAnalysisExecutor.CreateAnalyzerLoadFailureDiagnostic(e, FullPath, _projectId, _language);
	lock (_gate)
	{
	_analyzerLoadErrors = _analyzerLoadErrors.Add(data);
	_hostDiagnosticUpdateSource.UpdateDiagnosticsForProject(_projectId, this, _analyzerLoadErrors);
	}
	}

[RikkiGibson]

I also wanted to do an end-to-end test for VB, but I wasn't able to figure out how to provide a ref to Microsoft.VisualBasic.dll and System.dll in this context. Tried a few ways of making a mock compiler.

[jaredpar]

Why use 42.42.42.42 here? This will sometimes match the version in the MS.CA running in this process and sometimes not match it. Feels like that could lead to flakiness down the road (running unit tests on official binaries for example). My instinct says a different number should be here. Am I missing something about what this helps with?

[RikkiGibson]

We can use a small number here. The only point of this is to be a baseline, so in the next scenario we can pinpoint the high version number as the reason it won't load. I did find that helpful when initially setting all this up though it perhaps doesn't look as helpful now that things are working :)

[RikkiGibson]

Looking at the failing netframework tests now.

[RikkiGibson]

@jaredpar @chsienki This is ready for another review pass.