Several improvements to diagnostics in the new parser #623
Conversation
ahoppen
force-pushed
the
pr/diag-changes
branch
from
a3c50eb to
6cf6c96
Compare
|
@swift-ci Please test |
Package.swift
Outdated
| @@ -58,6 +58,10 @@ let package = Package( | |||
| "README.md" | |||
| ] | |||
| ), | |||
| .target( | |||
| name: "Diagnostics", | |||
There was a problem hiding this comment.
This is a very general name. How about SwiftDiagnostics, since Swift-based tooling will use it?
There was a problem hiding this comment.
Good idea. I changed it.
| } | ||
| if let unexpectedCondition = node.body.unexpectedBeforeLeftBrace, | ||
| unexpectedCondition.tokens(withKind: .semicolon).count == 2 { | ||
| // FIXME: This is aweful. We should have a way to either get all children between two cursors in a syntax node or highlight a range from one node to another. |
There was a problem hiding this comment.
Ouch. Can we define a range as being all of the source between two Syntax nodes?
There was a problem hiding this comment.
I totally agree that this is awful. The reason why I didn’t want to use a range was that that might now become a runtime error if the end node is not after the start node (or even in a different syntax tree).
As I mentioned in the PR description, I would like to implement a few more diagnostics first to find common patterns that we use in diagnostic generation before designing the entire diagnostic system. E.g. if it turns out that this is the only diagnostic that needs to highlight >3 syntax nodes, I would maybe be fine with it. Or it would certainly influence our design.
There was a problem hiding this comment.
Basically, I would like to get this merged so I can start implementing new diagnostics on top of it.
There was a problem hiding this comment.
Oh yes, let's merge and we can iterate in the tree.
This should avoid clashes of diagnostic IDs with the same name from different modules.
ahoppen
force-pushed
the
pr/diag-changes
branch
from
6cf6c96 to
0f7b847
Compare
|
@swift-ci Please test |
|
Are there plans to make the There's also the one in swift-tools-support-core, but that package is something of a grab bag that brings in a lot of other things that aren't needed for just diagnostics. |
We do need to figure out the layering strategy here. This
Right, and I think we'd like to avoid a dependency on swift-tools-support-core here, because it also brings in a dependency on Foundation, which complicates the compiler build. I suspect this does mean we need a common layer underlying these tools, which knows how to render diagnostics with varying levels of source information (file/line/column, syntax nodes, etc.), emission to the Clang diagnostic file format, etc. I'd like to understand what else might live at that layer, or whether diagnostics is enough to form its own package. |
| //===----------------------------------------------------------------------===// | ||
|
|
||
| import SwiftSyntax | ||
|
|
There was a problem hiding this comment.
IMO we want a Note (but not with that name because it's the same as severity). Notes can have messages and a possibly-empty list of changes. Diagnostics can have any numbers of notes. Ie. it's just additional information on a diagnostic.
There was a problem hiding this comment.
I agree. Let’s add them when the need comes up.
| var message: String { get } | ||
|
|
||
| /// See ``MessageID``. | ||
| var fixItID: MessageID { get } |
There was a problem hiding this comment.
Do these (or notes in my scheme) need IDs? Seems like clients would only ever care about the ID of the diagnostic itself.
There was a problem hiding this comment.
I think it might be useful if clients want to hide certain fix-its or otherwise transform them. Adding IDs to things never hurts IMO.
| public struct FixIt { | ||
| public enum Change { | ||
| /// Replace `oldNode` by `newNode`. | ||
| case replace(oldNode: Syntax, newNode: Syntax) |
There was a problem hiding this comment.
I suspect we'll end up needing more than this but no harm in starting with this.
There was a problem hiding this comment.
Absolutely, that’s why I made it an enum. I’ll add more cases as uses come up.
| } | ||
|
|
||
| public enum StaticParserFixIt: String, FixItMessage { | ||
| case moveThrowBeforeArrow = "Move 'throws' in before of '->'" |
There was a problem hiding this comment.
| case moveThrowBeforeArrow = "Move 'throws' in before of '->'" | |
| case moveThrowBeforeArrow = "Move 'throws' before '->'" |
| XCTAssertSingleDiagnostic(in: signature, line: 1, column: 7, message: "'throws' may only occur before '->'") | ||
| XCTAssertSingleDiagnostic( | ||
| in: signature, | ||
| line: 1, column: 7, |
There was a problem hiding this comment.
It'd be nice to add a marker equivalent to the structure tests. Maybe not as important here, I just highly dislike referencing line/col in tests in general 😅
There was a problem hiding this comment.
I agree. Filed #638 to track it.
DiagnosticIDThe goal of this PR is to provide the mentioned functionality above. At this point, the goal is not to review ParseDiagnosticsGenerator.swift. I know that the generator has super rough edges at the moment but before optimizing it for better ergonomics, I would like to implement a few more diagnostics to see what the common patterns are.