[Batch mode] Cope with bugs that cause error suppression. #23735

davidungar · 2019-04-02T07:52:32Z

In batch mode, any diagnostics located in non-primary files are suppressed, with the expectation that they will surface when the containing file is compiled as primary. Although this assumption should hold, it does not always. For example, certain inputs can interact with associated type inferencing to violate it.

This PR tracks the current primary input in the DiagnosticEngine, which passed it down to the DiagnosticConsumer. The FileSpecificDiagnosticConsumer uses this information to output the diagnostic into the appropriate primary's .dia file. It addresses rdar://49303574.

Needs apple/swift-lldb#1431

…agnostic. Unformmated.

davidungar · 2019-04-02T07:53:26Z

@swift-ci please smoke test os x platform

davidungar · 2019-04-02T16:18:48Z

@swift-ci please smoke test os x platform

jrose-apple

This can't work as is. We emit diagnostics when we first see a problem, but we might first see a problem for declaration X1 in file X because we're trying to use X1 in file Y. Putting that diagnostic in Y's .dia file isn't correct because it won't always be Y that first tries to use X1.

davidungar · 2019-04-02T17:41:16Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

davidungar · 2019-04-02T19:36:36Z

This can't work as is. We emit diagnostics when we first see a problem, but we might first see a problem for declaration X1 in file X because we're trying to use X1 in file Y. Putting that diagnostic in Y's .dia file isn't correct because it won't always be Y that first tries to use X1.

Thanks! But I don't understand. Isn't this PR doing the same thing as in the pre-batch-mode, single-file case? That's what it seems to do in non-batch mode.

Also, what is the significance of "first"? Why is that important?

David and I talked in person (well, over live video) and he convinced me that 1. We won't lose diagnostics this way even if they're not always ascribed to the "right" file due to other bugs; they may move from file to file but won't disappear. 2. In a particularly contrived set of dependencies we might end up with warnings that persist in incremental builds even if you fix the issue, but (a) those would be fixed by clean builds, (b) it's not clear that that contrived case can even be built today, and (c) that's still better than not seeing diagnostics that are present. 3. We already do this when we compile a single file, because that looks the same as "no batch mode" and so we don't try to filter diagnostics in any special way. So I'm taking another look.

jrose-apple · 2019-04-03T03:12:47Z

include/swift/AST/DiagnosticConsumer.h

                                ArrayRef<DiagnosticArgument> FormatArgs,
-                                const DiagnosticInfo &Info) = 0;
+                                const DiagnosticInfo &Info,
+                                StringRef currentPrimaryInput) = 0;


