Implement compiler /warnversion flag and one "wave" warning #45750

gafter · 2020-07-07T17:19:32Z

Fixes #45702
Fixes #45744
Related to #45701 and #45704

Fixes dotnet#45702 Fixes dotnet#45744 Related to dotnet#45701 and dotnet#45704

jaredpar · 2020-07-07T17:21:34Z

Looks like you're missing the MSBuild hookup to the switch here. #Resolved

…ructor.

gafter · 2020-07-07T18:50:06Z

Looks like you're missing the MSBuild hookup to the switch here.

See #45701 for the work breakdown. This is the compiler portion only. The MSBuild hookup is tracked by dotnet/roslyn-analyzers#3823 and @jmarolf plans to implement it. #Resolved

Fixes dotnet#45760

…-warnversion

jmarolf · 2020-07-09T17:58:50Z

@jaredpar yep I am going to build on top of this #Resolved

RikkiGibson · 2020-07-09T20:06:45Z

src/Compilers/CSharp/Test/Semantic/Semantics/WarningVersionTests.cs

+            CreateCompilation(source, options: TestOptions.ReleaseDll.WithWarningVersion(4.9m)).VerifyDiagnostics();
+            CreateCompilation(source, options: TestOptions.ReleaseDll.WithWarningVersion(5m)).VerifyDiagnostics(whenWave5);
+            CreateCompilation(source, options: TestOptions.ReleaseDll.WithWarningVersion(5.0m)).VerifyDiagnostics(whenWave5);
+            CreateCompilation(source, options: TestOptions.ReleaseDll.WithWarningVersion(5.1m)).VerifyDiagnostics(whenWave5);


Is there a possibility of a platform version like "dotnet 5.10", which is later than dotnet 5.2? #ByDesign

I don't think we are planning to use minor versions for .net core at all (though the current version is 3.1). It seems unlikely we would have so many minor versions without a major version bump. Also, many people would be confused if we did that.

In reply to: 452460481 [](ancestors = 452460481)

jcouv · 2020-07-09T21:11:34Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -4617,7 +4620,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
 -warnaserror[+|-]             Report all warnings as errors
 -warnaserror[+|-]:&lt;warn list&gt; Report specific warnings as errors
                              (use "nullable" for all nullability warnings)
-warn:&lt;n&gt;                     Set warning level (0-4) (Short form: -w)


Should we keep the old setting, since still works? #Closed

No, since we do not want to encourage people to use it. The UI for setting this is also being removed from VS.

In reply to: 452492523 [](ancestors = 452492523)

jcouv · 2020-07-09T21:12:59Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

+Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.CSharpCompilationOptions(Microsoft.CodeAnalysis.OutputKind outputKind, bool reportSuppressedDiagnostics = false, string moduleName = null, string mainTypeName = null, string scriptClassName = null, System.Collections.Generic.IEnumerable<string> usings = null, Microsoft.CodeAnalysis.OptimizationLevel optimizationLevel = Microsoft.CodeAnalysis.OptimizationLevel.Debug, bool checkOverflow = false, bool allowUnsafe = false, string cryptoKeyContainer = null, string cryptoKeyFile = null, System.Collections.Immutable.ImmutableArray<byte> cryptoPublicKey = default(System.Collections.Immutable.ImmutableArray<byte>), bool? delaySign = null, Microsoft.CodeAnalysis.Platform platform = Microsoft.CodeAnalysis.Platform.AnyCpu, Microsoft.CodeAnalysis.ReportDiagnostic generalDiagnosticOption = Microsoft.CodeAnalysis.ReportDiagnostic.Default, int warningLevel = 4, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Microsoft.CodeAnalysis.ReportDiagnostic>> specificDiagnosticOptions = null, bool concurrentBuild = true, bool deterministic = false, Microsoft.CodeAnalysis.XmlReferenceResolver xmlReferenceResolver = null, Microsoft.CodeAnalysis.SourceReferenceResolver sourceReferenceResolver = null, Microsoft.CodeAnalysis.MetadataReferenceResolver metadataReferenceResolver = null, Microsoft.CodeAnalysis.AssemblyIdentityComparer assemblyIdentityComparer = null, Microsoft.CodeAnalysis.StrongNameProvider strongNameProvider = null, bool publicSign = false, Microsoft.CodeAnalysis.MetadataImportOptions metadataImportOptions = Microsoft.CodeAnalysis.MetadataImportOptions.Public, Microsoft.CodeAnalysis.NullableContextOptions nullableContextOptions = Microsoft.CodeAnalysis.NullableContextOptions.Disable, decimal warningVersion = 0) -> void
+Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.CSharpCompilationOptions(Microsoft.CodeAnalysis.OutputKind outputKind, bool reportSuppressedDiagnostics, string moduleName, string mainTypeName, string scriptClassName, System.Collections.Generic.IEnumerable<string> usings, Microsoft.CodeAnalysis.OptimizationLevel optimizationLevel, bool checkOverflow, bool allowUnsafe, string cryptoKeyContainer, string cryptoKeyFile, System.Collections.Immutable.ImmutableArray<byte> cryptoPublicKey, bool? delaySign, Microsoft.CodeAnalysis.Platform platform, Microsoft.CodeAnalysis.ReportDiagnostic generalDiagnosticOption, int warningLevel, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Microsoft.CodeAnalysis.ReportDiagnostic>> specificDiagnosticOptions, bool concurrentBuild, bool deterministic, Microsoft.CodeAnalysis.XmlReferenceResolver xmlReferenceResolver, Microsoft.CodeAnalysis.SourceReferenceResolver sourceReferenceResolver, Microsoft.CodeAnalysis.MetadataReferenceResolver metadataReferenceResolver, Microsoft.CodeAnalysis.AssemblyIdentityComparer assemblyIdentityComparer, Microsoft.CodeAnalysis.StrongNameProvider strongNameProvider, bool publicSign, Microsoft.CodeAnalysis.MetadataImportOptions metadataImportOptions, Microsoft.CodeAnalysis.NullableContextOptions nullableContextOptions) -> void
+Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.WarningVersion.get -> decimal
+Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.WithWarningVersion(decimal warningVersion) -> Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions


decimal [](start = 74, length = 7)

Should we use an enum (like we do for WithLanguageVersion)? #ByDesign

No, as there is not a fixed set of values you can specify.

In reply to: 452493235 [](ancestors = 452493235)

jcouv · 2020-07-09T21:13:46Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -4617,7 +4620,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
 -warnaserror[+|-]             Report all warnings as errors
 -warnaserror[+|-]:&lt;warn list&gt; Report specific warnings as errors
                              (use "nullable" for all nullability warnings)
-warn:&lt;n&gt;                     Set warning level (0-4) (Short form: -w)
+-warnversion:&lt;n&gt;        Enable recently introduced warnings. Default '0'.


Should we support -warnversion:? as we do for -langversion:? #ByDesign

No plans to do so, since there is not a fixed set of values you can specify.

In reply to: 452493597 [](ancestors = 452493597)

jcouv · 2020-07-09T21:15:04Z

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs

+        /// the warning wave in which it was introduced.
+        /// </summary>
+        /// <param name="code"></param>
+        /// <returns></returns>


nit: xml can be cleaned up #Closed

jcouv · 2020-07-09T21:17:03Z

docs/compilers/CSharp/Warnversion Warning Waves.md

+`k` or later is used to compile the code.
+
+The default warning version is `0` (produce no optional warnings).
+Our first warning under control of `/warnversion` was introduced in version `5`.


nit: may be good to mention this corresponds to .NET 5 #Closed

jcouv · 2020-07-09T21:18:06Z

src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_Warnings.cs

@@ -792,7 +792,7 @@ private void CheckLiftedBinOp(BoundBinaryOperator node)

                    string always = node.OperatorKind.Operator() == BinaryOperatorKind.NotEqual ? "true" : "false";

-                    if (_compilation.FeatureStrictEnabled || !node.OperatorKind.IsUserDefined())
+                    if (_compilation.FeatureStrictEnabled || !node.OperatorKind.IsUserDefined() || ErrorCode.WRN_NubExprIsConstBool2.GetWarningVersion() <= this._compilation.Options.WarningVersion)


FeatureStrictEnabled [](start = 37, length = 20)

Should we assume that FeatureStrictEnabled is true when warn version >=5? #Closed

