[DiagnosticBridge] Make sure that diagnostic queues up its child notes #79812
Conversation
|
@swift-ci please smoke test |
|
Can you add some test case? I see the CAS diagnostics test actually needs to be updated for new child diagnostics but it will be good to have a test that is outside CAS to exercise the new code. |
|
@cachemeifyoucan That’s why I added you, looking for suggestions for how to test this properly. |
xedin
force-pushed
the
swift-formatter-should-print-child-notes
branch
from
22adfb0 to
782f7f1
Compare
`DiagnosticEngine` has an API that allows to attach notes to a "primary" diagnostic (an error or a warning). This works well with old formatting (`llvm`) but `swift` formatter doesn't display attached notes which makes some diagnostics very hard to work with i.e. `invalid_redecl`.
Serialized diagnostic stores an `std::string` which means that the category gets marked as present even if its empty, which is incorrect.
xedin
force-pushed
the
swift-formatter-should-print-child-notes
branch
from
0c00433 to
1c86dd7
Compare
|
@swift-ci please test |
xedin
requested review from
ahoppen,
bnbarham,
CodaFi,
hamishknight and
rintaro
as code owners
|
@swift-ci please test |
|
LGTM |
| @@ -85,3 +88,7 @@ foo(b: | |||
| // CHECK: [[#LINE]] | foo(b: 1, a: 2) | |||
| // CHECK: | `- error: argument 'a' must precede argument 'b' | |||
| // CHECK: [[#LINE+1]] | | |||
|
|
|||
| // CHECK: SOURCE_DIR{{[/\]+}}test{{[/\]+}}diagnostics{{[/\]+}}pretty-printed-diagnostics.swift:[[#LINE:]]:6 | |||
There was a problem hiding this comment.
It'd be nice to check for the error here too, I was initially very confused by this and thought that it was somehow a note off on its own 😅
| for (auto *childNote : info.ChildDiagnosticInfo) { | ||
| addQueueDiagnostic(queuedDiagnostics, *childNote, SM); | ||
| } |
There was a problem hiding this comment.
Might be clearer to move this up into enqueueDiagnostic?
There was a problem hiding this comment.
I'd prefer to keep this in here for now since this is technically an implementation detail a "primary" diagnostic and ideally should be dispatched as an argument like what I mentioned in a TODO.
There was a problem hiding this comment.
Unless I'm misunderstanding, the infrastructure currently assumes that notes are attached to the first primary diagnostic in the group. Ie. enqueueDiagnostic isn't necessarily a primary diagnostic anyway. But 🤷♂️
There was a problem hiding this comment.
You might be talking about lower level things but there is a dedicated API in DiagnosticEngine which let's the users attach notes directly to the diagnostic they want. I think we should replace all of the disjoint emission of notes with diagnoseWithNotes and remove any inference that happens later on...
There was a problem hiding this comment.
Yeah, all for moving to diagnoseWithNotes. Happy to leave it where it is 👍
Default value for `Category` for serialization purposes is an empty string but it should be handled as `nil` while bridging because category name cannot be empty when present.
xedin
force-pushed
the
swift-formatter-should-print-child-notes
branch
from
40c92eb to
a7a9a4d
Compare
|
@swift-ci please test |
|
@swift-ci please test Linux platform |
DiagnosticEnginehas an API that allows to attach notes to a "primary" diagnostic (an error or a warning). This works well with old formatting (llvm) butswiftformatter doesn't display attached notes which makes some diagnostics very hard to work with i.e.invalid_redecl.