Roslyn: .Net 4.6 optimizations breaking on 4.5.2 servers

[NickCraver]

We hit a break when rolling out .Net 4.6 at Stack Overflow which I did a sanity check on here. This is the Array<T> optimization in play:

The optimization itself isn't as important as the class it uses: Array.Empty<T>, which doesn't exist in 4.5.2. This creates an overlapping set of issues of builds and runtime that resulted in a break for us. I first upgraded our canary servers, but not 100% correctly - the .Net 4.5.2 reference assemblies were not present. This resulted in the following layout:

Build server: .Net 4.6 (reference assemblies up through 4.5.1, but _not_ 4.5.2)
Project target: .Net 4.5.2
Runtime server: .Net 4.5.2

So what happened is that the build server did not find .Net 4.5.2 reference assemblies and happily resolved .Net 4.6 assemblies instead. The only hint of this is a build warning, not an error. (Aside: You could easily, IMO, argue that .Net 4.5.1 is a much safer fallback resolution here. Separately, you could argue the commonplace of these build warnings makes them weaker justification in breaks.) Since the compiled-against assembly is what Roslyn checks against in GetWellKnownTypeMember(), this mean that regardless of the targeted framework, a .Net 4.6+ optimization was used _in the IL_.

I think there's another discussion to be had on forward vs. backwards resolution on a target failure (or having the default actually be an error, not a warning), but let's scope this issue more to this specific problem. In discussion on the other issue, @FransBouma made what I think is an excellent point:
Why isn't this a JIT optimization?

It makes much more sense here, at least on the surface, that a more well-informed JIT can make a better decision here given the types actually available at runtime. What was the reasoning for this being in roslyn instead of a runtime optimization? The net result currently is a runtime-only break as a result of this optimization, seen here:

System.MissingMethodException: Method not found: '!!0[] System.Array.Empty()'.
   at Calculon.Web.Scheduled.Tasks.ReportRedisInfoToBosunTask.<Execute>d__5.MoveNext()
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Calculon.Web.Scheduled.Tasks.ReportRedisInfoToBosunTask.Execute(DateTime started)
   at Calculon.Web.Scheduled.ScheduledTasks.<RunScheduledTaskAsync>d__7.MoveNext()

To me, the runtime-only nature of the break makes it quite a bit more serious for people upgrading to .Net 4.6 and (quite literally) changing nothing else. What was the thinking here? Does JIT not make sense for other reasons that aren't so apparent? If this was reasoned through and consciously decided, I'd love to see and read the discussions. I simply can't find them, though I have searched.

[akoeplinger]

This isn't a Roslyn issue. Remember that csc.exe isn't doing resolution of the reference assemblies, it just takes whatever you pass to the "/r" switch.

You'd hit the same problem at runtime if you used any other new class/method in .NET 4.6 in your code but fail to compile against the correct reference assemblies. The same applies to compiling against the 3.5 profile and running on 2.0.

The culprit is MSBuild here, it does the resolution and I personally agree that it should fail instead of warn if the assemblies for the specified target framework can't be found, but as others have mentioned this might have some compatibility risks.

[NickCraver]

@akoeplinger I agree the _resolution_ isn't a roslyn issue, but the _optimization_ is. Doing such an optimization in the compile to IL vs. letting the JIT handle it is the main reason for posting the issue here.

I totally agree the MSBuild resolution itself is a separate (though related) component, I want to talk with my team a bit more before proposing any changes on that side of the fence. Honestly, I'll have to go find which repo is appropriate to file MSBuild issues first (I _assume_ buildtools these days, but Microsoft/msbuild is separate from an earlier release - I'll dig more later today). I suppose the change would be to the task itself.

[akoeplinger]

No it isn't, as far as Roslyn is concerned you're targeting 4.6 (or more specifically an assembly that provides Array.Empty) so it is free to use anything from that profile. As I said, it'd also happily compile if your own code contained Array.Empty or any other new method.

MSBuild is here: https://github.com/Microsoft/msbuild

[akoeplinger]

Oh and I suspect the reason why a JIT optimization isn't a good idea is that you can't just replace new T[0] with Array.Empty<T>(), code may rely on getting a new instance and might fail if it gets the cached one, see https://github.com/dotnet/corefx/issues/2363 for some more discussion.

[NickCraver]

@akoeplinger I don't follow...how is that any different than the assumptions Roslyn is currently making when performing the optimization? I think we're talking about the same assumptions and optimizations, but a difference of where in the pipe the swap happens.

I'm digging into the MSBuild side of things now. IMO, this should be a break, but I'm going through their code to find if there is any reasoning documented. FWIW, we'd love to move off MSBuild as soon as possible (this is one of many behavioral reasons) - it's just not a quick thing to change.

[jaredpar]