No. FeatureStrictEnabled is orthogonal and most of the diagnostics under its control are currently errors. Once we migrate something to a warning wave we can remove it from FeatureStrictEnabled after a period of time sufficient for people to start using it.

In reply to: 452495746 [](ancestors = 452495746)

jcouv

Done with review pass (iteration 4)

…-warnversion

gafter · 2020-07-10T15:17:09Z

@jcouv I have responded to your comments. Do you have any others?

jmarolf · 2020-07-10T18:52:38Z

@gafter I do think someone needs to update the compiler MSBuild tasks to accept this new option right?

I want to be able to make this change to the msbuild targets

    <Csc Resources="@(_SatelliteAssemblyResourceInputs)"
         Sources="$(_AssemblyInfoFile)"
         OutputAssembly="$(_OutputAssembly)"
         References="@(ReferencePath)"
         KeyContainer="$(KeyContainerName)"
         KeyFile="$(KeyOriginatorFile)"
         NoConfig="true"
         NoLogo="$(NoLogo)"
         NoStandardLib="$(NoCompilerStandardLib)"
         PublicSign="$(PublicSign)"
         DelaySign="$(DelaySign)"
         Deterministic="$(Deterministic)"
         DisabledWarnings="$(DisabledWarnings)"
         WarningLevel="$(WarningLevel)"
         WarningsAsErrors="$(WarningsAsErrors)"
         WarningsNotAsErrors="$(WarningsNotAsErrors)"
+        WarnVersion="$(WarnVersion)"
         TargetType="Library"
         ToolExe="$(CscToolExe)"
         ToolPath="$(CscToolPath)"
         UseSharedCompilation="$(UseSharedCompilation)">

But I believe the Csc task needs to be updated to support this option #Resolved

jcouv

LGTM Thanks (iteration 6)

mavasani · 2020-07-12T00:36:48Z

docs/compilers/CSharp/Warnversion Warning Waves.md

+as part of .NET 5.
+If you want the compiler to produce all applicable warnings, you can specify
+`/warnversion=9999`.
+In the project file, the property used to specify the warning version is `AnalysisLevel`.


Why not even call the MSBuild project property WarnVersion? #Resolved

I think because it is intended it be used to specify a level of analysis for analyzers to use too.

In reply to: 453250128 [](ancestors = 453250128)

mavasani · 2020-07-12T00:37:20Z

docs/compilers/CSharp/Warnversion Warning Waves.md

@@ -0,0 +1,25 @@
+# /warnversion warning "waves"


Will this flag be added/respected for VB as well? #Resolved

my understanding is that it would not be at this time, but it could be added later if there are warnings we want to add #Resolved

mavasani · 2020-07-12T00:38:36Z

src/Compilers/CSharp/Portable/CSharpCompilationOptions.cs

+        /// associated with the warning. Use zero to get none of the versioned warnings, or a large number like 9999
+        /// to get all of the versioned warnings. The first version that enables some warnings is version 5.
+        /// </remarks>
+        public decimal WarningVersion { get; private set; }


Why decimal and not Version, given the property itself is named version? #Resolved

#45941

In reply to: 453250225 [](ancestors = 453250225)

mavasani · 2020-07-12T00:40:52Z

docs/compilers/CSharp/Warnversion Warning Waves.md

+Our first warning under control of `/warnversion` was introduced in version `5`
+as part of .NET 5.
+If you want the compiler to produce all applicable warnings, you can specify
+`/warnversion=9999`.


Should this instead be a special string, say latest instead of a number? This would also match the LangVersion property values. #Resolved

Open issue tracked by #45941

In reply to: 453250389 [](ancestors = 453250389)

gafter · 2020-07-13T21:23:43Z

I (mistakenly?) thought you were planning to do that under dotnet/roslyn-analyzers#3823. Should I add that to this PR?

In reply to: 656830706 [](ancestors = 656830706)

…-warnversion

jasonmalinowski · 2020-07-15T00:14:12Z

...tures/CSharpTest/Completion/CompletionProviders/PropertySubPatternCompletionProviderTests.cs

@@ -503,7 +503,7 @@ class D
    public int P2 { get; set; }
 }
 ";
-            await VerifyItemExistsAsync(markup, "P1");
+            await VerifyNoItemsExistAsync(markup);


Did this sneak in from another unrelated PR? #Resolved

No, just fixing a test broken in the merged branch. Probably broken in master too. See #45990 (comment) #Resolved

…-warnversion

jmarolf · 2020-07-16T00:03:38Z

I (mistakenly?) thought you were planning to do that under dotnet/roslyn-analyzers#3823. Should I add that to this PR?

@gafter so I believe there are several phases to this:

C# compiler commandline support (this PR: Implement compiler /warnversion flag and one "wave" warning #45750)
C# compiler build task (add /warnversion to csc msbuild task #45940)
.NET SDK passes argument to C# compiler build task (Add AnalysisLevel to configure both compiler warning wave and active analyzers sdk#12422)
IDE support for new compilation option (in progress)
Project property page support (in progress)
Project system support? I'm still investigating

Each of these needs to go in, for the most part, in the order listed but I don't think we should block this PR on that. Once the compiler team is ok with the changes to the commandline api lets merge this and then I'll rebase the downstream PRs with whatever changes are made

ghost

Auto-approval

steveharter · 2020-08-03T18:23:24Z

It appears the latest version of the Runtime\SDK (5.0.0-rc.1.20401.1 \ 5.0.100-rc.1.20403.5) causes a simple console app project to not compile in VS. Also running VS Version 16.7.0 Preview 6.0.

Compiling gives:
Error CS1900 Warning level must be in the range 0-4

Each of these needs to go in, for the most part, in the order listed but I don't think we should block this PR on that. Once the compiler team is ok with the changes to the commandline api lets merge this and then I'll rebase the downstream PRs with whatever changes are made
...
4. IDE support for new compilation option (in progress)

Any suggestions to unblock? Using <NoWarn>$(NoWarn);1900</NoWarn> in the csproj doesn't help.

gafter · 2020-08-03T18:43:43Z

@steveharter Until this version of the compiler is integrated, use warning level 4.

steveharter · 2020-08-03T18:56:19Z

@gafter the .csproj is not specifying any warning level; it's using the default. I also tried creating a new console app with the same error.

Is there an environment variable?

jmarolf · 2020-08-03T18:57:49Z

You can specify <AnalysisLevel>4</AnalysisLevel> in the project file to get the old behavior while we wait for VS to put out a new build

jmarolf · 2020-08-03T18:58:41Z

For those internal to microsoft this contains the fix on the VS side. internal vs builds will be working within a day.

steveharter · 2020-08-03T19:02:04Z

@jmarolf the <AnalysisLevel>4</AnalysisLevel> worked; thanks.

steveharter · 2020-08-04T15:09:37Z

@jmarolf does this also require a down-level fix in VS? (older VS, newer compiler?)

jmarolf · 2020-08-04T22:28:50Z

@steveharter .NET 5 is only going to work with VS 16.8. Moving forward you are going to need either the .NET 5 SDK to build or VS 16.8

Implement compiler /warnversion flag and one "wave" warning

d49e2af

Fixes dotnet#45702 Fixes dotnet#45744 Related to dotnet#45701 and dotnet#45704

gafter requested a review from a team as a code owner July 7, 2020 17:19

gafter self-assigned this Jul 7, 2020

gafter added Area-Compilers New Feature - Warning Waves Warning Waves Language-C# labels Jul 7, 2020

gafter added this to the 16.8 milestone Jul 7, 2020

Fix public API file to reflect changed CSharpCompilationOptions const…

8605348

…ructor.

Neal Gafter added 2 commits July 7, 2020 17:13

Document warning waves

37520cf

Fixes dotnet#45760

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

32edbd5

…-warnversion

RikkiGibson reviewed Jul 9, 2020

View reviewed changes

RikkiGibson approved these changes Jul 9, 2020

View reviewed changes

jcouv reviewed Jul 9, 2020

View reviewed changes

jcouv self-assigned this Jul 9, 2020

Neal Gafter added 2 commits July 9, 2020 19:21

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

696ddef

…-warnversion

Minor changes per review.

37a45de

jmarolf mentioned this pull request Jul 10, 2020

