Remove duplicate validation logic #42769

jaredpar · 2020-03-25T18:36:05Z

The MSBuild task for csc / vbc compilers contained validation to detect
that no duplicate source files or resources were provided. This
validation is duplicated with logic in the compiler hence it's
unnecessary.

Further this use came during ValidateParameters which means that when
it fails the underlying task doesn't actually execute. This means that
design time builds don't process the project in the face of that type of
error which blocks a number of Visual Studio based scenarios.

closes #42465

The MSBuild task for csc / vbc compilers contained validation to detect that no duplicate source files or resources were provided. This validation is duplicated with logic in the compiler hence it's unnecessary. Further this use came during `ValidateParameters` which means that when it fails the underlying task doesn't actually execute. This means that design time builds don't process the project in the face of that type of error which blocks a number of Visual Studio based scenarios. closes dotnet#42465

jaredpar · 2020-03-25T19:36:21Z

@dotnet/roslyn-compiler @jasonmalinowski PTAL

jasonmalinowski

I'm OK with this if you're OK with this!

jcouv

LGTM Thanks

jaredpar · 2020-03-25T20:47:08Z

I'm OK with this if you're OK with this!

@rainersigwald is okay with this so who am I to dispute?

rainersigwald

No pressure! 😅

agocke · 2020-03-25T22:55:25Z

@jaredpar Is there a Csc/Vbc task test? And a command line test that demonstrates our resilience?

jaredpar · 2020-03-25T23:12:02Z

@agocke

There are existing compiler tests:

Similar tests exist for VB. The C# tests actually demonstrate another reason for removing this check. The compiler is actually tolerant of duplicate source files, it just emits a warning.

There were no Task tests.

agocke · 2020-03-25T23:14:05Z

@jaredpar do you want to add one to http://sourceroslyn.io/#Microsoft.Build.Tasks.CodeAnalysis.UnitTests/CscTests.cs,691dd5a92870fd19? 😄

jaredpar · 2020-03-25T23:16:46Z

@agocke

Tests to just verify the files are passed multiple times?

jasonmalinowski · 2020-03-25T23:40:04Z

The compiler is actually tolerant of duplicate source files, it just emits a warning.

Which case comparison was it using in that case? I was surprised this was doing case-insensitive, because you were just in trouble if you had two differently cased files on Linux. At least if it's a warning in the compiler you have some hope.

(I of course pray nobody is relying on this, but alas....)

agocke · 2020-03-26T03:05:24Z

Ah, I see, so the task unit test wouldnt' exercise the code path that was previously causing the problem. OK, in that case we can proceed, but it would be good to see how we can add a regression test for the scenario.

jaredpar · 2020-03-26T03:24:08Z

@jasonmalinowski

Which case comparison was it using in that case?

The compiler is rather inconsistent in how it applies case to paths. Back when I re-joined Roslyn I tried to rationalize it but eventually gave up. I think the only rationalization we could ever really add
without breaking compat was that the compiler always compares file paths case insensitive. I'd prefer the other but I fear that would definitely break compat.

rainersigwald · 2020-03-26T11:59:08Z

That story is very familiar to me; it's basically where we've landed in MSBuild. Don't distinguish files based only on case for .NET code: that way lies pain.