Use directory-scoped ALCs to load analyzers in .NET Core by RikkiGibson · Pull Request #55098 · dotnet/roslyn

RikkiGibson · 2021-07-23T23:26:39Z

Resolves #52177

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

…AssemblyLoader.Core.cs

AraHaan · 2021-07-27T23:18:38Z

So when this gets merged will this by chance make it in .NET 6 in time?

RikkiGibson · 2021-07-27T23:22:37Z

Yes but probably not till RC1.

AraHaan · 2021-07-27T23:31:27Z

Alright, I get the daily RC1 builds so hopefully it lands on those soon.

jaredpar

The loader should only be loading analyzer assemblies that are explicitly passed to the compiler.

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

src/Compilers/Test/Resources/Core/TestResources.cs

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerFileReferenceTests.cs

src/Compilers/Test/Core/AssemblyLoadTestFixture.cs

src/Compilers/Core/CodeAnalysisTest/AssemblyUtilitiesTests.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

src/VisualStudio/Core/Test.Next/Remote/SnapshotSerializationTests.cs

src/Compilers/Test/Core/AssemblyLoadTestFixture.cs

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

jaredpar · 2021-08-18T23:54:53Z

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

+            analyzer.GetType().GetMethod("Method")!.Invoke(analyzer, new object[] { sb });
+            Assert.Equal(ExecutionConditionUtil.IsCoreClr ? "1" : "42", sb.ToString());
+        }
+


One test I think is missing is firing up two different DefaultAnalyzerAssemblyLoader and verifying they can load different assemblies that have the same simple name. That is effectively what the compiler server is going to be doing.

Does this comment describe a different scenario than what is covered in AssemblyLoading_MultipleVersions (where 'Delta1' and 'Delta2' have the same simple name)?

Yes. In that test you're creating one DefaultAnalyzerAssemblyLoader and loading two different versions of Delta into it and verifying that one fails. I'm saying create two DefaultAnalyzerAssemblyLoader instances and load Delta1 into the first and Delta2 into the second. That is effectively how long lived hosts on .NET Core will work (new loader per compilation) and want to verify that it's working and that the correct Assembly is loaded into each context

Actually one other test you need is loading Delta1 and Delta2 into the same DefaultAnalyzerAssemblyLoader but from different directories. That should work just fine now. That is essentially our core analyzers with conflicting dependencies case.

We have AssemblyLoading_MultipleVersions to test loading different versions of the same assembly into one loader. I added a test AssemblyLoading_MultipleVersions_MultipleLoaders to do the same but with each version in a different loader.

…AssemblyLoader.Core.cs Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

jaredpar

Minor feedback.

jaredpar · 2021-08-20T16:18:38Z

src/Compilers/Core/CodeAnalysisTest/DefaultAnalyzerAssemblyLoaderTests.cs

+            analyzer.GetType().GetMethod("Method")!.Invoke(analyzer, new object[] { sb });
+            Assert.Equal(ExecutionConditionUtil.IsCoreClr ? "1" : "42", sb.ToString());
+        }
+


Yes. In that test you're creating one DefaultAnalyzerAssemblyLoader and loading two different versions of Delta into it and verifying that one fails. I'm saying create two DefaultAnalyzerAssemblyLoader instances and load Delta1 into the first and Delta2 into the second. That is effectively how long lived hosts on .NET Core will work (new loader per compilation) and want to verify that it's working and that the correct Assembly is loaded into each context

jaredpar · 2021-08-20T16:21:11Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs

+            return loadContext.LoadFromAssemblyName(name);
+        }
+
+        internal static class TestAccessor


Why did you opt to have a TestAccessor here vs. just adding an internal method on DefaultAnalyzerAssemblyLoader?