Why isn't this a JIT optimization?

@akoeplinger nailed the reasoning here. Replacing every arbitrary new T[0] with Array.Empty<T> does have a non-trivial chance of breaking programs. And it in facts breaks part of the BCL (as we found when implementing this). That is why the Roslyn optimization limits itself to cases where the array is compiler generated. There is little chance developer code took a dependency on the uniqueness of compiler generated arrays.

This type of language specific optimization has no place in the JIT. Instead it should be focused on optimizations which are applicable to all languages and don't violate the stated rules of the underlying IL instructions.

So what happened is that the build server did not find .Net 4.5.2 reference assemblies and happily resolved .Net 4.6 assemblies instead.

This seems like a bug in MSBuild then. From Roslyn's perspective we are getting a reference to a 4.6 assembly and are basing our behavior off of that reference. The language has always changed code generation and enabled / disabled features based on the references it was given. Deploying to an earlier version of the framework in such a case always has the potential for runtime issues.

[FransBouma]

@akoeplinger The thing is that even though the person compiling the code might target 4.6, you can't determine that from the assembly at all: .net 4.0 loads it without problems. I think it's something else than explicitly referencing a .NET 4.6 specific class/assembly: with that one can say: "it's compiled against v4.6 for a reason". But if the code is actually .NET 4.5 compatible, but compiled against 4.6, it doesn't actively reference Array.Empty, but the _compiler_ introduces that dependency.

That's IMHO what's the problem here: the developer just compiled against 4.6, while the code is compatible with 4.5, but the compiler made it hard-dependent on 4.6, and at the same time the _user_ of the assembly has no way of knowing that, as the assembly references mscorlib 4.0 ;)

So the compiler introduced optimization is great, but it depends on using .NET 4.6 assemblies _at runtime_, something that's not guaranteed, as the IL references .NET 4.0 (not 4.6, as all assemblies are versioned 4.0). It's a bit of an edge case, (compiling against 4.6 and using it on a previous .NET version), nevertheless, the dependency is introduced by the compiler at compile time, which has unforeseen (IMHO) consequences at runtime.

One could argue: "but you compiled against 4.6, so don't run it on 4.5", but again, if one is handed the assembly and it 'works' (as the code is 4.5 compatible) on 4.5, it's not really determinable.

[akoeplinger]

@FransBouma well if you compile against a newer version of the framework than the one you're running on, all bets are off and if it works then you were just lucky :smile:

I of course agree that the way this is currently surfaced to the average developer is less than ideal and a potential source for confusion.

[jaredpar]

One could argue: "but you compiled against 4.6, so don't run it on 4.5",

That is exactly the problem here and it's not limited to 4.6 and 4.5. This rationale has existed in the compiler since C# 2.0. Virtually every version of the language since then has this dependency in some form.

New compiler features usually depend on new framework APIs. The compiler looks for these APIs in the set of references to know what features it can enable. If you then deploy to a framework without those types the all bets are off.

[FransBouma]

@jaredpar with one difference of course: the developer didn't make the dependency, the compiler did. :) In case of using e.g. .NET 4.5 specific elements and then running the compiled assembly on .net 4.0 is IMHO different, as the _developer_ made the dependency, not the compiler: the dependencies were known at compiletime _by the developer_.

[NickCraver]

I think @jaredpar's response covers an item I hadn't considered: the JIT's inability to determine a compiler generated empty array vs. one the user passed in - that makes it a hard non-option in this case. The optimization logic seems pretty sounds firm (and correct), csc.exe really isn't aware of the target - it's not a switch that MSBuild passes in.

The core of the issue here is that the build chain changes the dependency tree for deploy _for you_, without opting in. After digging through it - I'm of the opinion that not finding your specified framework reference assemblies to build against should be an _error_, not a _warning_ (with a "helpful" substitution) as it is now.

I'll close this out and open an issue specifically on the MSBuild project around this behavior where Roslyn is really just a downstream consumer of (IMO) bad references.

Thanks for the responses! I'll comment here when the new issue is ready - should be just a bit.

[jaredpar]

@FransBouma

MSBuild chose the dependencies here, not the compiler. The compiler only does the most naiive of assembly resolution when used on the command line. When invoked from MSBuild the compiler is given full paths to the assemblies in question and does no resolution itself.

[NickCraver]

An MSBuild issue has been opened to hopefully address this:
GAC Resolution fallback introduces unexpected bugs - should be an error instead of a warning

[niemyjski]

This needs to be fixed NOW. How the heck has this issue been open for 6 months. We just upgraded our build server on appveyor to .NET 4.6 (VS 2015 build image) and changed nothing. Now all of our customers who consume our nuget packages are getting this error. Who the heck thought this was acceptable to not take action for the past 6 months. I'd say this is a HUGE Compatibility issue.

