[Diagnostics][NFC] Optionally add notes as explicit children of an error/warning #26830

owenv · 2019-08-25T18:16:25Z

Add a new builder method, addChildNotes, to InFlightDiagnostic, which accepts a lambda expression. Any notes diagnosed inside will be added as children of the parent error or warning. DiagnosticConsumers may opt in to consuming these child notes alongside the parent
diagnostic, or they can continue to consider them seperately.

Moved PrintingDiagnosticConsumer and a couple of diagnostics to the new
system as a proof of concept.

This is intended to unblock some future improvements to diagnostic presentation and formatting. The most controversial bits of this are probably the DiagnosticInfo refactor and the use of addChildNotes blocks to associate notes with a parent diagnostic.

This change is opt-in for DiagnosticConsumers. Child notes are both attached to their parent, and emitted standalone. Consumers which have been updated with knowledge of child notes can ignore the standalone emissions. Also, this change makes many of the handleDiagnostic arguments redundant now that everything is stored in DiagnosticInfo. Since there are many consumers that would need to be migrated, I thought it would be best not to clutter this PR.

jrose-apple

I definitely like this idea, and it helps enforce the idea that notes should always be attached to a parent. I am worried about the implementation, though. Ownership needs to be clear at all stages. Would it be possible to reuse more of the existing transaction machinery for this?

include/swift/AST/DiagnosticConsumer.h

lib/AST/DiagnosticEngine.cpp

include/swift/AST/DiagnosticEngine.h

lib/AST/DiagnosticEngine.cpp

owenv · 2019-08-28T16:27:31Z

@jrose-apple Thanks for the feedback, apologies for some of the rookie c++ mistakes! I've fixed the smaller issues you pointed out.

~~I'm not sure there's a good way to leverage transactions here. The issue I ran into is that the order of events when attaching child notes is roughly:~~

~~begin building the parent diagnostic~~
~~begin building notes~~
~~as notes are built and go out of scope, attach them to the parent~~
~~finish building the parent, emit it when it goes out of scope~~
~~emit the parent's children after the parent is emitted~~

~~Emitting the parent in between finishing building the notes and emitting them breaks the LIFO transaction model.~~

Having both TransactionStrings and ChildNoteStrings around to hold arguments is definitely less than ideal, but they are at least disjoint because child notes don't get added to a transaction, they're just emitted whenever their parent is.

owenv · 2019-09-16T16:06:23Z

@jrose-apple I've updated this PR to use a transaction-based API to group diagnostics, which simplifies argument ownership quite a bit. Do you mind taking another look?

The DiagnosticConsumer interface could still be simplified to take only a DiagnosticInfo after these updates, but I was going to leave that refactor to another PR because a lot of tools use PrintingDiagnosticConsumer in somewhat unconventional ways right now.

jrose-apple

I like how this turned out, though it's too bad we end up copying things around a little more instead of getting to rely on strings just being alive by virtue of being on the stack when we call diagnose. I think there's still an ownership problem here, though—see the comments.

include/swift/AST/DiagnosticEngine.h

jrose-apple · 2019-09-16T22:54:50Z

lib/Sema/CSApply.cpp

