[DiagnosticVerifier] Make DiagnosticVerifier a DiagnosticConsumer #28313

owenv · 2019-11-17T07:05:14Z

Previously, DiagnosticVerifier hooked emission of llvm::SMDiagnostic to capture diagnostics for verification. This change makes DiagnosticVerifier a subclass of DiagnosticConsumer instead. This means that instead of checking expected diagnostics against the llvm::SMDiagnostics emitted by PrintedDiagnosticConsumer, it checks them directly against the DiagnosticInfos it receives from the DiagnosticEngine.

This is desirable for a couple reasons:

It allows us to verify information about diagnostics that's lost when a DiagnosticInfo is translated to an SMDiagnostic. This will allow adding verifier support for educational notes in a follow-up
It begins to reduce dependencies on SMDiagnostic. The current diagnostic pipeline looks like Diagnostic -> DiagnosticInfo -> SMDiagnostic -> Text Output when using the PrintingDiagnosticConsumer. Eventually I'd like to reduce that to just Diagnostic -> DiagnosticInfo -> Text Output. In addition to simplifying the diagnostics subsystem, this will make improving multi-byte character support in diagnostics much easier, and enable smaller QoL enhancements like https://bugs.swift.org/browse/SR-6024. (Edit: This will also allow running tests with -enable-experimental-diagnostic-formatting)

The downside of making this change now is that we lose test coverage of PrintingDiagnosticConsumer itself. Under the old system every test which used the verifier relied on it.

owenv · 2019-11-17T18:26:25Z

@swift-ci please test

owenv · 2019-12-04T02:10:04Z

Was re-reviewing this and found a few additional cleanup opportunities

owenv · 2019-12-10T19:25:06Z

@swift-ci please test

lib/Frontend/DiagnosticVerifier.cpp

owenv · 2019-12-12T18:50:00Z

@brentdax do you mind taking a look at this?

beccadax · 2020-01-20T22:19:19Z

@owenv I accidentally ended up commenting on this PR in #28964—sorry about that!

owenv · 2020-01-21T00:59:11Z

@brentdax No problem! I'll respond to your comments from the other PR here:

Do we know why DiagnosticVerifier wasn't a DiagnosticConsumer before? I'm worried that this change might make the verifier not, for instance, compose with other diagnostic consumers the way it used to. Do -serialize-diagnostics-path and -emit-fixits-path in combination with -verify work the same way they used to? I'm also worried about whether other diagnostic-related features like -warnings-as-errors, -debug-diagnostic-names, or even -color-diagnostics will work the same way they used to. How do these things interact with this change?

I'm not sure why it wasn't a DiagnosticConsumer before. The one downside I do know of is I had to add the suppressOutput hack to PrintingDiagnosticConsumer, but I may be missing historical context somewhere.

The old verifier hooked the output of PrintingDiagnosticConsumer, and this one is independent of all the other consumers. As a result, -serialize-diagnostics-path and -emit-fixits-path continue to work because SerializedDiagnosticConsumer is unchanged. -warnings-as-errors overrides the diagnostic kind in DiagnosticsEngine before diagnostics reach consumers, so its behavior is also unchanged. -debug-diagnostic-names continues to work because the verifier looks for a substring instead of an exact match (and the old verifier already received the debug strings if the flag was passed).

I hadn't thought to test the color flags, it turns out the current verifier doesn't respect them. I've updated this PR to begin taking them into account.

Separately: Why is DiagnosticVerifier's full definition provided in this include/swift header? The other diagnostic consumers only declare a function to create an instance and hide the rest of the implementation, and the old -verify implementation also only exposed a few functions to set it up. Is there an architectural difference that forces DiagnosticVerifier to be different? If not, moving a lot of these declarations into the .cpp file, or at least into a separate header, might help us keep compile times down.

No particular reason, I looked over the code again and was able to move a decent number out of the header. CapturedDiagnosticInfo is still there because the vector can't have an incomplete type, and the three remaining private methods all need to be able to call DiagnosticConsumer::getRawLoc.

owenv · 2020-01-21T01:00:36Z

Is this comment still accurate with the refactoring you've done? I don't think the final processing actually happens in this if anymore.

Yeah, this comment was in the wrong place. I've moved it

owenv · 2020-02-07T00:21:43Z

@swift-ci please smoke test

owenv · 2020-02-07T00:23:39Z

@brentdax I was able to extend the test coverage a bit to cover some of the edge cases you mentioned. Do you mind taking another look?

Also, since performance is relevant here, I ran the test suite locally with and without this change applied, and got 415s before and 430s after

owenv · 2020-02-07T03:07:20Z

@swift-ci please smoke test macOS

owenv · 2020-02-18T03:20:37Z

@swift-ci please smoke test

owenv · 2020-02-19T21:39:56Z

@brentdax bump

owenv · 2020-02-29T01:47:33Z

@swift-ci please test

owenv · 2020-02-29T01:47:45Z

@swift-ci please test Windows platform

owenv · 2020-03-04T23:26:43Z

I'm not quite sure how to resolve these conflicts with the new dependency verifier. @CodaFi does DependencyVerifier rely on hooking the emission of SMDiagnostics? I've been trying to move away from that approach in this PR because it blocks testing aspects of Swift diagnostics that can't be represented by SMDiagnostic, and couples the verifier to implementation details of PrintingDiagnosticConsumer.

