[Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 #16485

davidungar · 2018-05-10T00:42:26Z

When in batch mode, swallow diagnostics for non-primary files.
rdar://40032762

davidungar · 2018-05-10T00:42:45Z

@swift-ci please smoke test

davidungar · 2018-05-10T01:10:40Z

@swift-ci please smoke test

davidungar · 2018-05-10T01:53:01Z

@swift-ci please smoke test

…ing unneeded ObjCHeader.

davidungar · 2018-05-10T02:19:51Z

@swift-ci please smoke test

davidungar · 2018-05-10T02:42:31Z

@swift-ci please smoke test

davidungar · 2018-05-10T03:50:03Z

@swift-ci please smoke test

davidungar · 2018-05-10T03:57:24Z

@swift-ci please smoke test

jrose-apple

Thanks for taking this, David! All my comments are small.

jrose-apple · 2018-05-10T15:59:21Z

include/swift/AST/DiagnosticConsumer.h

+  /// a particular consumer if diagnostic goes there.
+  ///
+  /// (For Xcode compatibility, batch mode must suppress diagnostics to
+  /// non-primaries.)


This isn't really accurate. Emitting diagnostics to non-primaries is a normal thing to do and has nothing to do with Xcode.

Did you mean "omitting diagnostics to non-primaries"? If emitting them is normal, then why are we omitting them if not for Xcode compatibility? What would be the accurate statement here?

No, emitting is correct. We are omitting them so that we don't end up with a copy in multiple serialized diagnostic files. It's true that only Xcode reads those files right now, but that's not a "compatibility" issue, and it's not really part of the design here.

Have rewritten that comment. Thanks for clarifying!

jrose-apple · 2018-05-10T16:00:28Z

lib/AST/DiagnosticConsumer.cpp

    assert(bufferID.hasValue() && "consumer registered for unknown file");
    CharSourceRange range = SM.getRangeForBuffer(bufferID.getValue());
-    ConsumersOrderedByRange.emplace_back(range, pair.second.get());
+    ConsumersOrderedByRange.emplace_back(range, pair.second ? pair.second.get()


I believe get handles this already.

Thanks! Fixed.

jrose-apple · 2018-05-10T16:02:10Z

lib/AST/DiagnosticConsumer.cpp

+  } else if (DiagnosticConsumer *c = specificConsumer.getValue())
+    c->handleDiagnostic(SM, Loc, Kind, FormatString, FormatArgs, Info);
+  else {
+    /// suppress non-primary diagnostic in batch mode


Nitpick: This should not be a doc comment, and it should be formatted like a sentence.
Extra-nitpick: I'm not sure it's officially in the style guide, but I'd prefer if all parts of an if/else if/else chain are consistent about whether they use braces!

Thank you. I have fixed those nitpicks, though I liked the other rule about using braces for > 1 line. I guess the idea is to use the disjunction: braces when either arm has > 1 line.

jrose-apple · 2018-05-10T16:02:40Z

lib/Frontend/FrontendInputsAndOutputs.cpp

+bool FrontendInputsAndOutputs::forEachInputNotProducingSupplementaryOutput(
+    llvm::function_ref<bool(const InputFile &)> fn) const {
+  // If no primary inputs, compiler is in whole-module-optimzation mode, and
+  // every input can produce supplementary outputs.


Nitpick: "contributes to the supplementary outputs", maybe? Because we still only produce one of each supplementary output for the whole job in WMO mode.

I disagree; your comment is a great catch, and not just a nitpick! I have fixed the code. Should probably add a test for this case in a subsequent PR.

jrose-apple · 2018-05-10T16:03:29Z

lib/FrontendTool/FrontendTool.cpp

+        printAsObjCIfNeeded(
+            Invocation.getObjCHeaderOutputPathForAtMostOnePrimary(),
+            Instance.getMainModule(), opts.ImplicitObjCHeaderPath,
+            moduleIsPublic);


Can you save this for a later PR? (I'm confused as to why it's needed anyway, but it's not relevant here.)

I don't see how to.
Without this fix, the test causes the compiler to falsify an assertion. If you ask for a -typecheck in batch mode, it calls getObjCHeaderOutputPathForAtMostOnePrimary, but that trips an assertion because there is >1 primary, even though no objC header has been requested.
The test could be folded into getObjCHeaderOutputPathForAtMostOnePrimary but that would be inconsistent with the other ForAtMostOnePrimary functions.

What changed that makes this come up now, though?

Good question. Will investigate after lunch.

I believe this to be a bug that has been there for a while. I ran an older version of the compiler with the following arguments: -frontend -typecheck -primary-file /s/io/1.swift -primary-file /s/io/2.swift (where each .swift file is empty) and it falsified an assertion.
It is the combination of -typecheck with > 1 primary that excites the bug.

Got it. I guess we can sneak it in here then.

jrose-apple · 2018-05-10T16:05:52Z

test/Misc/serialized-diagnostics-batch-mode-nonprimary-suppression.swift

+// NO-DIAGNOSTICS: Number of diagnostics: 0
+
+// RUN: not %FileCheck -check-prefix=ERROR %s <%t.main.txt
+// RUN: not %FileCheck -check-prefix=ERROR %s <%t.empty.txt


This time I'm going to ask that you use the CHECK-NOT strategy. If anyone adds additional ERROR check lines, this won't be testing the same thing anymore.

(although honestly the NO-DIAGNOSTICS check is probably good enough)

I don't completely understand.

Do you mean to ask for combined or separate FileCheck invocations for the positive and negative checks?

Or do you prefer simply the NO-DIAGNISTICS check?

When you write "this won't be testing the same thing anymore", I don't understand the meaning of "same".
I'm missing something.

I'm saying it's bad style to use not %FileCheck, because in general there are many ways something can fail. In this case there's only one CHECK line that's being matched, so not %FileCheck + CHECK is equivalent to %FileCheck + CHECK-NOT, but that's not generally a good thing to rely on.

Got it, thanks. Fixed.

jrose-apple · 2018-05-10T16:06:30Z

test/Misc/serialized-diagnostics-batch-mode-nonprimary-suppression.swift

+
+
+func test(x: SomeType) {
+ // CHECK-MAIN-DAG: serialized-diagnostics-batch-mode.swift:[[@LINE]]:3: error: use of unresolved identifier 'nonexistent'


This isn't actually the right error, and you're not testing these CHECK lines. Did you mean to be?

Oops! I goofed. Thanks. Check lines removed.

davidungar · 2018-05-10T17:38:33Z

@swift-ci please smoke test

davidungar · 2018-05-10T17:52:41Z

@swift-ci please smoke test

jrose-apple · 2018-05-10T20:31:41Z

test/Misc/serialized-diagnostics-batch-mode-nonprimary-suppression.swift

-
+// RUN: %FileCheck -check-prefix=NO-ERROR %s <%t.main.txt
+// RUN: %FileCheck -check-prefix=NO-ERROR %s <%t.empty.txt
+// NO-ERROR-NOT: error:


jrose-apple · 2018-05-10T20:32:25Z

include/swift/AST/DiagnosticConsumer.h

-  /// non-primaries.)
+  /// (If batch mode failed to suppress diagnostics for non-primary files, it
+  /// would end up writing each of those diagnostics in each primary file's
+  /// serialized diagnostic file.)


Sorry, I should have been clearer, because this is still describing the behavior (which is already on the line above). I don't think you should have any comment here at all, because it's not FileSpecificDiagnosticConsumer's job to decide why some diagnostics are suppressed. If you have a comment at the client side, it should note that we're relying on diagnostics located in a non-primary file already appearing when you compile that file.

Thanks, that's easy: Comment deleted.

davidungar · 2018-05-10T20:37:22Z

@swift-ci please smoke test

jrose-apple · 2018-05-10T21:02:17Z

lib/Frontend/FrontendInputsAndOutputs.cpp

+    return forEachNonPrimaryInput(fn);
+  // If no primary inputs, compiler is in whole-module-optimization mode, and
+  // only the first input can produce supplementary outputs, although all
+  // inputs may contribute.


Argh. This is what I meant by the name being wrong. We want the behavior you had before, even though it's not the complement of forEachInputProducingSupplementaryOutput, because it would be Very Bad if we dropped diagnostics from every file but the first one in WMO mode.

Fixed per off-line discussion.

…antics for forEachInputNotProducingSupplementaryOutput.

graydon

Looks ok to me!

davidungar · 2018-05-10T21:26:54Z

@swift-ci please smoke test and merge

jrose-apple · 2018-05-10T22:05:38Z

@swift-ci Please smoke test

[Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 (cherry picked from commit 46b8ad3)

davidungar requested review from graydon and jrose-apple May 10, 2018 00:43

optional pointer to consumer replaces NullDiagnosticConsumer placeholder

02fcbf7

davidungar force-pushed the oddball-diags branch from 9705db3 to 02fcbf7 Compare May 10, 2018 01:10

Check for null pointer in finishProcessing.

4f003b4

If type-checking in batch mode with no errors, don’t crash on outputt…

4a194a9

…ing unneeded ObjCHeader.

David Ungar added 2 commits May 9, 2018 19:32

Another null-pointer fix.

1e66ee6

Cleanup

c5b82b8

Testing batch-mode diagnostic suppression.

71a0a56

Add clarifying comment to forEachInputNotProducingSupplementaryOutput.

34f423f

davidungar changed the title ~~[Batch Mode] WIP on suppressing diagnostics for non-primaries~~ [Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 May 10, 2018

jrose-apple mentioned this pull request May 10, 2018

Make sure protocol witness errors don't leave the conformance context #16489

Merged

jrose-apple reviewed May 10, 2018

View reviewed changes

Addressing review comments.

aaf51f7

David Ungar added 2 commits May 10, 2018 10:43

Use CHECK-NOT style

c4f676a

FIx comment about diagnostic suppression.

2126f72

jrose-apple reviewed May 10, 2018

View reviewed changes

Deleted bogus comment.

bc51e05

jrose-apple reviewed May 10, 2018

View reviewed changes

Call forEachNonPrimaryInput directly instead of providing useless sem…

aa2f2eb

…antics for forEachInputNotProducingSupplementaryOutput.

jrose-apple approved these changes May 10, 2018

View reviewed changes

graydon approved these changes May 10, 2018

View reviewed changes

davidungar merged commit 46b8ad3 into swiftlang:master May 10, 2018

jrose-apple mentioned this pull request May 11, 2018

[Driver] -enable-batch-mode implies -continue-building-after-errors #16521

Merged

jrose-apple pushed a commit to jrose-apple/swift that referenced this pull request May 11, 2018

Merge pull request swiftlang#16485 from davidungar/oddball-diags

5cd39f4

[Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 (cherry picked from commit 46b8ad3)

jrose-apple pushed a commit to jrose-apple/swift that referenced this pull request May 11, 2018

Merge pull request swiftlang#16485 from davidungar/oddball-diags

c2ffdc9

[Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 (cherry picked from commit 46b8ad3)

This was referenced May 11, 2018

[4.2] Fixes for batch mode diagnostics in Xcode #16529

Merged

[4.2 EARLY] Fixes for batch mode diagnostics in Xcode #16530

Merged

davidungar deleted the oddball-diags branch June 14, 2019 18:03



		func test(x: SomeType) {
		// CHECK-MAIN-DAG: serialized-diagnostics-batch-mode.swift:[[@LINE]]:3: error: use of unresolved identifier 'nonexistent'

[Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 #16485

[Batch Mode] Suppressing diagnostics for non-primaries - rdar://40032762 #16485

Uh oh!

Conversation

davidungar commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar commented May 10, 2018

Uh oh!

davidungar commented May 10, 2018 •

edited

Loading