+            {
+              CompoundDiagnosticTransaction transaction(tc.Diags);
+              tc.diagnose(loc, swift::diag::optional_ambiguous_case_ref,
+                          baseTyName, baseTyUnwrappedName, memberName.str());


The separate transaction is a little clunky. What you think about having something like diagnoseWithNotes on DiagnosticEngine that returns an InFlightDiagnosticWithNotes, which inherits from InFlightDiagnostic but also carries a CompoundDiagnosticTransaction?

auto errorDiag = tc.diagnoseWithNotes(…); tc.diagnose(…); tc.diagnose(…);

instead of

CompoundDiagnosticTransaction transaction(tc.Diags); tc.diagnose(…); tc.diagnose(…); tc.diagnose(…);

I agree, this is a really great idea! It strikes a nice balance between the two approaches I've tried so far.

It turns out this doesn't work out too well because the transaction needs to outlive the parent InFlightDiagnostic(WithNotes). Otherwise we end up keeping the parent alive while diagnosing the notes which breaks a lot of important assumptions.

The best alternative I can come up with at the moment is a buildCompoundDiagnostic helper which accepts a lambda with an implicit transaction (see my second commit). This doesn't reduce the code involved, but it's easier to read the code and understand what's going on.

tc.buildCompoundDiagnostic([&]() { tc.diagnose(...); tc.diagnose(...); tc.diagnose(...); });

Hm. I like the explicit stack scoping, but it still feels like the parent diagnostic should be in the compound action. What about this (on DiagnosticEngine)?

template<typename ...ArgTypes> void diagnoseWithNotes( SourceLoc Loc, Diag<ArgTypes...> ID, typename detail::PassArgument<ArgTypes>::type... Args, llvm::function_ref<void(void)> builder) { CompoundDiagnosticTransaction transaction(*this); diagnose(Loc, ID, Args...); builder(); }

The one thing I don't like is that you can't add highlights or fix-its this way, but it would be possible to have another overload that took an InFlightDiagnostic as well. Though you'd have to call flush() explicitly there.

Hmm, types are only deduced for variadic parameter packs when they come at the end of an argument list though, which makes this kind of unwieldy. On the other hand, moving builder to the front of the list would be pretty unintuitive.

I don't think the inability to add fix-its is that big an issue. If an error has notes, it very rarely has fix-its of its own.

I ended up updating this to just accept an InFlightDiagnostic, manually specifying template parameters became too annoying.

That allows us to write:

tc.diagnoseWithNotes( tc.diagnose(...), [&]() { tc.diagnose(...); tc.diagnose(...); });

It's not perfect, but I think it's still an improvement

include/swift/AST/DiagnosticEngine.h

…ror/warning Add a new type of diagnostic transaction, CompoundDiagnosticTransaction. The first diagnostic emitted inside the transaction will become the parent of the subsequent notes. DiagnosticConsumers may opt in to consuming these child notes alongside the parent diagnostic, or they can continue to consider them seperately. Moved PrintingDiagnosticConsumer and a couple of diagnostics to the new system as a proof of concept.

jrose-apple

This works for me! @xedin, what do you think of these use sites?

lib/Sema/TypeCheckDecl.cpp

…need to be used manually in simple cases

jrose-apple · 2019-09-17T20:45:26Z

@swift-ci Please smoke test

owenv · 2019-09-24T21:04:06Z

@xedin bump, mind taking a quick look at this when you have a chance?

owenv · 2019-10-01T20:17:34Z

@xedin bump

xedin

Sorry I missed this one. Nice improvement! I think time will be a good judge whether we'll have to change something here when we start using this in more places.

jrose-apple · 2019-10-02T01:12:19Z

Let's do a retest just to be sure.

@swift-ci Please smoke test

owenv · 2019-10-02T16:26:05Z

I'm pretty sure the linux CI didn't actually hit an unexpected compile error, it looks like the real failure was in sourcekit-lsp integration testing. It's hard to say for sure whether it's related to this change, I may try to spin up an Ubuntu install later and investigate if it continues to fail.

jrose-apple · 2019-10-02T16:34:35Z

Let's try again.

@swift-ci Please smoke test Linux

jrose-apple · 2019-10-02T16:37:14Z

@swift-ci Please smoke test Linux

jrose-apple · 2019-10-02T17:51:18Z

*grumble*

@swift-ci Please smoke test Linux

jrose-apple · 2019-10-02T21:34:42Z

Thanks, @owenv!

owenv · 2019-10-02T21:43:02Z

🎉🎉🎉 Thanks for keeping an eye on the CI!

xedin · 2019-10-02T21:50:22Z

@owenv Adoption of the new setup in https://github.com/apple/swift/blob/master/lib/Sema/ConstraintSystem.cpp#L2521 would be a real challenge, so I'd suggest we try to do that next.

xedin requested a review from jrose-apple August 26, 2019 17:18

jrose-apple reviewed Aug 28, 2019

View reviewed changes

owenv force-pushed the diag_child_notes_refactor branch from ae19b41 to 4d1eb0c Compare August 28, 2019 16:15

owenv force-pushed the diag_child_notes_refactor branch from 4d1eb0c to c076133 Compare August 28, 2019 19:13

owenv force-pushed the diag_child_notes_refactor branch 2 times, most recently from 8c81fd7 to d74a340 Compare September 16, 2019 15:49

jrose-apple reviewed Sep 16, 2019

View reviewed changes

owenv force-pushed the diag_child_notes_refactor branch 2 times, most recently from 8368b74 to 8d6b60d Compare September 17, 2019 19:04

jrose-apple reviewed Sep 17, 2019

View reviewed changes

lib/Sema/TypeCheckDecl.cpp Outdated Show resolved Hide resolved

Add diagnoseWithNotes helper so CompoundDiagnosticTransation doesn't …

6e1e768

…need to be used manually in simple cases

owenv force-pushed the diag_child_notes_refactor branch from 8d6b60d to 6e1e768 Compare September 17, 2019 20:43

jrose-apple requested a review from xedin September 17, 2019 20:45

xedin approved these changes Oct 2, 2019

View reviewed changes

jrose-apple self-assigned this Oct 2, 2019

jrose-apple merged commit 5f26958 into swiftlang:master Oct 2, 2019

[Diagnostics][NFC] Optionally add notes as explicit children of an error/warning #26830

[Diagnostics][NFC] Optionally add notes as explicit children of an error/warning #26830

Uh oh!

Conversation

owenv commented Aug 25, 2019

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owenv commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owenv commented Sep 16, 2019

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

owenv Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

owenv Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

jrose-apple Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

owenv Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

owenv Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrose-apple commented Sep 17, 2019

Uh oh!

owenv commented Sep 24, 2019

Uh oh!

owenv commented Oct 1, 2019

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Oct 2, 2019

Uh oh!

owenv commented Oct 2, 2019

Uh oh!

jrose-apple commented Oct 2, 2019

Uh oh!

jrose-apple commented Oct 2, 2019

Uh oh!

jrose-apple commented Oct 2, 2019

Uh oh!

jrose-apple commented Oct 2, 2019

Uh oh!

owenv commented Oct 2, 2019

Uh oh!

xedin commented Oct 2, 2019

Uh oh!

Uh oh!

owenv commented Aug 28, 2019 •

edited

Loading