I wanted people to feel bad about ever accessing this in the implementation. I was attempting to imitate TestAccessor patterns in ChangedText.cs, SegmentedArray`1.cs, etc.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs

chsienki · 2021-08-23T17:32:23Z

src/Compilers/Test/Core/AssemblyLoadTestFixture.cs

+
+namespace Microsoft.CodeAnalysis.Test.Utilities
+{
+    public sealed class AssemblyLoadTestFixture : IDisposable


This is a really nice change!

chsienki

Couple nits, but otherwise looks great!

AraHaan · 2021-08-23T22:45:21Z

So basically this did not make it in time for the .NET 6 rc.1 daily builds?

jaredpar · 2021-08-24T03:52:38Z

So basically this did not make it in time for the .NET 6 rc.1 daily builds?

This will make 17.0 Preview 4 which should be in the .NET 6 SDK RC1 builds.

Use directory-scoped ALCs to load analyzers in .NET Core

60a8c98

RikkiGibson requested a review from a team as a code owner July 23, 2021 23:26

ghost added the Area-Compilers label Jul 23, 2021

RikkiGibson commented Jul 24, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs Outdated Show resolved Hide resolved

Update src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzer…

4f925b4

…AssemblyLoader.Core.cs

RikkiGibson mentioned this pull request Jul 27, 2021

Analyzer host process fails to load analyzers correctly when building in parallel #52177

Closed

chsienki approved these changes Jul 27, 2021

View reviewed changes

jaredpar requested changes Aug 1, 2021

View reviewed changes

jaredpar reviewed Aug 1, 2021

View reviewed changes

src/Compilers/Test/Resources/Core/TestResources.cs Outdated Show resolved Hide resolved

RikkiGibson and others added 4 commits August 2, 2021 17:06

Compile assembly load test resources during test run

148e522

Add AnalyzerAssemblyLoader.ShouldLoad

b2d13e2

Use new test harness

5f89f8b

Apply suggestions from code review

e8c807f

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

RikkiGibson commented Aug 4, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs Show resolved Hide resolved

jaredpar requested changes Aug 4, 2021

View reviewed changes

RikkiGibson and others added 2 commits August 5, 2021 11:27

Update src/Compilers/Test/Core/AssemblyLoadTestFixture.cs

6cbde0f

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

Fix shadow copy loader

b52c0f2

RikkiGibson requested a review from a team as a code owner August 5, 2021 18:27

RikkiGibson added 3 commits August 5, 2021 12:02

Fix formatting

b383164

Perform shadow copy on load

405a5e9

_testResources -> _testFixture

7e83c50

genlu mentioned this pull request Aug 9, 2021

ArgumentException when running diagnostic service on .net core #46687

Closed

AraHaan reviewed Aug 9, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs Outdated Show resolved Hide resolved

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs Outdated Show resolved Hide resolved

genlu reviewed Aug 10, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzerAssemblyLoader.Core.cs Show resolved Hide resolved

genlu mentioned this pull request Aug 10, 2021

Try to fix the analyzer path when loading in OOP #55520

Merged

RikkiGibson added 2 commits August 11, 2021 14:44

simplify

26c76e8

simplify

e3e711e

RikkiGibson added 4 commits August 18, 2021 11:53

Merge branch 'main' of github.com:dotnet/roslyn into alc

3c43d03

Fix tests

dcb4e84

sort simple names

72cb240

Move AnalyzerWithDependency to fixture

71854bc

jaredpar reviewed Aug 18, 2021

View reviewed changes

RikkiGibson and others added 7 commits August 18, 2021 16:57

Don't worry about warnings

f93a6aa

Update src/Compilers/Core/Portable/DiagnosticAnalyzer/DefaultAnalyzer…

7c87d23

…AssemblyLoader.Core.cs Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

more

50490c0

Fix test

703a261

Check ALCs content in test

365ff28

Check more accurate assembly data

02ca0d7

Only test native dependency in windows coreclr

1dadcc3

jaredpar approved these changes Aug 20, 2021

View reviewed changes

Add suggested test

254bf2e

chsienki reviewed Aug 23, 2021

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerAssemblyLoader.cs Outdated Show resolved Hide resolved

chsienki reviewed Aug 23, 2021

View reviewed changes

chsienki approved these changes Aug 23, 2021

View reviewed changes

Doc comment feedback

c92e149

RikkiGibson enabled auto-merge (squash) August 23, 2021 18:36

RikkiGibson merged commit 99f45b7 into dotnet:main Aug 23, 2021

ghost added this to the Next milestone Aug 23, 2021

RikkiGibson deleted the alc branch August 23, 2021 19:04

CyrusNajmabadi pushed a commit to CyrusNajmabadi/roslyn that referenced this pull request Aug 24, 2021

Use directory-scoped ALCs to load analyzers in .NET Core (dotnet#55098)

153244f

dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021

JoeRobich mentioned this pull request Sep 11, 2021

Update the vnext branch OmniSharp/omnisharp-roslyn#2223

Merged

PathogenDavid mentioned this pull request Sep 22, 2021

RC1 regression: dependencies no longer work correctly in source generators across <ProjectReference> #56442

Closed

andrei-epure-sonarsource mentioned this pull request Nov 3, 2021

Calling the compiler with analyzers which reference different versions of the same assembly results in FileLoadException #41421

Open

Conversation

RikkiGibson commented Jul 23, 2021

Uh oh!

Uh oh!

AraHaan commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson commented Jul 27, 2021

Uh oh!

AraHaan commented Jul 27, 2021

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaredpar Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

jaredpar Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

jaredpar left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

jaredpar Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chsienki Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

chsienki left a comment

Choose a reason for hiding this comment

Uh oh!

AraHaan commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar commented Aug 24, 2021

Uh oh!

Reviewers

AraHaan commented Jul 27, 2021 •

edited

Loading

RikkiGibson Aug 19, 2021 •

edited

Loading

RikkiGibson Aug 20, 2021 •

edited

Loading

AraHaan commented Aug 23, 2021 •

edited

Loading