Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DiagnosticBridge] Make sure that diagnostic queues up its child notes #79812

Merged
merged 4 commits into from
Mar 7, 2025
Merged

[DiagnosticBridge] Make sure that diagnostic queues up its child notes #79812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a0dc2b
[DiagnosticBridge] Make sure that diagnostic queues up its child notes
xedin Mar 6, 2025
b7eecc3
[Frontend] Treat empty diagnostic category as "not present"
xedin Mar 6, 2025
b26c8aa
[Tests] NFC: Add a test-case for printing child notes associated with…
xedin Mar 6, 2025
a7a9a4d
[DiagnosticsBridge] Treat empty category name as `nil`
xedin Mar 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/AST/DiagnosticBridge.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ static void addQueueDiagnostic(void *queuedDiagnostics,
documentationPath.size(),
highlightRanges.data(),
highlightRanges.size() / 2);

// TODO: A better way to do this would be to pass the notes as an
// argument to `swift_ASTGen_addQueuedDiagnostic` but that requires
// bridging of `Note` structure and new serialization.
for (auto *childNote : info.ChildDiagnosticInfo) {
addQueueDiagnostic(queuedDiagnostics, *childNote, SM);
}
Comment on lines +84 to +86
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be clearer to move this up into enqueueDiagnostic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, all for moving to diagnoseWithNotes. Happy to leave it where it is 👍

}

void DiagnosticBridge::enqueueDiagnostic(SourceManager &SM,
Expand Down
9 changes: 8 additions & 1 deletion lib/ASTGen/Sources/ASTGen/DiagnosticsBridge.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,13 +339,20 @@ public func addQueuedDiagnostic(
}
}

let category: DiagnosticCategory? = categoryName.map { categoryNamePtr in
let category: DiagnosticCategory? = categoryName.flatMap { categoryNamePtr in
let categoryNameBuffer = UnsafeBufferPointer(
start: categoryNamePtr,
count: categoryLength
)
let categoryName = String(decoding: categoryNameBuffer, as: UTF8.self)

// If the data comes from serialized diagnostics, it's possible that
// the category name is empty because StringRef() is serialized into
// an empty string.
guard !categoryName.isEmpty else {
return nil
}

let documentationURL = documentationPath.map { documentationPathPtr in
let documentationPathBuffer = UnsafeBufferPointer(
start: documentationPathPtr,
Expand Down
9 changes: 9 additions & 0 deletions test/diagnostics/pretty-printed-diagnostics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ foo(b:
1,
a: 2)

// Test for child notes attached directly to a "primary" error/warning diagnostic
func test(a: Int) {}
func test(a: Int) {}

// Test fallback for non-ASCII characters.
// CHECK: SOURCE_DIR{{[/\]+}}test{{[/\]+}}diagnostics{{[/\]+}}pretty-printed-diagnostics.swift:[[#LINE:]]:11
Expand All @@ -85,3 +88,9 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅

// CHECK: [[#LINE-1]] | func test(a: Int) {}
// CHECK: | `- note: 'test(a:)' previously declared here
// CHECK: [[#LINE]] | func test(a: Int) {}
// CHECL: [[#LINE+1]] | `- error: invalid redeclaration of 'test(a:)'