Roslyn: Need to restart VS for analyzer to work

[twsouthwick]

I find that I need to restart VS to update an analyzer. The steps I take:

1. Use VS 2015 RTM and created a new analyzer.
2. Load it up in a project via nuget and it works as expected.
3. Make a change in the analyzer; compile
4. Update the analyzer via nuget
5. Attempt to use diagnostic

Observed:
The change is not seen until after restarting VS

Expected
The change is seen

I can understand the need to restart (extensions do that as well), but there should be a notification if this is required

[mavasani]

I thought we already showed a warning for this case, recommending that a VS restart is needed. Let me try out the repro on VS2015 RTM.

/cc @tmeschter

[mavasani]

I tried the repro and Taylor is correct. We don't get the "AnalyzerChangedOnDisk" diagnostic for his repro steps. We get that diagnostic only in following cases:

1. After step 4, explicitly delete the downloaded nuget package from the packages dir for the solution and do a nuget package restore OR
2. At step 2, we directly add the analyzer assembly to project as an analyzer reference, not via a nuget package.

Severity    Code    Description Project File    Line
Warning AnalyzerChangedOnDisk   The analyzer assembly 'c:\users\mavasani\documents\visual studio 2015\Projects\Analyzer2\Analyzer2\Analyzer2\bin\Debug\Analyzer2.dll' has changed. Diagnostics may be incorrect until Visual Studio is restarted.   ConsoleApplication2     1

[mavasani]

@heejaechang Is it likely that your project caching bug fix also fixed this, or is it unrelated?

[heejaechang]

@mavasani I am not sure, the fix will let VS know about dll reference updated by nuget, but not sure about analyzer dll being updated.

actually I am not sure whether project system will let VS know about analyzer dll change outside of VS at all currently.

[mavasani]

This still repros with the latest Roslyn bits.

I found the root cause - AnalyzerDependencyChecker which finds conflicting analyzer assemblies and reports the analyzer dependency conflict diagnostic operates on currentAnalyzerPaths. In the scenario here, user just did an upgrade of an existing analyzer. Dependency checker doesn't flag this a conflict as you just have a single analyzer. Additionally, AnalyzerFileWatcherService.ErrorIfAnalyzerAlreadyLoaded doesn't generate the analyzer changed on disk diagnostic as the full path to analyzer assembly changed with nuget package upgrade.

[mavasani]

I see couple more cases where we should detect conflicts in AnalyzerDependencyChecker:

1. Nuget package upgrade (this issue), which causes us to replace an existing analyzer with a newer one, which has a different assembly identity, but still will not be loaded. We need to extend AnalyzerDependencyChecker to include loaded assemblies in the conflict check.
2. Conflicting analyzers with different assembly identity in the same solution: If we have 2 projects in solution referencing different versions of nuget package for the same analyzer, we seem to load analyzer from one of the nuget packages and run the same analyzer on both projects – we should give the conflict warning to indicate that user needs to upgrade all projects in solution to the newer nuget package.

[mavasani]

@tmeschter Based on your offline suggestions, Sri and I agree with your view that we need to perform additional conflict checks related to loaded assemblies at another place - AnalyzerDepedencyChecker is not the right place to do these. We might also need to design how to find additional conflicts, as per your suggestions. However, we are unlikely to get to this in 1.2, so we moved this issue to 1.3.

[NPatch]

Btw, with VS2017 it still repros. Not only that but even removing the analyzer and readding it doens't help.

[sharwell]

This will always occur in the following two cases, which are outside the ability of Roslyn to fix:

1. The analyzer assembly does not have a strong name
2. The analyzer assembly version number does not change between the two packages

For each of these cases, both versions of the analyzer have effectively declared themselves identical to the assembly loader.

[NPatch]

I think I tried to change the AssemblyVersion through the cs file and even then it didn't change anything.
To be honest, if I need to change the AssemblyVersion every time I do a minor fix to the code, it's still as annoying as restarting VS. Perhaps if we could invalidate the Analyzer's state on demand, it would serve. Or if removing and readding the dll would always recache the analyzer.
PS: There seems to be a fix but it's scheduled for later milestones.