Hm. I feel really weird having a DiagnosticConsumer know about "primary inputs". DiagnosticConsumer is a general interface, and not everything we do has a particular primary file associated with it. Moreover, "primary files" are a Frontend concept, and DiagnosticConsumer is down in AST, so this smells like a conceptual layering violation to me. (Also, I can't remember if whole-module mode uses "primary files" at all.)

An alternate idea I had was to use a SourceLoc as the "current" token, which for now could be the start of the "current" file. I still don't really know what to call it, though.

Thank you for your clear summary of our talk.

I like your thinking about the layering. If we pass in a SourceLoc, we could call it the diagnosticDestinationOfLastResort. Or the finalRestingPlace.

OTOH, it might seem unintuitive to use a SourceLoc as a stand-in for the .dia file. Here's another idea: we could pass in the .dia file of the current primary, but then FileSpecificDiagnosticConsumer would need another (unordered) map. What do you think? Any other ideas?

Another thought, we could call the thing that's passed in to the DiagnosticEngine, by it filename or location, the diagnosticOrphanage, since it's where homeless ones go. diagnosticMotel? (Diagnostics check in but they don't check out.) "Refuge"? I hope all these suggestions will inspire you to think of something really good!

Let me try it with a location and see how it looks

I really want to keep knowledge of FileSpecificDiagnosticConsumer out of DiagnosticEngine as much as possible, mostly to preserve separation of concerns. It already feels weird to me to have it track which file is currently being processed.

fallbackBufferLocation? (I put "buffer" in there to make it clear that the exact SourceLoc isn't relevant.) activeBuffer? bufferIndirectlyResponsibleForDiagnostic?

These are good! I'll ponder. Thanks!

Another design alternative would be to not store the fallbackBufferLocation in the DiagnosticEngine, but for it to merely forward the set method to all consumers. The FileSpecificDiagnosticConsumer would be the only one to store it. How does that alternative feel to you?

jrose-apple · 2019-04-03T03:14:50Z

lib/AST/DiagnosticConsumer.cpp

-  Optional<FileSpecificDiagnosticConsumer::Subconsumer *> subconsumer;
+  auto subconsumer = findSubconsumerAndRememberItForNotes(SM, Loc, Kind);
+  if (subconsumer.hasValue() && !(*subconsumer)->getConsumer())
+    subconsumer = findSubconsumerForPrimaryCausingErrorInNonprimary(


I think you need to remember-for-notes in this case too, so I suggest just pushing this into your new findSubconsumerAndRememberItForNotes, or even subconsumerForLocation.

I agree, and I was under the impression that the code in this PR does that. Specifically,findSubconsumerForPrimaryCausingErrorInNonprimary returns through findSubconsumerAndRememberItForNotes. I see why it was misleading, though I don't have a better alternative.

If you think it's clearer, findSubconsumerForPrimaryCausingErrorInNonprimary could return a possibly-invalid SourceLoc. And the call to findSubconsumerAndRememberItForNotes could then be moved up into handleDiagnostic.

Hmm... I have an idea for a better refactoring. Will try it out.

Oh, whoops. I read the code and didn't take that in at all because of the name.

But your reaction was good because it led to clearer code!

jrose-apple

Thanks, this factoring of the consumer-finding makes more sense to me too.

jrose-apple · 2019-04-03T16:48:40Z

lib/AST/DiagnosticConsumer.cpp

+}
+
+Optional<FileSpecificDiagnosticConsumer::Subconsumer *>
+FileSpecificDiagnosticConsumer::findSubconsumerForAnyKind(


I'd just drop the "ForAnyKind" here. This is the general entry point for finding a consumer; the other one specifically calls out the "non-note"-ness. I guess it is edging a little close to the lower-level subconsumerForLocation, but the longer name isn't addressing that part anyway.

Sure! I tried thinking of a name including "AndMakingSureThatNotesWouldBeInTheSamePlaceAsDiagnostics" but I couldn't find anything I liked.

davidungar · 2019-04-03T16:53:20Z

Glad the new factoring works. I have a bug to track down, then I'll rerun the tests.

No more vacuous subconsumers. Output into active primary or everywhere.

davidungar · 2019-04-03T18:41:03Z

The new refactoring led to a more complete change for how we handle non-primaries. I've pushed that so you can see it. Still plan to rename defaultDiagnosticLoc to one of your suggestions.

davidungar · 2019-04-03T18:41:42Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

davidungar · 2019-04-03T19:54:28Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

davidungar · 2019-04-03T20:02:49Z

@jrose-apple I've finished the changes we spoke of, and also they led to a redoing on the subconsumer invariants. (Consumer cannot not be null anymore.) Check it out at your convenience and let me know. (Thanks for the great name!)

Also, I'm starting to look at the testing aspect. I want to include a test before landing this.

Thanks.

davidungar · 2019-04-04T02:51:28Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

davidungar · 2019-04-04T06:24:53Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

jrose-apple

Looks reasonable. It needs a new test, though, even if it's a new case in unittests/ that's contrived.

jrose-apple · 2019-04-04T16:12:10Z

include/swift/AST/DiagnosticConsumer.h

+                   StringRef FormatString,
+                   ArrayRef<DiagnosticArgument> FormatArgs,
+                   const DiagnosticInfo &Info,
+                   SourceLoc bufferIndirectlyCausingDiagnostic) = 0;


Can you document the parameter too? It's pretty subtle for someone writing their own DiagnosticConsumer. (It may even be worth mentioning that it's safe to ignore if your consumer doesn't do anything special with different files.)

You could mention setBufferIndirectlyCausingDiagnosticToInput too.

Good idea! Done, but a little verbosely.

jrose-apple · 2019-04-04T16:13:17Z

include/swift/AST/DiagnosticEngine.h

+    void setBufferIndirectlyCausingDiagnosticToInput(
+        StringRef defaultDiagnosticInputFile);
+    void resetBufferIndirectlyCausingDiagnostic();
+    SourceLoc getDefaultDiagnostLoc() const {


Typo: "Diagnost".

Thanks!! Fixing it.

jrose-apple · 2019-04-04T16:13:33Z

include/swift/AST/DiagnosticEngine.h

+    /// a non-primary file, use this affordance to place it in the .dia
+    /// file for the primary that is currently being worked on.
+    void setBufferIndirectlyCausingDiagnosticToInput(
+        StringRef defaultDiagnosticInputFile);


Hm, why make the DiagnosticEngine do the string lookup? It feels to me like DiagnosticEngine API ought to traffic in SourceLocs.

The alternatives that I see are:

Expose the DiagnosticEngine::SourcMgr instance variable (via a get method?) so that a method in the RAII struct can use it do to the string lookup.

Do the lookup in every caller of the RAII class (two, so far).

Pass in a SourceManager to the RAII constructor.

Which, if any, are better than what's there now?

I was thinking # 2 is the best because we probably don't have to do a string lookup in all cases. In particular, SourceFile::getBufferID is available in all cases, right?

(I guess we could make this interface take a buffer ID too, but I don't like that either because it doesn't have a strong type today, and it's also not a type we traffic in in many parts of the compiler. The other advantage of a SourceLoc is that we could some day use it for something more specific.)

I tried a couple of alternatives. As a result, I quite dislike #2 because we end up with too many lines (for my taste) of duplicated code. Rather, prefer moving the functionality in question to the RAII object, which is sort of a go-between anyway. Also, following your suggestion, the id is obtained without using the filename. So the RAII object gets a SourceFile, translates that to a SourceLoc, and passes that in to the DiagnosticEngine. This layering feels right to me. If you think it best, the RAII definitions and declarations can be moved out of DiagnosticEngine.*. Just tell me where to put them & I'm happy to do it.

jrose-apple · 2019-04-04T16:14:30Z

lib/AST/DiagnosticConsumer.cpp

@@ -118,6 +118,11 @@ FileSpecificDiagnosticConsumer::subconsumerForLocation(SourceManager &SM,
  if (loc.isInvalid())
    return None;

+  // What if a there's a consumer for fixits but there is no fixits output path?


Nitpick: "fix-its". Also, I don't understand the point here—what makes fix-it consumers different from serializing consumers?

Tx for the nitpick. I've clarified the comment, hope it works better now.

jrose-apple · 2019-04-04T16:16:43Z

lib/FrontendTool/FrontendTool.cpp

@@ -1218,6 +1219,9 @@ static bool performCompileStepsPostSILGen(
  SILOptions &SILOpts = Invocation.getSILOptions();
  IRGenOptions &IRGenOpts = Invocation.getIRGenOptions();

+  BufferIndirectlyCausingDiagnosticRAII cpi(Context.Diags,
+                                            PSPs.MainInputFilenameForDebugInfo);


Eek. Now it's no longer "for debug info". I don't think this is safe to use because the name for debug info may not be the same as the name of the buffer.

Thanks--great catch! I switched it to use the MSF when it's a source file. Does that look right?

jrose-apple · 2019-04-04T16:17:20Z

lib/FrontendTool/FrontendTool.cpp

  // multiple times. So, create a diagnostic "eater" for those non-primary
  // files.
+  //
+  // This routine gets called for \c createJSONFixItDiagnosticConsumerIfNeeded


This seems overly specific. "This routine gets called in cases where…" seems fine to me.

jrose-apple · 2019-04-04T23:24:37Z

lib/FrontendTool/FrontendTool.cpp

@@ -1218,6 +1219,11 @@ static bool performCompileStepsPostSILGen(
  SILOptions &SILOpts = Invocation.getSILOptions();
  IRGenOptions &IRGenOpts = Invocation.getIRGenOptions();

+  BufferIndirectlyCausingDiagnosticRAII ricd(


In case it's useful, here's the idiom for an RAII you may or may not want:

Optional<BufferIndirectlyCausingDiagnosticRAII> bicd; if (condition) bicd.emplace(Context.Diags, "foo");

Thanks, that's better. I 'll use it.

davidungar · 2019-04-04T23:44:44Z

Regarding testing, see https://github.com/apple/swift/pull/23735/files#diff-43eb268d973af29cc397746d4ed96b7a , which shows a modified test that is part of this PR. The test fails with the status quo and passes with this fix. Is that enough?

davidungar · 2019-04-05T05:43:53Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

jrose-apple

Implementation looks good. :-) All remaining comments are polish.

jrose-apple · 2019-04-05T16:36:38Z

lib/AST/DiagnosticConsumer.cpp

@@ -118,6 +118,12 @@ FileSpecificDiagnosticConsumer::subconsumerForLocation(SourceManager &SM,
  if (loc.isInvalid())
    return None;

+  // What if a there's a FileSpecificDiagnosticConsumer but there are no
+  // subconsumers in it? (This situation obtains for the fix-its


Typo: "obtains" instead of "occurs".

I meant "obtains", but "occurs" is better. Fixed, tx.

jrose-apple · 2019-04-05T16:38:08Z

lib/AST/DiagnosticEngine.cpp

+    : Diags(SF.getASTContext().Diags) {
+  auto id = SF.getBufferID();
+  if (!id)
+    return;


Nitpick: This won't come up in practice because you only modify the setting using RAII, but otherwise: should it return? Or should it reset the buffer instead?

Yes, it was reset in an earlier draft. Hmm... This question feels like it illuminates a flaw: We don't account for nested RAII's. Rather than implement a push-down stack for the buffer, let me try just outlawing such nesting for today with an assertion. That will at least detect the occurrence of such a situation.

jrose-apple · 2019-04-05T16:42:11Z

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

@@ -1,18 +0,0 @@
-// Ensure that an error in a primary causes an error in the errorless primary.


This test is still relevant, isn't it?

I see, it is partly still relevant because it tests an erroneous primary. I'll modify it and put it back. Thanks for the catch! (Although we may have other tests that cover this.)

jrose-apple · 2019-04-05T16:42:25Z

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

@@ -1,16 +0,0 @@
-// Ensure that an error in a non-primary causes an error in the errorless primary.


I can't tell if this one is still relevant or not.

I don't think it is still needed because it checks for:

The absence of "compilation stopped by errors in other files", which is a string which is not in the compiler at all anymore, AFAICT.

The empty.dia file existing and being empty, but this same check is in the remaining test, serialized-diagnostics-batch-mode-nonprimary-errors-only.swift.

Summary: one predicate is meaningless today because the code is no longer in the compiler, and the other is covered by the remaining test.

I guess I feel like it's still a slightly different configuration even if it doesn't test any new code paths. But I'll defer to you on this.

Thanks. You have a point, but I think I'm confident that this test doesn't carry it's weight.

davidungar · 2019-04-05T17:46:07Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test linux platform

davidungar · 2019-04-05T18:15:43Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

davidungar · 2019-04-05T18:40:52Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test linux platform

davidungar · 2019-04-05T19:08:38Z

PS: Thanks for that "Implementation looks good"!

davidungar · 2019-04-05T20:33:52Z

Please test with following pull request:
apple/swift-lldb#1431

@swift-ci please smoke test os x platform

[Batch mode] Cope with bugs that cause error suppression.

…sion-fix-swift-5.1-branch [Batch mode] Merge pull request #23735 -rdar-40167351

[Batch mode] Cope with bugs that cause error suppression. # Conflicts: # include/swift/AST/DiagnosticConsumer.h # lib/FrontendTool/FrontendTool.cpp # tools/libSwiftSyntaxParser/libSwiftSyntaxParser.cpp

David Ungar added 2 commits April 2, 2019 00:43

Store current primary in diagnostic engine, pass it down via handleDi…

7a0e0ff

…agnostic. Unformmated.

Format

c90d9e6

davidungar requested a review from jrose-apple April 2, 2019 07:53

typo

f0fa6f1

jrose-apple previously requested changes Apr 2, 2019

View reviewed changes

davidungar mentioned this pull request Apr 2, 2019

[Batch mode] Cope with bugs that cause error suppression. apple/swift-lldb#1431

Merged

jrose-apple reviewed Apr 3, 2019

View reviewed changes

David Ungar added 4 commits April 2, 2019 21:41

Refactor handleDiagnostic

3b8b903

Pass defaultDiagnosticLoc to handleDiagnostic, not currentPrimaryInput.

c139c59

Format

0baa668

Typos

b112f73

davidungar force-pushed the tracking-primary branch from 3e276a0 to b112f73 Compare April 3, 2019 05:52

jrose-apple reviewed Apr 3, 2019

View reviewed changes

David Ungar added 2 commits April 3, 2019 11:01

Rm ForAnyKind

7ac4592

Change strategy for non-primaries

fd63a1e

No more vacuous subconsumers. Output into active primary or everywhere.

David Ungar added 3 commits April 3, 2019 12:41

Fix SourceLoc in SwiftLangSupport.h

c8341fa

Rename defaultDiagnosticLoc to bufferIndirectlyCausingDiagnostic.

9cc3a4a

Format

44daa88

David Ungar added 2 commits April 3, 2019 13:53

Update test

bd7a7cf

Updated test for batch-mode nonprimary errors & removed redundant tests.

ee449ae

jrose-apple reviewed Apr 4, 2019

View reviewed changes

David Ungar and others added 4 commits April 4, 2019 13:58

WIP

5c83732

Use MSF not mainInputFilenameForDebugInfo

8448da3

Better comment.

cba4ae8

Fix typo

9d99eb7

jrose-apple reviewed Apr 4, 2019

View reviewed changes

Expand parameter description.

bb131fb

davidungar and others added 3 commits April 4, 2019 16:59

Better idiom for sometimes RAII.

aafc2a3

WIP

240b573

Move location lookup to RAII object.

c8cd0f3

jrose-apple reviewed Apr 5, 2019

View reviewed changes

Typo and handle recursive RAII case.

aa0687a

Replaced a test.

6d79bed

davidungar force-pushed the tracking-primary branch from 4c8722c to 6d79bed Compare April 5, 2019 18:34

jrose-apple approved these changes Apr 5, 2019

View reviewed changes

davidungar merged commit cf5d81c into swiftlang:master Apr 6, 2019

davidungar pushed a commit to davidungar/swift that referenced this pull request Apr 7, 2019

Merge pull request swiftlang#23735 from davidungar/tracking-primary

cd3aaad

[Batch mode] Cope with bugs that cause error suppression.

davidungar pushed a commit that referenced this pull request Apr 7, 2019

Merge pull request #23848 from davidungar/rdar-40167351-error-suppres…

dad8180

…sion-fix-swift-5.1-branch [Batch mode] Merge pull request #23735 -rdar-40167351

davidungar mentioned this pull request Apr 9, 2019

[Batch mode] Cherry-pick fix for error suppression #23879

Closed

davidungar deleted the tracking-primary branch September 23, 2019 16:18

		@@ -1,18 +0,0 @@
		// Ensure that an error in a primary causes an error in the errorless primary.

		@@ -1,16 +0,0 @@
		// Ensure that an error in a non-primary causes an error in the errorless primary.

[Batch mode] Cope with bugs that cause error suppression. #23735

[Batch mode] Cope with bugs that cause error suppression. #23735

Uh oh!

Conversation

davidungar commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidungar commented Apr 2, 2019

Uh oh!

davidungar commented Apr 2, 2019

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

davidungar commented Apr 2, 2019

Uh oh!

davidungar commented Apr 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

davidungar commented Apr 3, 2019

Uh oh!

davidungar commented Apr 3, 2019

Uh oh!

davidungar commented Apr 3, 2019

Uh oh!

davidungar commented Apr 3, 2019

Uh oh!

davidungar commented Apr 3, 2019

Uh oh!

davidungar commented Apr 4, 2019

Uh oh!

davidungar commented Apr 4, 2019

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!

davidungar commented Apr 2, 2019 •

edited

Loading

davidungar Apr 3, 2019 •

edited

Loading

jrose-apple Apr 4, 2019 •

edited

Loading

davidungar Apr 5, 2019 •

edited

Loading