[Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors. #16526

davidungar · 2018-05-11T04:01:02Z

rdar://problem/40167848 For primary in batch mode, emit a vague error diagnostic if the batch encountered an error and that primary has no other errors of its own.

davidungar · 2018-05-11T04:01:22Z

@source-ci please smoke test

davidungar · 2018-05-11T04:02:32Z

@jrose-apple Here is the first rough cut. Not sure if it should be just one or all serialized diagnostics files that receive the vague diagnostic.

davidungar · 2018-05-11T04:36:42Z

@swift-ci please smoke test

davidungar · 2018-05-11T04:36:56Z

"@source-ci"?! I must be tired.

davidungar · 2018-05-11T17:13:47Z

@swift-ci please smoke test

davidungar · 2018-05-11T17:17:37Z

@jrose-apple I think that this works, so the next step is probably for you to review. I would attach a radar number, but I'm not aware of one.

jrose-apple

Thanks for starting on this. I think you've solved a different problem, though (see comments).

By the way, "fatal error" already means something in our diagnostics system: an error where there's no point even emitting more diagnostics, because things are irrevocably broken. These are just normal errors we're talking about in this change.

jrose-apple · 2018-05-11T17:48:15Z

include/swift/AST/DiagnosticsFrontend.def

@@ -235,6 +235,8 @@ WARNING(cannot_assign_value_to_conditional_compilation_flag,none,
 ERROR(error_optimization_remark_pattern, none, "%0 in '%1'",
      (StringRef, StringRef))

+ERROR(error_compilation_failed,none, "compilation failed", ())


This is too ambiguous. We need to say something about there being errors in other files.

Changed to a strawman: "some error occured in a file that was used in this one". Suggestions for a better message would be most welcome.

It's not necessarily true that the file was used in this one. This error comes up if any file in the batch resulted in an error in any file not in the batch.

jrose-apple · 2018-05-11T17:49:40Z

include/swift/AST/DiagnosticConsumer.h

@@ -101,6 +101,8 @@ class DiagnosticConsumer {

  /// \returns true if an error occurred while finishing-up.
  virtual bool finishProcessing() { return false; }
+
+  virtual bool hadOnlySuppressedFatalErrors() const { return false; }


I'd really rather not change DiagnosticConsumer. Suppression is a feature of FileSpecificDiagnosticConsumer, not the diagnostics system in general.

jrose-apple · 2018-05-11T17:50:43Z

lib/AST/DiagnosticConsumer.cpp

+}
+
+bool FileSpecificDiagnosticConsumer::hadOnlySuppressedFatalErrors() const {
+  return WasAFatalDiagnosticSuppressed && !WasAFatalDiagnosticEmitted;


This condition isn't correct, either. We want to emit the generic error if any file did not receive any errors, not if all of them did not receive any errors.

jrose-apple · 2018-05-11T17:57:35Z

Filed rdar://problem/40167848 for you.

davidungar · 2018-05-11T22:06:20Z

Thanks a heap!

…

On May 11, 2018, at 10:57 AM, Jordan Rose ***@***.***> wrote: Filed rdar://problem/40167848 for you. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#16526 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAcJ0NT7wu35s37kPQbqdLhCKexrU0Sfks5txdEXgaJpZM4T63vo>.

davidungar · 2018-05-13T03:55:20Z

@swift-ci please smoke test

davidungar · 2018-05-13T04:02:37Z

@jrose-apple To satisfy the other constraints, I had to add a parameter to finishProcessing, the SourceManager. That's because, in order to insert the non-specific error, the source manager must be passed down. It does work as desired, I think. Needs polishing, comments, etc.

davidungar · 2018-05-13T14:55:10Z

And there’s a const cast, sigh.

jrose-apple · 2018-05-13T23:23:28Z

include/swift/AST/DiagnosticsFrontend.def

@@ -235,6 +235,8 @@ WARNING(cannot_assign_value_to_conditional_compilation_flag,none,
 ERROR(error_optimization_remark_pattern, none, "%0 in '%1'",
      (StringRef, StringRef))

+ERROR(error_some_error_occured_in_a_file_that_was_used_by_this_one,none, "some error occured in a file that was used by this one", ())


Sorry, this still isn't the correct description. All we know is that an error occurred somewhere—maybe by another file in this batch, maybe in a file used by this file, maybe in a file used by another file in the batch. That is, because dia files are per-input in batch mode, we need this error even if no diagnostics are suppressed.

Fixed (I think).

jrose-apple · 2018-05-13T23:27:31Z

include/swift/AST/DiagnosticConsumer.h

+  Optional<ConsumerSpecificInformation *>
+      ConsumerSpecificInfoForSubsequentNotes = None;
+
+  bool WasAnErrorSuppressed = false;


This shouldn't be necessary.

Removed, but I did had a flag hasAnErrorBeenConsumed.

jrose-apple · 2018-05-13T23:30:36Z

include/swift/AST/DiagnosticConsumer.h

-    std::pair<CharSourceRange, DiagnosticConsumer *>;
+  // The commented-out consts are there because the data does not change
+  // but the swap method gets called on this structure.
+  struct ConsumerSpecificInformation {


One consequence of putting the flag in with the mapping like this is that the same consumer might get multiple copies of the dummy error, one for each range (file) it's covering. We're not reusing consumers that way, so it doesn't matter, but I thought I'd bring it up and see what you thought.

(I was originally thinking it'd be a SmallPtrSet<DiagnosticConsumer *, N> that tracked all of the consumers that actually received non-note diagnostics. That would avoid the const_cast, but at the practical, run-time cost of an extra lookup.)

I like the separation of concerns in your scheme; I never thought about reusing the consumers. I would rather put the flag in the consumer, and let a diagnostic consumer know if it had seen an error. But I got the impression that you disliked adding that functionality to the general consumer & I can see that. For now, I would prefer sticking with the current scheme.

jrose-apple · 2018-05-13T23:39:15Z

include/swift/AST/DiagnosticConsumer.h


 private:
+  void addNonSpecificErrors(SourceManager &SM);


Nitpick: this needs either a doc comment or a better name (or both).

jrose-apple · 2018-05-13T23:46:59Z

lib/AST/DiagnosticConsumer.cpp

+  DiagnosticInfo Info;
+  Info.ID = diagnostic.getID();
+  Info.Ranges = diagnostic.getRanges();
+  Info.FixIts = diagnostic.getFixIts();


We know there are no ranges or fix-its here, so it's probably better just to omit this.

jrose-apple · 2018-05-13T23:49:49Z

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

-// Ensure there was an error:
+// RUN: %FileCheck -check-prefix=NO-COMPILATION-FAILED %s <%t.main.txt
+// RUN: %FileCheck -check-prefix=NO-COMPILATION-FAILED %s <%t.empty.txt
+// NO-COMPILATION-FAILED-NOT: compilation failed


You changed the text of the error, so this isn't up to date anymore. (A hazard of negative checks!)

Thanks! Fixed.

jrose-apple · 2018-05-13T23:50:00Z

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


 func test(x: SomeType) {
+  nonexistant(); // create a fatal error here


Just a regular error!

jrose-apple · 2018-05-13T23:50:48Z

test/Misc/serialized-diagnostics-batch-mode-nonspecific-error.swift

@@ -0,0 +1,26 @@
+// To avoid redundant diagnostics showing up in Xcode, batch-mode must suppress diagnostics in
+// non-primary files.
+// But if the only  errors are suppressed ones, a nonspecific error must be emitted


As mentioned above, I think I was misleading about "suppression". This is something important even for errors that do get emitted, just for another file in the batch.

Changing the verbiage.

davidungar · 2018-05-14T01:50:58Z

@swift-ci please smoke test

davidungar · 2018-05-14T01:55:51Z

@swift-ci please clean smoke test linux platform

davidungar · 2018-05-14T01:57:35Z

@swift-ci please clean smoke test linux platform

davidungar · 2018-05-14T01:58:26Z

@jrose-apple OK, I think I've made the needed fixes and pushed.

davidungar · 2018-05-14T02:04:16Z

@swift-ci please smoke test linux platform

jrose-apple

Thanks, I think this is the logic I was thinking of! Did you get a chance to try it in Xcode with a real error?

jrose-apple · 2018-05-14T03:45:06Z

lib/AST/DiagnosticConsumer.cpp

+    if (!info.hasAnErrorBeenEmitted && info.consumer) {
+      produceNonSpecificError(info.consumer, SM);
+      info.hasAnErrorBeenEmitted = true;
+      HasAnErrorBeenConsumed = true;


This line should be unnecessary since HasAnErrorBeenConsumed is guarding this section.

Thanks. I must be losing focus.

jrose-apple · 2018-05-14T03:47:30Z

include/swift/AST/DiagnosticsFrontend.def

@@ -235,6 +235,8 @@ WARNING(cannot_assign_value_to_conditional_compilation_flag,none,
 ERROR(error_optimization_remark_pattern, none, "%0 in '%1'",
      (StringRef, StringRef))

+ERROR(error_an_error_occurred,none, "an error occurred", ())


This is still too vague; imagine how it would look to a user. I still think my phrasing is reasonable, something like "compilation stopped due to errors in other files".

How about "error(s) in other file(s) halted compilation"?

I feel like the funny pluralization isn't buying us anything, and it also seems weird to me to use active tense for this. My mental model is that the compiler chose to stop compiling this file because of errors in that file. But maybe others don't share that view.

I liked the ambiguous pluralization because it was more accurate. Could be one, could be many errors. But in the interest of time, I can live with the plural. I'll passivate the message, too.

jrose-apple · 2018-05-14T03:48:35Z

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

@@ -0,0 +1,20 @@
+// Batch-mode has no good place to emit diagnostics that occur in non-primary files
+// because then cannot be locatlized to a particular primary's serialized diagnostics file.


Typo: "locatlized" (and "then"). That's also not a great word because it makes me think of the internationalization localization. But the description is no longer accurate anyway.

Fixed. I'm getting terser as the evening wears on.

jrose-apple · 2018-05-14T03:49:05Z

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

+//
+// RUN: rm -f %t.*
+
+// RUN: not %target-swift-frontend -typecheck  -primary-file %s  -serialize-diagnostics-path %t.main.dia -primary-file %S/../Inputs/empty.swift  -serialize-diagnostics-path %t.empty.dia %S/Inputs/serialized-diagnostics-batch-mode-suppression-helper.swift  2> %t.stderr.txt


This is no longer testing non-primary errors…but it still should! We need both tests.

Yes, now testing both.

davidungar · 2018-05-14T05:16:57Z

@swift-ci please smoke test

davidungar · 2018-05-14T16:23:09Z

@swift-ci please smoke test

jrose-apple · 2018-05-14T17:05:49Z

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

+
+// RUN: %FileCheck -check-prefix=NO-GENERAL-ERROR-OCCURRED %s <%t.main.txt
+// RUN: %FileCheck -check-prefix=GENERAL-ERROR-OCCURRED %s <%t.empty.txt
+// GENERAL-ERROR-OCCURRED: compilation stopped by errors in other files


For this to really be testing that the non-primary is causing the message, you'll have to not have an error in either primary.

jrose-apple · 2018-05-14T17:06:49Z

lib/AST/DiagnosticConsumer.cpp

+      diag::error_compilation_stopped_by_errors_in_other_files);
+
+  // Stolen from DiagnosticEngine::emitDiagnostic
+  DiagnosticInfo Info;


Nitpick: You have two things named "Info" now. (That said, this function could get away with just taking a DiagnosticConsumer &.)

[Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors.

…rror-swift-4.2-4-30-2018-branch [Batch mode] <rdar://40167848> Merge pull request #16526 from davidungar/compilation-failed

davidungar requested a review from jrose-apple May 11, 2018 04:01

davidungar changed the title ~~[Batch Mode] WIP emit fatal diag if all fatals suppressed~~ [Batch Mode] Emit fatal diag if all fatals suppressed. May 11, 2018

jrose-apple suggested changes May 11, 2018

View reviewed changes

davidungar changed the title ~~[Batch Mode] Emit fatal diag if all fatals suppressed.~~ [Batch Mode] Emit error diagnostic for a primary file that has errors only in non-primaries. May 11, 2018

David Ungar added 4 commits May 11, 2018 15:14

Emit error diagnostic for a primary if all errors suppressed

70aecfe

WIP backup

8baef6a

Pass SM in finishProcessing

524d88e

Track errors

b4dce82

davidungar force-pushed the compilation-failed branch from fdea616 to b4dce82 Compare May 13, 2018 03:54

jrose-apple suggested changes May 13, 2018

View reviewed changes

FIx functionality & address other concerns.

5206ce0

davidungar changed the title ~~[Batch Mode] Emit error diagnostic for a primary file that has errors only in non-primaries.~~ [Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors. May 14, 2018

jrose-apple reviewed May 14, 2018

View reviewed changes

Zeroing in…

c315bdd

David Ungar added 3 commits May 14, 2018 08:35

Set the locations of the errors.

f9e9a73

Adjust the error text.

e0ed336

Fix error message in tests

167c8dc

jrose-apple reviewed May 14, 2018

View reviewed changes

jrose-apple approved these changes May 14, 2018

View reviewed changes

davidungar merged commit 9d08b8c into swiftlang:master May 14, 2018

davidungar pushed a commit to davidungar/swift that referenced this pull request May 14, 2018

Merge pull request swiftlang#16526 from davidungar/compilation-failed

617b994

[Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors.

davidungar pushed a commit to davidungar/swift that referenced this pull request May 14, 2018

Merge pull request swiftlang#16526 from davidungar/compilation-failed

4946382

[Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors.

davidungar pushed a commit that referenced this pull request May 14, 2018

Merge pull request #16596 from davidungar/rdar-40167848-nonspecific-e…

d23e583

…rror-swift-4.2-4-30-2018-branch [Batch mode] <rdar://40167848> Merge pull request #16526 from davidungar/compilation-failed

davidungar deleted the compilation-failed branch June 14, 2019 18:03


		func test(x: SomeType) {
		nonexistant(); // create a fatal error here

		@@ -0,0 +1,20 @@
		// Batch-mode has no good place to emit diagnostics that occur in non-primary files
		// because then cannot be locatlized to a particular primary's serialized diagnostics file.

[Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors. #16526

[Batch Mode] Emit a non-specific error for all primaries lacking specific errors when a batch has errors. #16526

Uh oh!

Conversation

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

Uh oh!

Uh oh!

davidungar commented May 11, 2018

Uh oh!

davidungar commented May 11, 2018

Uh oh!

davidungar commented May 11, 2018

Uh oh!

davidungar commented May 11, 2018

Uh oh!

davidungar commented May 11, 2018

Uh oh!

davidungar commented May 11, 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!

jrose-apple commented May 11, 2018

Uh oh!

davidungar commented May 11, 2018 via email

Uh oh!

davidungar commented May 13, 2018

Uh oh!

davidungar commented May 13, 2018

Uh oh!

davidungar commented May 13, 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!

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 14, 2018

Uh oh!

davidungar commented May 14, 2018

Uh oh!

davidungar commented May 14, 2018

Uh oh!

davidungar commented May 14, 2018

Uh oh!

davidungar commented May 14, 2018

Uh oh!

davidungar commented May 11, 2018 •

edited

Loading