[heejaechang]

tagging @jinujoseph and @mavasani

detecting and restarting are 2 different issues. if we move analyzer completely to OOP, even for open files, we can reload analyzers without restarting VS once we can reliable detect when we need to do that.

[NPatch]

I'm sure they are. Just saying that both of them can lead to a lot of time being wasted just to verify your changes.

UPDATE: I remember changing the version to 1.0.0.1 and retrying to remove/readd the assembly. Not sure if the revision part is neglected though in checks(sth that just now occured to me).

[heejaechang]

@NPatch I think right now, what you are doing is probably ones we specifically decided not supported scenario yet. (using analyzer without restart, not reporting part, reporting part is just pure bug)

for analyzer author, we have this idea.
https://github.com/dotnet/roslyn/issues/18093

tagging @jinujoseph @mavasani

[tmeschter]

@NPatch If the analyzer assembly doesn't have a strong name then the version is ignored completely.

This scenario runs up against the limitations of the runtime's assembly loading mechanisms. Notably, there's no way to unload an assembly once it is loaded, and once an assembly is loaded the runtime will not load another assembly with the "same" name (where "sameness" takes into account binding redirects, assembly version, strong name signing, and other factors). So there are no easy solutions to this problem, and few hard ones. :-)

@mavasani @heejaechang If we move all analyzers out-of-process, we can load multiple copies of the "same" assembly (without restarting the process or AppDomain) by loading them into the "no context" loading context (via Assembly.Load(byte[]). However, assemblies loaded this way bypass NGEN entirely, and we would have to re-implement discovery and loading of dependent assemblies, binding redirects, and support for satellite assemblies. We don't use this approach in VS because VS twiddles some runtime flag that effectively means VS will never ask for satellite assemblies for an assembly in the "no context" context, meaning analyzers would never use localized resources. I'm not sure if this would be an issue out-of-process.

[NPatch]

Thanks for all the info. I'll try strong naming it and retry using different versions.
The project reference solution would be a good option to have. At least, you'd have testing that way inside VS. But I haven't looked at the thread yet so don't know if it's difficult or not. Will read though.

[heejaechang]

@tmeschter my thinking was just restarting OOP (OOP already supports it) when analyzer is re-loaded. I think it is fine since it is a rare occasion and simpler since we don't need to implement our own mechanisms for loading and all other binding issues.

[NPatch]

Can you tell me what OOP stands for? I'm guessing it's some component in VS?

[heejaechang]

@NPatch OOP -> Out of process :) sorry.

[NPatch]

No problem. Thanks!

[NPatch]

If I understand @tmeschter 's response now that I reread it, even with strong-naming and different versions, the analyzer won't be reloaded after the first time in a single VS session.
Also correct me if I'm wrong but the Component Cache won't remove it's entry for the Analyzer even if you remove it from the session, so it's easier to fetch if you want to re-add it. And the re-adding will ignore the version(again even when strong named).

Are these correct? Sorry if I'm making you repeat yourselves but I'm not that knowledgeable when it comes to Visual Studio internals.
Btw, I did try strong-naming and changing the versions(both of AssemblyV and AssemblyFileV) and it didn't work, which if I wrote the first part correctly is verified by this behavior.

[tmeschter]

@NPatch If the assembly has a strong name and the version has changed we should be able to load the new version in the same VS session. We will not be able to unload the old version, but I would expect us to stop using it in favor of the new one.

Which "component cache" are you referring to? A MEF cache?

The relevant version is the AssemblyVersion. The AssemblyFileVersion is not relevant for the purposes of assembly loading.

[NPatch]

Yeah, unless that doesn't take part in it. It stands to reason that they get cached somewhere since removing them doesn't remove this issue.
I tried both of them anyway but it didn't help.

[bichuga]

Any updates here? Any tips to avoid having to restart VS when iterating on an analyzer?