owenv · 2020-03-06T02:41:11Z

@swift-ci please smoke test

owenv · 2020-03-10T16:44:40Z

@brentdax there's no rush, but do you mind taking another look at this when you have a chance? I think it's getting closer to being a viable replacement for the existing verifier

owenv · 2020-03-12T17:00:07Z

@swift-ci please smoke test

owenv · 2020-03-20T19:25:14Z

@swift-ci test

owenv · 2020-03-23T00:15:36Z

@swift-ci test

lib/Frontend/DiagnosticVerifier.cpp

CodaFi · 2020-03-27T17:56:52Z

lib/FrontendTool/FrontendTool.cpp

+  // The verifier must be setup after CompilerInstance::setup is called above to
+  // ensure the input buffer IDs have been added.
+  std::unique_ptr<DiagnosticVerifier> verifier;
+  if (diagOpts.VerifyMode != DiagnosticOptions::NoVerify) {


This seems like a really tricky initialization condition - maybe we should move the verifier instance onto CompilerInstance and set it up in the right place then.

Good idea, moving it onto CompilerInstance seems to work well

CodaFi · 2020-03-27T18:00:09Z

lib/FrontendTool/FrontendTool.cpp

+    // If we're in -verify mode, temporarily ignore errors. If verification
+    // failed, we'll still report an error correctly when finishDiagProcessing
+    // runs.
+    HadError = false;


Is universally resetting the error code here a good idea? Granted, it's probably not a good idea for the verifier to have been doing this in the first place, but I imagine we've grown quite a few tests that rely on this behavior.

I moved this logic into finishDiagProcessing instead so we override the error code at the last possible moment, which I think makes this logic easier to understand.

lib/Frontend/DiagnosticVerifier.cpp

include/swift/Frontend/DiagnosticVerifier.h

CodaFi · 2020-03-27T18:06:18Z

lib/Frontend/DiagnosticVerifier.cpp

      addError(expected.MessageRange.begin(), "incorrect message found", fixIt);
-    } else if (I->getColumnNo() + 1 != (int)*expected.ColumnNo) {
+    } else if (I->Column != *expected.ColumnNo) {


Were we off-by one?

This is just a consequence of operating on DiagnosticInfo instead of SMDiagnostic. I don't know why their column indexing is inconsistent, but I'm hesitant to change the behavior as part of this PR

lib/Frontend/DiagnosticVerifier.cpp

CodaFi

A raft of stylistic points. I really appreciate you trying to improve this long-neglected critical component.

…bclass Update DiagnosticVerifier to respect color flags Improve DiagnosticVerifier test coverage verifier updates to support new llvm stable branch

owenv · 2020-03-28T19:49:35Z

@CodaFi thanks for all the feedback, this is really helpful! I believe I've addressed your initial points.

CodaFi

Nice and tidy! Let's get this merged.

CodaFi · 2020-03-30T19:30:23Z

@swift-ci test

owenv · 2020-03-30T23:38:33Z

@swift-ci please test Windows platform

CodaFi · 2020-03-31T00:46:27Z

⛵️

owenv requested review from DougGregor and beccadax November 18, 2019 17:38

owenv force-pushed the verifier-consumer branch from 555b047 to 2bdaf1d Compare December 4, 2019 02:06

owenv mentioned this pull request Dec 4, 2019

[Diagnostics] Add an educational note explaining closure type inference rules #28572

Merged

CodaFi reviewed Dec 10, 2019

View reviewed changes

lib/Frontend/DiagnosticVerifier.cpp Outdated Show resolved Hide resolved

owenv force-pushed the verifier-consumer branch 2 times, most recently from 5f2cefa to 8d5a866 Compare December 10, 2019 21:46

owenv mentioned this pull request Dec 28, 2019

[Diagnostics] Add verifier support for educational notes #28964

Closed

owenv force-pushed the verifier-consumer branch from 8d5a866 to b38b0b1 Compare January 21, 2020 00:15

owenv force-pushed the verifier-consumer branch from 6a7843d to c6ea47c Compare February 7, 2020 00:20

owenv force-pushed the verifier-consumer branch from c6ea47c to 317d944 Compare February 18, 2020 03:20

owenv force-pushed the verifier-consumer branch from 317d944 to 6c0c858 Compare February 20, 2020 21:33

owenv mentioned this pull request Feb 24, 2020

[Diagnostics] Updated diagnostic printing style #30027

Merged

owenv force-pushed the verifier-consumer branch from 6c0c858 to d464609 Compare February 29, 2020 01:46

owenv force-pushed the verifier-consumer branch from d464609 to 1738128 Compare March 4, 2020 23:18

owenv force-pushed the verifier-consumer branch from f29d0d9 to 658cca7 Compare March 10, 2020 16:43

owenv force-pushed the verifier-consumer branch from 658cca7 to 88d3a6d Compare March 12, 2020 16:59

owenv force-pushed the verifier-consumer branch 2 times, most recently from 138d902 to 01a5c39 Compare March 20, 2020 19:24

owenv force-pushed the verifier-consumer branch from 01a5c39 to dfd9158 Compare March 23, 2020 00:11

owenv mentioned this pull request Mar 23, 2020

Turn Educational Notes On-By-Default #30583

Merged

owenv requested a review from CodaFi March 24, 2020 23:50