[jaredpar]

@niemyjski

How the heck has this issue been open for 6 months.

This issue you're commenting on has been closed since August. There is no bug on the compiler side here, nor is there really any work around on our part. The compiler is responding, correctly, to the set of references that were provided to it.

The underlying bug here is in MSBuild as noted above. Issue 173 is tracking it.

[simonthum]

I've just been bitten by this and I don't think it's a pure MSBuild or deployment bug, nor would I agree that it is fixed!

I ask you to consider abolishing the optimization on LanguageLevel < 6, i.e. not just when it's technically impossible to execute but also when there is a reasonable expectation that the code's author did not consider the implications of that optimization.

The project that failed has an explicit LangageLevel of 5. That is, it is using the most appropriate tool (AFAIK) available outside Microsoft to communicate to the compiler that C# 5.0 syntax (and semantics, as far as is required) are desired. I recommend to use this information as one might expect it given a change in language semantics.

This probably needs to go tere:
https://github.com/dotnet/roslyn/pull/1117/files#diff-e863607288a2b39deee93a05d62deadcR621

[jaredpar]

The language version flag is used to control what language features are available, it does not control for optimizations implemented by the compiler. There are plenty of times when we've chosen in the past to optimize in new versions of the compiler irrespective of the language flag and this is just a new instance.

I've just been bitten by this and I don't think it's a pure MSBuild or deployment bug, nor would I agree that it is fixed!

I'm sorry but I disagree. The compiler can only process the input that it is given. In this case it is given a library that has types which we feel can make for a better program. If the program is then deployed with a different set of types then there really isn't much the compiler can do about that.

[simonthum]

Well, we seem too have different expectation as to what LangaugeLevel should do.

You say you have enabled optimizations in the past, but there is an underlying change in language semantics that is much harder to identify as a root cause than a MissingMethodException. And, it's not (just) an optimization.

I disabled optimizations in my bug hunting, and the failures remained. Portraying it as an optimization isn't quite exact - it's clearly a change in semantics. Quite close to a language feature, IMO. I admit that the semantics were not the failure mode for me, but treating them based on the LanguageLevel certainly would have helped, and is likely to help others bitten by that issue.

I like the change, in fact when I found out about it I wondered why it was not in C# 2.0, but given its slightly lopsided introduction please consider a more cautious if.

[mihailik]

It looks like the Microsoft itself made the same mistake, and built Microsoft.Net.Compilers nuget package using wrong set of tools/on wrong server.

https://github.com/dotnet/roslyn/issues/18572

Nuget package meant to support .NET 4.5.2 doesn't work on .NET 4.5.2 with the Array.Empty-related bug.

[mihailik]

@jaredpar you've been involved with #18572 , can you please clarify this versioning logic here?

If the problem is indeed in MSBuild, but it creeps on to Roslyn's own build process — it may require a more proactive solution.

The purpose of that nuget package has always been to allow builds on a statically defined version of tools, regardless of the local framework install. These bugs mean we can build on any framework, as long as that framework is 4.6.1.

[jaredpar]

@mihailik

It looks like the Microsoft itself made the same mistake, and built Microsoft.Net.Compilers nuget package using wrong set of tools/on wrong server.

We made a different mistake here. The binaries contained in that NuGet package (csc.exe, vbc.exe, etc ...) deliberately changed their minimum required runtime from 4.5.2 to 4.6 in the 2.0 release timeframe. There were a number of reasons for this, in particular we needed access to some threading primitives to help with performance issues in the workspace / IDE layers. The minimum runtime change though was deliberate.

The mistake we made was failing to note the new minimum runtime requirement in our app.config file. Hence when the binaries are run on 4.5.2 it ends up with errors like missing Array.Empty instead of printing a more helpful message about minimum runtime.

The primary tracking issue for this is #17908 and the PR to add the entry is #18018.

[evil-shrike]

BTW NuGet.exe 4.6.2 fails with this error on a machine where 4.5.1 .NET Framework installed.

Unhandled Exception: System.MissingMethodException: Method not found: '!!0[] System.Array.Empty()'.
   at NuGet.Common.CommandLineResponseFile.ParseArgsResponseFiles(String[] args, Int32 parseArgsResponseFileRecursionDepth)
   at NuGet.CommandLine.Program.Main(String[] args)

From HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft.NETFramework\v4.0.30319\SKUs:

[jaredpar]

@evil-shrike in this case it looks like an application was compiled while targeting .NET 4.6.2 and then deployed to a machine that only has 4.5.1 installed. The application crashing due to missing APIs is expected in this case. If the intent is to deploy the application there then it needs to be compiled against 4.5.1.

In this case the binary is NuGet.exe. They should also likely include a minimum runtime requirement in their app.config to make this more obvious.