Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not skip emit if errors are suppressed #34174

Merged
merged 5 commits into from
Mar 19, 2019
Merged

Do not skip emit if errors are suppressed #34174

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22f872d
Do not skip emit if errors are suppressed
agocke Mar 15, 2019
8224c6d
Use AsEnumerable with resolution
agocke Mar 18, 2019
be6f079
Respond to PR comments
agocke Mar 18, 2019
d705714
Add a comment explaining how to use the IsUnsuppressedError method
agocke Mar 18, 2019
849a1cc
Also adjust comment on FilterAndAppendDiagnostics
agocke Mar 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the windows path here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[WorkItem(34101, "https://github.com/dotnet/roslyn/issues/34101")]
public void SuppressedWarnAsErrorsStillEmit()
{
var dir = Temp.CreateDirectory();
var src = dir.CreateFile("temp.cs").WriteAllText(@"
#pragma warning disable 1591

public class P {
public static void Main() {}
}");
const string docName = "doc.xml";

var cmd = CreateCSharpCompiler(null, dir.Path, new[] { "/nologo", "/errorlog:errorlog", $"/doc:{docName}", "/warnaserror", src.Path });

var outWriter = new StringWriter(CultureInfo.InvariantCulture);
var exitCode = cmd.Run(outWriter);
Assert.Equal(0, exitCode);
Assert.Equal("", outWriter.ToString());

string exePath = Path.Combine(dir.Path, "temp.exe");
Assert.True(File.Exists(exePath));
var result = ProcessUtilities.Run(exePath, arguments: "");
Assert.Equal(0, result.ExitCode);
}

[ConditionalFact(typeof(WindowsDesktopOnly), Reason = ConditionalSkipReason.TestExecutionNeedsWindowsTypes)]
public void XmlMemoryMapped()
{
Expand Down
17 changes: 17 additions & 0 deletions src/Compilers/CSharp/Test/Emit/Emit/EndToEndTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,23 @@ private static void RunInThread(Action action)
}
}

[Fact]
public void EmitWithSuppressedWarnAsError()
agocke marked this conversation as resolved.
Show resolved Hide resolved
{
var src = @"
#pragma warning disable 1591

public class P {
public static void Main() {}
}";
var parseOptions = TestOptions.RegularWithDocumentationComments;
var options = TestOptions.ReleaseDll
.WithXmlReferenceResolver(XmlFileResolver.Default)
.WithGeneralDiagnosticOption(ReportDiagnostic.Error);

var verifier = CompileAndVerify(src, parseOptions: parseOptions, options: options);
}

// This test is a canary attempting to make sure that we don't regress the # of fluent calls that
// the compiler can handle.
[WorkItem(16669, "https://github.com/dotnet/roslyn/issues/16669")]
Expand Down
22 changes: 17 additions & 5 deletions src/Compilers/Core/Portable/CommandLine/CommonCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AlekseyTs AlekseyTs Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlekseyTs Did this address your concern?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{
foreach (var diag in diagnostics.AsEnumerableWithoutResolution())
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
if (IsReportedError(diag))
{
return true;
}
}
return false;
}

/// <summary>
/// Returns true if the diagnostic is an error that should be reported.
/// </summary>
Expand Down Expand Up @@ -648,7 +660,7 @@ private void CompileAndEmit(

// Print the diagnostics produced during the parsing stage and exit if there were any errors.
compilation.GetDiagnostics(CompilationStage.Parse, includeEarlierStages: false, diagnostics, cancellationToken);
if (diagnostics.HasAnyErrors())
if (HasUnsuppressedErrors(diagnostics))
{
return;
}
Expand All @@ -674,7 +686,7 @@ private void CompileAndEmit(
}

compilation.GetDiagnostics(CompilationStage.Declare, includeEarlierStages: false, diagnostics, cancellationToken);
if (diagnostics.HasAnyErrors())
if (HasUnsuppressedErrors(diagnostics))
{
return;
}
Expand Down Expand Up @@ -786,7 +798,7 @@ private void CompileAndEmit(
{
using (var win32ResourceStreamOpt = GetWin32Resources(MessageProvider, Arguments, compilation, diagnostics))
{
if (diagnostics.HasAnyErrors())
if (HasUnsuppressedErrors(diagnostics))
{
return;
}
Expand Down Expand Up @@ -882,7 +894,7 @@ private void CompileAndEmit(
}
}

if (diagnostics.HasAnyErrors())
if (HasUnsuppressedErrors(diagnostics))
{
return;
}
Expand All @@ -902,7 +914,7 @@ private void CompileAndEmit(
if (analyzerExceptionDiagnostics != null)
{
diagnostics.AddRange(analyzerExceptionDiagnostics);
if (analyzerExceptionDiagnostics.HasAnyErrors())
if (HasUnsuppressedErrors(analyzerExceptionDiagnostics))
{
return;
}
Expand Down
7 changes: 5 additions & 2 deletions src/Compilers/Core/Portable/Compilation/Compilation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Collections.Immutable;
using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.Contracts;
using System.IO;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -1370,13 +1371,15 @@ internal bool FilterAndAppendDiagnostics(DiagnosticBag accumulator, IEnumerable<
continue;
}


var filtered = Options.FilterDiagnostic(d);
agocke marked this conversation as resolved.
Show resolved Hide resolved
if (filtered == null ||
(!reportSuppressedDiagnostics && filtered.IsSuppressed))
{
continue;
}
else if (filtered.Severity == DiagnosticSeverity.Error)
else if (filtered.Severity == DiagnosticSeverity.Error &&
!filtered.IsSuppressed)
Copy link
Member

@jcouv jcouv Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!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

Copy link
Contributor

@AlekseyTs AlekseyTs Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

@agocke agocke Mar 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
hasError = true;
}
Expand Down Expand Up @@ -2578,7 +2581,7 @@ internal CommonPEModuleBuilder CheckOptionsAndCreateModuleBuilder(
}
}

if (diagnostics.HasAnyErrors())
if (CommonCompiler.HasUnsuppressedErrors(diagnostics))
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Have we done filtering to get suppression state? #Closed

{
return null;
}
Expand Down
24 changes: 24 additions & 0 deletions src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,30 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CommandLine.UnitTests
Return VisualBasicCommandLineParser.Script.Parse(args, baseDirectory, sdkDirectory, additionalReferenceDirectories)
End Function

<Fact>
<WorkItem(34101, "https://github.com/dotnet/roslyn/issues/34101")>
Public Sub SuppressedWarnAsErrorsStillEmit()
Dim dir = Temp.CreateDirectory()
Dim src = dir.CreateFile("temp.vb").WriteAllText("
#Disable Warning BC42302
Module Module1
Sub Main()
Dim x = 42 ''' <test />
End Sub
End Module")
Const docName As String = "doc.xml"

Dim cmd = New MockVisualBasicCompiler(Nothing, dir.Path, {"/nologo", "/errorlog:errorlog", $"/doc:{docName}", "/warnaserror", src.Path})

Dim outWriter = New StringWriter(CultureInfo.InvariantCulture)
Dim exitCode = cmd.Run(outWriter)
Assert.Equal("", outWriter.ToString())
Assert.Equal(0, exitCode)

Dim exePath = Path.Combine(dir.Path, "temp.exe")
Assert.True(File.Exists(exePath))
End Sub

<Fact>
Public Sub XmlMemoryMapped()
Dim dir = Temp.CreateDirectory()
Expand Down