Fix #2423: Coverlet: don't use implicit yield

[Tarmil]

Use explicit yield to construct the MSBuild param list to make sure that they don't get ignored by an older language version.

[matthid]

fixes #2423

Do we know why the tests passed? /cc @SteveGilham

[SteveGilham]

With the current tip (commit cd7d749) the code generated for the function of interest, Fake.DotNet.Testing.Coverlet.properties@92.GenerateNext, is very different in debug and release builds. Debug is the sort of state machine one would expect, and emits the expected results in order; Release evaluates all the results in the initial state, then drops them on the floor, to give an empty output. This has to be an underlying compiler issue.

[matthid]

Maybe a compiler bug in the new feature /cc @dsyme

[SteveGilham]

Bumping the .net SDK version to 3.0.100 in global.json from 2.1.508 -- picking up the tooling which has the implicit yield implementation -- puts the full state machine into the Release build which now matches up with the Debug version.

[matthid]

@SteveGilham As the CI always builds in Release mode, shouldn't the test still fail in CI, before this PR?

[SteveGilham]

That the tests pass is still a mystery to me.

[SteveGilham]

Not sure if related, but reverting the global.json, I loaded Fake.sln in Visual Studio, and in Release configuration, attempted to debug the relevant unit tests. In the process of the build that kicks off, the Coverlet assembly went to version 1.0.0.0 and the GenerateNext method in that version had the state machine OK. I couldn't step through the tests, but they went green.

[dsyme]

With the current tip (commit cd7d749) the code generated for the function of interest....

Which compiler is being used here?

If an old compiler is being used, then I assume it's pouring out warnings about ignored values, and then release mode is removing them.

[matthid]

Ok, I took a look at why this could happen. It seems to be a VERY unfortunate combination of events. Here is what I found:

• Test code failed due to this:
  
  So even the props are empty the test is still green. -> Reported Can we make containsAll fail when the expected argument is an empty list? haf/expecto#359

• In fact, when using dotnet build on the command line (when using the CI) an old compiler is used (properly emitting the warnings as @dsyme said)

• When using Visual Studio 2019 for building and running the tests a new compiler is used and you have no warnings (and even worse when you debug it you see 'expected' behavior), even though global.json locks an older version. This is VERY unfortunate. @cartermp, @dsyme I think this is most likely a "known limitation" or "by design", should I open an issue anyway?

[cartermp]

This would be by design. Since tooling is always built and tested against the compiler that it ships with, that's what is always used. There's no way to use an older compiler by setting global.json and there isn't a correspondence between a value there and a given LangVersion configuration. This might be machinery someone could build, but I highly doubt it's on anyone's priority list at Microsoft.

[matthid]

@cartermp Ok not opening an issue then, I still think it is worth keeping an eye open to see if others fall into the same trap. Thanks for the fast response.