search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.97k stars 4.03k forks source link

Warnings CS1701 and CS1702 should be removed #19640

Open nguerrera opened 7 years ago

nguerrera commented 7 years ago

These warnings assume runtime behavior that only holds on full .NET framework: that you need to supply "policy" to bind a ref to a lower assembly to a higher version. This means that the warnings have to be disabled when targeting other .NET platforms

Furthermore, even when targeting .NET Framework, msbuild will also warn and so removing these warnings doesn't prevent our greater build ecosystem from diagnosing the issue where it actually applies. Plus, msbuild actually understands app.config binding redirects in making the call on whether to warn, and it can even generate a correct one for you (via AutoGenerateBindingRedirects=true), which it recommends in its warning.

These warnings have had to be disabled in project templates for a long time. Searching for "NoWarn" 1701 on GitHub yields 250K results: https://github.com/search?q=NoWarn+1701&type=Code&utf8=%E2%9C%93

dotnet/sdk disables them for you to avoid cluttering your csproj, but this opens up issues around the ordering of NoWarn between user and SDK. I'd like to get the SDK out of the business of disabling warnings. Over the past several years, the only times I see 1701 is when a build misconfiguration has undone a vital NoWarn, and nobody can decipher the message. e.g. https://github.com/dotnet/sdk/issues/1205

nguerrera commented 6 years ago

Just found something funny: The core compiler targets actually try to disable it for anything other than ancient .NETFramework v1.0 or v1.1:

https://github.com/dotnet/roslyn/blob/aa683ac4ec4c6ce746776a2a316309fe6a7d1740/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L40-L45

This doesn't check TFM so it started showing up in .NETStandard and .NETCoreApp 1.0 and 1.1!

We should just remove this from the targets and delete the warnings.

Dean-NC commented 2 years ago

Still hanging around in VS 2022 17.2

stevenvolckaert commented 1 year ago

I confirm this is still the case in Visual Studio 2022 17.4.2

Youssef1313 commented 1 year ago

It looks like if a project doesn't define <NoWarn>, the SDK will automatically set CS1701 and CS1702.

https://github.com/dotnet/sdk/blob/1139facea93c648f5afe8f8e8d802396dc6fdabc/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.CSharp.props#L15

So it looks like a typical simple project will have these suppressed automatically by the SDK.

@jcouv Any concerns removing these from the compiler?

jcouv commented 1 year ago

@Youssef1313 I conferred with Jared and he'd be fine to remove them.

jaredpar commented 1 year ago

The only issue is I'm not exactly sure if there is any particular process we need to follow when removing a warning from the compiler. Items that need to be considered are:

  1. What happens when a /nowarn for a non-existent diagnostic happens?
  2. What happens when a /warn or /warnaserror happens for a non-existent diagnostic?

These may have simple answers, I haven't looked, but they need to be validated. Another option may be to just leave the diagnostics in place but set them to disabled by default.

Youssef1313 commented 1 year ago

@jaredpar I think they will (or at least should) be ignored