Compiler can't load an analyzer where a binding redirect may be needed #53672
Comments
dotnet-issue-labeler
bot
added
the
untriaged
|
Another variant of the problem: Fails with:
For the System.Text.Json failure, it only repros when I have the .NETCoreApp3.1 runtime installed. Even though I'm using the 5.0 SDK, the compiler runs in a .NET Core 3.1 runtime process when it's available, and that's where it fails. Probably because assemblies that ship with the runtime won't be loaded from the app directory (or from the list of analyzer assemblies in my case) unless the .deps.json file tells the runtime to do so. But I need the .NET Core 3.1 runtime for this repo/pipeline because I run tests on it. |
AArnott
added a commit
to microsoft/CsWin32
that referenced
this issue
Jun 12, 2021
This version ships with .NET Core 3.1 and therefore resolves one possible bug that shows up in [this condition][1]. [1]: dotnet/roslyn#53672 (comment)
That is unexpected. Are you using the stock compiler that comes with the SDK or are you overriding it via say Microsoft.Net.Compilers? Don't believe you're doing the latter but that behavior is unexpected and it's one explanation for why you may be seeing it. |
|
Ah yes, in fact I was referencing the |
|
Assigning this to myself since we're going to address this in an IDE-only change. |
jasonmalinowski
removed
the
untriaged
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Sep 16, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly; by requiring exact versions we'd end up in diamond dependency cases where you couldn't practically produce an analyzer package that would cleanly load. Right now we are targeting this fix for the IDE only since it still runs on the .NET Framework; for the command line compiler that should be running on .NET Core where we are using AssemblyLoadContexts. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Sep 16, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly; by requiring exact versions we'd end up in diamond dependency cases where you couldn't practically produce an analyzer package that would cleanly load. Right now we are targeting this fix for the IDE only since it still runs on the .NET Framework; for the command line compiler that should be running on .NET Core where we are using AssemblyLoadContexts. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Sep 17, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly; by requiring exact versions we'd end up in diamond dependency cases where you couldn't practically produce an analyzer package that would cleanly load. Right now we are targeting this fix for the IDE only since it still runs on the .NET Framework; for the command line compiler that should be running on .NET Core where we are using AssemblyLoadContexts. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Oct 29, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly; by requiring exact versions we'd end up in diamond dependency cases where you couldn't practically produce an analyzer package that would cleanly load. Right now we are targeting this fix for the IDE only since it still runs on the .NET Framework; for the command line compiler that should be running on .NET Core where we are using AssemblyLoadContexts. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 1, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly; by requiring exact versions we'd end up in diamond dependency cases where you couldn't practically produce an analyzer package that would cleanly load. Right now we are targeting this fix for the IDE only since it still runs on the .NET Framework; for the command line compiler that should be running on .NET Core where we are using AssemblyLoadContexts. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 2, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 2) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with A.dll's dependency, but if analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 2, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 2) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with A.dll's dependency, but if analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 2, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 2) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with A.dll's dependency, but if analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 2, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 2) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with A.dll's dependency, but if analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 3, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on and ships B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 1) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and the A that B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with Ananlyer 2's dependency, but if Analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 23, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on and ships B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 1) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and the A that B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with Ananlyer 2's dependency, but if Analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 23, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on and ships B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 1) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and the A that B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with Ananlyer 2's dependency, but if Analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
jasonmalinowski
added a commit
to jasonmalinowski/roslyn
that referenced
this issue
Nov 23, 2021
When we're loading analyzer dependencies, our current logic was to require the exact version that was requested from the loading assembly. This is problematic in a lot of cases where you would normally have a binding redirect to ensure assemblies load cleanly in diamond dependency scenarios. Without this, if you depend on B.dll and A.dll, but B.dll depends on an older version of A.dll, you can't produce a package that actually loads because there's no sane way to package both A.dlls at the same time. We avoid adding any logic here where we'll still try to use an older version if that's still available. Consider a more complicated example of: 1. Analyzer 1 depends on and ships B.dll (version 2) and A.dll (version 2); B.dll (version 2) depends on A.dll (version 1). 2. Analyzer 2 depends on A.dll (version 1) and ships that. When loading Analyzer 1, we need a redirect to ensure the A that it consumes and the A that B consumes are unified. We don't want the presence of Analyzer 2 to change that. We could satisfy B.dll's request with Ananlyer 2's dependency, but if Analyzer 1 expects type unification then adding Analyzer 2 to the build would break it. Fixes dotnet#53672
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In microsoft/CsWin32#218, we had a report of a source generator that was failing to load in Omnisharp. Debugging revealed it the analyzer depends on System.Text.Json, and the NuGet package there depends on assembly version 4.1.3 of System.Numerics.Vectors. However, another dependency brings in 4.1.4 of System.Numerics.Vectors, which in a normal app would be resolved with a binding redirect at runtime. Since microsoft/CsWin32#261 was fixed, the analyzer does package 4.1.4.
For our command line compilers, right now it works out that this analyzer will load, only because System.Numerics.Vectors 4.1.4 happens to be a dependency of the compiler and thus a direct is in place for it. But that's somewhat an implementation detail and could change. For Omnisharp however, it doesn't have a redirect and thus won't load. And there's no practical way to package both 4.1.3 and 4.1.4 versions of the dependency in the same NuGet package to work around the issues.
Discussing with @jaredpar it seems the best resolution here is to change our assembly loading rules a bit. When we load an analyzer, we require exact matches of dependencies here; changing that to allow an "or higher" would mean we'd be effectively doing a binding redirect at runtime, which would resolve these issues.
Omnisharp currently has a copy/paste of the analyzer assembly loading code in it's repo; once we have the compiler change in hand we should update that copy. Since we have an External Access shim, I'd propose we simply make an API for them to fetch the implementation Roslyn has and they can reuse that directly.
The text was updated successfully, but these errors were encountered: