Do not skip emit if errors are suppressed #34174

agocke · 2019-03-15T21:04:28Z

In the command line compilation we try to discover when to stop the
compilation stages based on if an error is produced. If the error was
produced by /warnaserror, then suppressed, this should not be considered
a compilation-halting error.

Fixes #34101

In the command line compilation we try to discover when to stop the compilation stages based on if an error is produced. If the error was produced by /warnaserror, then suppressed, this should not be considered a compilation-halting error. Fixes dotnet#34101

src/Compilers/CSharp/Test/Emit/Emit/EndToEndTests.cs

src/Compilers/Core/Portable/Compilation/Compilation.cs

jcouv · 2019-03-15T21:30:26Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

                var filtered = Options.FilterDiagnostic(d);
                if (filtered == null ||
                    (!reportSuppressedDiagnostics && filtered.IsSuppressed))
                {
                    continue;
                }
-                else if (filtered.Severity == DiagnosticSeverity.Error)
+                else if (filtered.Severity == DiagnosticSeverity.Error &&
+                         !filtered.IsSuppressed)


!filtered.IsSuppressed) [](start = 25, length = 23)

~~can errors be suppressed? I thought only warnings could.~~

I see that IsReportedError also handles this combination (suppressed error) #Closed

jcouv

LGTM Thanks (iteration 1) with some nits

jaredpar · 2019-03-15T21:51:36Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

@@ -434,6 +434,18 @@ internal bool ReportErrors(IEnumerable<Diagnostic> diagnostics, TextWriter conso
        private bool ReportErrors(DiagnosticBag diagnostics, TextWriter consoleOutput, ErrorLogger errorLoggerOpt)
            => ReportErrors(diagnostics.ToReadOnly(), consoleOutput, errorLoggerOpt);

+        internal static bool HasUnsuppressedErrors(DiagnosticBag diagnostics)


The name threw me off here a bit. Felt like has not fake error. Consider a more positive term like HasReal/ReportedError.

Suppressions may or may not be reported. A warning elevated to an error isn't a "real" error to me. This is the most accurate term I can think of.

jaredpar · 2019-03-15T21:53:17Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

@@ -95,6 +95,32 @@ private CSharpCommandLineArguments FullParse(string commandLine, string baseDire
            return CSharpCommandLineParser.Default.Parse(args, baseDirectory, sdkDirectory, additionalReferenceDirectories);
        }

+        [ConditionalFact(typeof(WindowsDesktopOnly), Reason = ConditionalSkipReason.TestHasWindowsPaths)]


Where is the windows path here?

There technically isn't one, but I'm running an exe, which only works on Windows desktop and needed a reason.

jaredpar

AlekseyTs · 2019-03-15T22:24:16Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

@@ -434,6 +434,18 @@ internal bool ReportErrors(IEnumerable<Diagnostic> diagnostics, TextWriter conso
        private bool ReportErrors(DiagnosticBag diagnostics, TextWriter consoleOutput, ErrorLogger errorLoggerOpt)
            => ReportErrors(diagnostics.ToReadOnly(), consoleOutput, errorLoggerOpt);

+        internal static bool HasUnsuppressedErrors(DiagnosticBag diagnostics)
+        {
+            foreach (var diag in diagnostics.AsEnumerableWithoutResolution())


AsEnumerableWithoutResolution [](start = 45, length = 29)

Why without resolution? What if we there is a lazy diagnostics that becomes an error after resolution? #Closed

My understanding is that the next line causes resolution (checks severity) anyways.

My understanding is that the next line causes resolution (checks severity) anyways.

Then why pretend that we do not resolve?

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

I'll let Andy respond, but my assumption in reading the PR was to avoid extra processing which AsEnumerable does.
But I agree it's not obvious ;-)

Yes, it was for perf and the places I was looking at it I thought it was safe because we always realized diagnostics later, but it's not particularly worth the risk. I've changed it to resolve.

AlekseyTs · 2019-03-15T22:29:56Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

                var filtered = Options.FilterDiagnostic(d);
                if (filtered == null ||
                    (!reportSuppressedDiagnostics && filtered.IsSuppressed))
                {
                    continue;
                }
-                else if (filtered.Severity == DiagnosticSeverity.Error)
+                else if (filtered.Severity == DiagnosticSeverity.Error &&
+                         !filtered.IsSuppressed)


!filtered.IsSuppressed [](start = 25, length = 22)

It looks like previous if takes care of checking this. Shouldn't we respect the reportSuppressedDiagnostics option? #Closed

We are respecting the option. If that option is false we won't set hasErrors, but we also won't add the diagnostic to the bag. In this case we don't set hasErrors but we do add the diagnostic to the bag.

In other words, hasErrors is used to mean "stop compilation" if true, but that's not what was happening. In this case there were errors, but that doesn't imply we should stop compilation.

And reporting is just a completely different option that controls whether or not the diagnostic is output to the log at all.

AlekseyTs · 2019-03-15T22:49:04Z

src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs

@@ -434,6 +434,18 @@ internal bool ReportErrors(IEnumerable<Diagnostic> diagnostics, TextWriter conso
        private bool ReportErrors(DiagnosticBag diagnostics, TextWriter consoleOutput, ErrorLogger errorLoggerOpt)
            => ReportErrors(diagnostics.ToReadOnly(), consoleOutput, errorLoggerOpt);

+        internal static bool HasUnsuppressedErrors(DiagnosticBag diagnostics)


HasUnsuppressedErrors [](start = 29, length = 21)

It looks like this method is supposed to be called after the filtering is applied to content of the bag. It doesn't look like this happens for every call site. #Closed

I'm not sure it matters. Since you can't suppress an error that was originally an error, it shouldn't make a difference.

@AlekseyTs Did this address your concern?

Did this address your concern?

I do not think it did. If we haven't done filtering, we didn't have a chance to apply warn as error, correct?

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

Also, if it doesn't matter at many call sites, as you are saying. Then why adjusting those call sites?

In reply to: 266677762 [](ancestors = 266677762,266674162)

That's correct. I guess I might be thinking of a different scenario: my intention was only to use this helper as a "bailout" mechanism to stop compilation, meaning that not catching errors as early as possible may affect how soon compilation stops but not affect semantic guarantees.

Did you find a place where I'm not doing that?

AlekseyTs · 2019-03-15T22:50:55Z

src/Compilers/Core/Portable/Compilation/Compilation.cs

@@ -2578,7 +2581,7 @@ internal void EnsureAnonymousTypeTemplates(CancellationToken cancellationToken)
                }
            }

-            if (diagnostics.HasAnyErrors())
+            if (CommonCompiler.HasUnsuppressedErrors(diagnostics))


CommonCompiler.HasUnsuppressedErrors(diagnostics) [](start = 16, length = 49)

Have we done filtering to get suppression state? #Closed

AlekseyTs · 2019-03-15T22:52:45Z

Done with review pass (iteration 1) #Closed

nguerrera · 2019-03-16T20:08:08Z

Something to consider here: the diagnostic actually gets persisted to error log as an "error" that is "suppressed in source". I guess this matches the compiler implementation, but my mental model (and I'll bet I'm not alone) is that a warning was raised and suppressed before it could be elevated to an error. I would not expect "suppressed error" to be an observable state in the API or in the error log.

agocke · 2019-03-18T07:04:32Z

@nguerrera That may be your mental model, but it's not really how it works. Suppression is an orthogonal boolean state that is applied or not applied. There's no temporality or ordering, it's just suppressed or not suppressed. You can't suppress diagnostics that have the default severity of error, but that's just an unrelated rule.

nguerrera · 2019-03-18T15:37:59Z

I still think the way it works is counter-intuitive, and my confidence in betting that I'm not alone was based on:

The very fact that this bug was introduced
Do not skip emit if errors are suppressed #34174 (comment)
Do not skip emit if errors are suppressed #34174 (comment)

I was further noting that since these "suppressed errors" actually show up as such in the log, there are user-facing consequences. In other words, we can't solve the confusion by choosing better method names because the confusion is not confined to the compiler implementation.

This still looks wrong to me:

"ruleId": "CS1591",
"level": "error",
"message": "Missing XML comment for publicly visible type or member 'P'",
"suppressionStates": [ "suppressedInSource" ],

agocke · 2019-03-18T17:57:54Z

@nguerrera Nope, looks right to me. The severity was error, because that was what it was elevated to, and it was suppressed in source.

nguerrera · 2019-03-18T18:12:17Z

@agocke By looks wrong, I meant it "feels" wrong. I fully understand that this is the design we have now. I just think it would be a much more intuitive design to say:

You cannot suppress errors.
Suppressed warnings are not elevated to errors by /warnaserror

Than "you cannot suppress a diagnostic with default severity of error".

agocke · 2019-03-18T18:13:52Z

If you put the code in a compilation and call GetFilteredDiagnostics, this is what you get out. It may not be the design we'd like to have gone with, but that ship has sailed.

AlekseyTs

LGTM (iteration 5)

agocke added Bug Area-Compilers labels Mar 15, 2019

agocke requested a review from a team March 15, 2019 21:04

jcouv reviewed Mar 15, 2019

View reviewed changes

src/Compilers/CSharp/Test/Emit/Emit/EndToEndTests.cs Outdated Show resolved Hide resolved

jcouv reviewed Mar 15, 2019

View reviewed changes

src/Compilers/Core/Portable/Compilation/Compilation.cs Outdated Show resolved Hide resolved

jcouv reviewed Mar 15, 2019

View reviewed changes

jcouv added this to the 16.1.P1 milestone Mar 15, 2019

jcouv approved these changes Mar 15, 2019

View reviewed changes

jcouv self-assigned this Mar 15, 2019

jaredpar reviewed Mar 15, 2019

View reviewed changes

jaredpar approved these changes Mar 15, 2019

View reviewed changes

AlekseyTs reviewed Mar 15, 2019

View reviewed changes

Use AsEnumerable with resolution

8224c6d

agocke added 3 commits March 18, 2019 11:29

Respond to PR comments

be6f079

Add a comment explaining how to use the IsUnsuppressedError method

d705714

Also adjust comment on FilterAndAppendDiagnostics

849a1cc

AlekseyTs approved these changes Mar 19, 2019

View reviewed changes

agocke merged commit fb51650 into dotnet:master Mar 19, 2019

agocke deleted the emit-warnaserror branch March 19, 2019 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not skip emit if errors are suppressed #34174

Do not skip emit if errors are suppressed #34174

agocke commented Mar 15, 2019

jcouv Mar 15, 2019 •

edited

Loading

jcouv left a comment

jaredpar Mar 15, 2019

agocke Mar 18, 2019

jaredpar Mar 15, 2019

agocke Mar 18, 2019

jaredpar left a comment

AlekseyTs Mar 15, 2019 •

edited

Loading

jcouv Mar 15, 2019

AlekseyTs Mar 15, 2019

jcouv Mar 15, 2019

agocke Mar 18, 2019

AlekseyTs Mar 15, 2019 •

edited

Loading

agocke Mar 18, 2019 •

edited

Loading

AlekseyTs Mar 15, 2019 •

edited

Loading

agocke Mar 18, 2019

agocke Mar 18, 2019

AlekseyTs Mar 18, 2019

AlekseyTs Mar 18, 2019

agocke Mar 18, 2019

AlekseyTs Mar 15, 2019 •

edited

Loading

AlekseyTs commented Mar 15, 2019 •

edited

Loading

nguerrera commented Mar 16, 2019

agocke commented Mar 18, 2019

nguerrera commented Mar 18, 2019

agocke commented Mar 18, 2019

nguerrera commented Mar 18, 2019 •

edited

Loading

agocke commented Mar 18, 2019

AlekseyTs left a comment

Do not skip emit if errors are suppressed #34174

Do not skip emit if errors are suppressed #34174

Conversation

agocke commented Mar 15, 2019

jcouv Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

AlekseyTs Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

agocke Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Mar 15, 2019 • edited Loading

nguerrera commented Mar 16, 2019

agocke commented Mar 18, 2019

nguerrera commented Mar 18, 2019

agocke commented Mar 18, 2019

nguerrera commented Mar 18, 2019 • edited Loading

agocke commented Mar 18, 2019

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv Mar 15, 2019 •

edited

Loading

AlekseyTs Mar 15, 2019 •

edited

Loading

AlekseyTs Mar 15, 2019 •

edited

Loading

agocke Mar 18, 2019 •

edited

Loading

AlekseyTs Mar 15, 2019 •

edited

Loading

AlekseyTs Mar 15, 2019 •

edited

Loading

AlekseyTs commented Mar 15, 2019 •

edited

Loading

nguerrera commented Mar 18, 2019 •

edited

Loading