Add AnalysisLevel to configure both compiler warning wave and active analyzers dotnet/sdk#12422

Merged

jcouv approved these changes Jul 11, 2020

View reviewed changes

mavasani reviewed Jul 12, 2020

View reviewed changes

jmarolf mentioned this pull request Jul 13, 2020

add /warnversion to csc msbuild task #45940

Closed

Neal Gafter added 3 commits July 13, 2020 14:45

Make the parsing of warnversion culture invariant.

805b45a

Adjust test to make it culture neutral.

03f1b41

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

ba76700

…-warnversion

gafter requested a review from a team as a code owner July 15, 2020 00:09

jasonmalinowski reviewed Jul 15, 2020

View reviewed changes

Merge branch 'master' of https://github.com/dotnet/roslyn into master…

f78a347

…-warnversion

gafter added the auto-merge label Jul 16, 2020

ghost approved these changes Jul 16, 2020

View reviewed changes

ghost merged commit dbdae0b into dotnet:master Jul 16, 2020

This pull request was closed.

Implement compiler /warnversion flag and one "wave" warning #45750

Implement compiler /warnversion flag and one "wave" warning #45750

Conversation

gafter commented Jul 7, 2020

jaredpar commented Jul 7, 2020 • edited by gafter Loading

gafter commented Jul 7, 2020 • edited Loading

jmarolf commented Jul 9, 2020 • edited by gafter Loading

RikkiGibson Jul 9, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

gafter Jul 10, 2020

Choose a reason for hiding this comment

jcouv Jul 9, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 10, 2020

Choose a reason for hiding this comment

jcouv Jul 9, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 10, 2020

Choose a reason for hiding this comment

jcouv Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

gafter Jul 10, 2020

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

gafter commented Jul 10, 2020

jmarolf commented Jul 10, 2020 • edited by gafter Loading

jcouv left a comment

Choose a reason for hiding this comment

mavasani Jul 12, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 13, 2020

Choose a reason for hiding this comment

mavasani Jul 12, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

jmarolf Jul 13, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

mavasani Jul 12, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 13, 2020

Choose a reason for hiding this comment

mavasani Jul 12, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 13, 2020

Choose a reason for hiding this comment

gafter commented Jul 13, 2020

jasonmalinowski Jul 15, 2020 • edited by gafter Loading

Choose a reason for hiding this comment

gafter Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

jmarolf commented Jul 16, 2020

ghost left a comment

Choose a reason for hiding this comment

steveharter commented Aug 3, 2020 • edited Loading

gafter commented Aug 3, 2020

steveharter commented Aug 3, 2020

jmarolf commented Aug 3, 2020

jmarolf commented Aug 3, 2020

steveharter commented Aug 3, 2020

steveharter commented Aug 4, 2020 • edited Loading

jmarolf commented Aug 4, 2020

jaredpar commented Jul 7, 2020 •

edited by gafter

Loading

gafter commented Jul 7, 2020 •

edited

Loading

jmarolf commented Jul 9, 2020 •

edited by gafter

Loading

RikkiGibson Jul 9, 2020 •

edited by gafter

Loading

gafter Jul 10, 2020 •

edited

Loading

jcouv Jul 9, 2020 •

edited

Loading

jcouv Jul 9, 2020 •

edited by gafter

Loading

jcouv Jul 9, 2020 •

edited by gafter

Loading

jcouv Jul 9, 2020 •

edited

Loading

jcouv Jul 9, 2020 •

edited

Loading

jcouv Jul 9, 2020 •

edited

Loading

jmarolf commented Jul 10, 2020 •

edited by gafter

Loading

mavasani Jul 12, 2020 •

edited by gafter

Loading

mavasani Jul 12, 2020 •

edited by gafter

Loading

jmarolf Jul 13, 2020 •

edited by gafter

Loading

mavasani Jul 12, 2020 •

edited by gafter

Loading

mavasani Jul 12, 2020 •

edited by gafter

Loading

jasonmalinowski Jul 15, 2020 •

edited by gafter

Loading

gafter Jul 15, 2020 •

edited

Loading

steveharter commented Aug 3, 2020 •

edited

Loading

steveharter commented Aug 4, 2020 •

edited

Loading