Fix probing order in AssemblyResolver

[ericstj]

We should prefer the task directory over the AppDomain base, since
AppDomain will point to MSBuild's path.

Undo portion of 1a79896 that removed
System.Collections.Immutable. The current fix will work in both MSBuild
versions which include Immutable as well as do not.

Replaces previous fix for https://github.com/dotnet/corefx/issues/31925.
There are two potential causes for the issue:

1. Bad ordering of probing in our AssemblyResolve event.
2. A successful bind that we expected to fail.

The first issue is simply a bug. We were probing the AppDomain base directory before looking next to the task. MSBuild tasks run in a shared appdomain and this points to the MSBuild bin directory. As a result we would start unifying to the MSBuild version for mismatches, but load our local copy for exact matches. The result was code we compiled didn’t unify with our dependencies (since those used older versions).

The second issue is more insidious. Our tasks are built under the assumption that we’ll unify all the code to the assembly version the task carries with it. We can guarantee this for code we compile since we’ll include the exact version of the dependency next to the task. We’ll either load that version, or an equivalent version that’s already been loaded or present in the GAC. We cannot garuntee this for code we aren’t compiling, and we bank on the fact that nothing in the normal probing will find that old assembly. If it does, we’ll be back in this place. An added wrinkle to this is that if one of the assemblies we depend on ourselves was already loaded by someone else in the same appdomain and during that load msbuild or the task that loaded it unified its dependency to a different version we can’t change that and we’re stuck with the different version, and thus it won’t unify to what’s already been loaded.

Now the cases folks just started hitting are all 1. The fix will be to simply fix that bug. We need to do so carefully so that we don’t trigger issue 2.

Issue 2 is always present with MSBuild tasks (consider the GAC), it just became a bit more prevalent now that MSBuild put one of our dependencies inbox. We just need to make sure either 1) we either compile against the exact same version that MSBuild has so that even if someone pre-loads it we’re fine since we’ll unify to that anyway, 2) None of our dependencies reference that version, so we’ll never pre-load it.

Here's the set of versions that we know MSBuild included:

VS version	Immutable version
15.7	1.2.1.0
15.8	1.2.3.0

Here’s the current set we’re using in master of buildtools:

File	Immutable version
net46\Microsoft.DiaSymReader.Converter.dll	1.2.3.0
net46\Microsoft.DotNet.Build.Tasks.dll	1.2.3.0
net46\System.Reflection.Metadata.dll	1.2.3.0
net46\analyzers\Desktop.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.AnalyzerPowerPack.Common.dll	1.1.36.0
net46\analyzers\Microsoft.AnalyzerPowerPack.CSharp.dll	1.1.36.0
net46\analyzers\Microsoft.CodeAnalysis.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.CodeAnalysis.CSharp.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.DotNet.CodeAnalysis.dll	1.2.0
net46\analyzers\System.Runtime.Analyzers.dll	1.1.36.0
net46\analyzers\System.Runtime.CSharp.Analyzers.dll	1.1.36.0
net46\analyzers\System.Runtime.InteropServices.Analyzers.dll	1.1.36.0
net46\analyzers\System.Security.Cryptography.Hashing.Algorithms.Analyzers.dll	1.1.36.0

So currently we are “safe” because the only two overlapping versions are in assemblies we control and they match exactly. We can either keep them at 1.2.3.0, or update them to a new version and remain safe. We’ll have to be careful when taking updates to dependencies that they do not bring in dependencies on 1.2.1.0 (or 1.2.3.0 if we happen to move to a newer version).

This actually raises the issue that MSBuild should be careful about changes it makes to the binaries in the probing paths as those change could easily break existing tasks.

[weshaggard]

Thanks for the investigation and fix. Do you think it is useful to port to 2.1/2.2 branch as well?

[ericstj]

You can port if you want, but I don't think that's so urgent, unless you have folks complaining that they can't use older VS builds for development on release branches.

Here's a look at what is in 1.1 and 1.2 respectively:

release/1.1, Microsoft.DotNet.BuildTools.2.1.0-rc1-03131-06

File	Immutable version
net46\Microsoft.DiaSymReader.Converter.dll	1.2.1.0
net46\Microsoft.DotNet.Build.Tasks.dll	1.2.1.0
net46\System.Reflection.Metadata.dll	1.2.0.0
net46\analyzers\Desktop.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.AnalyzerPowerPack.Common.dll	1.1.36.0
net46\analyzers\Microsoft.AnalyzerPowerPack.CSharp.dll	1.1.36.0
net46\analyzers\Microsoft.CodeAnalysis.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.CodeAnalysis.CSharp.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.DotNet.CodeAnalysis.dll	1.2.0.0
net46\analyzers\System.Runtime.Analyzers.dll	1.1.36.0
net46\analyzers\System.Runtime.CSharp.Analyzers.dll	1.1.36.0
net46\analyzers\System.Runtime.InteropServices.Analyzers.dll	1.1.36.0
net46\analyzers\System.Security.Cryptography.Hashing.Algorithms.Analyzers.dll	1.1.36.0

release/2.2, Microsoft.DotNet.BuildTools.2.2.0-preview1-03131-04

File	Immutable version
net46\Microsoft.DiaSymReader.Converter.dll	1.2.1.0
net46\Microsoft.DotNet.Build.Tasks.dll	1.2.1.0
net46\System.Reflection.Metadata.dll	1.2.0.0
net46\analyzers\Desktop.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.AnalyzerPowerPack.Common.dll	1.1.36.0
net46\analyzers\Microsoft.AnalyzerPowerPack.CSharp.dll	1.1.36.0
net46\analyzers\Microsoft.CodeAnalysis.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.CodeAnalysis.CSharp.Analyzers.dll	1.1.36.0
net46\analyzers\Microsoft.DotNet.CodeAnalysis.dll	1.2.0.0
net46\analyzers\System.Runtime.Analyzers.dll	1.1.36.0
net46\analyzers\System.Runtime.CSharp.Analyzers.dll	1.1.36.0
net46\analyzers\System.Runtime.InteropServices.Analyzers.dll	1.1.36.0
net46\analyzers\System.Security.Cryptography.Hashing.Algorithms.Analyzers.dll	1.1.36.0

So the change is OK to take, so long as you keep the local copy of S.C.I at 1.2.1.0 (it must either match the version used by MSBuild or all references must be different).

[weshaggard]

I agree lets not worry about fixing this in the servicing branches unless we hit issues there that this will help with.