Skip to content

Emit diagnostics for missing nodes #739

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.

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

Merged
merged 8 commits into from
Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 8 additions & 3 deletions Sources/SwiftDiagnostics/Diagnostic.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,21 @@ public struct Diagnostic: CustomDebugStringConvertible {
/// The node at whose start location the message should be displayed.
public let node: Syntax

/// The position at which the location should be anchored.
/// By default, this is the start location of `node`.
public let position: AbsolutePosition

/// Nodes that should be highlighted in the source code.
public let highlights: [Syntax]

/// Fix-Its that can be applied to resolve this diagnostic.
/// Each Fix-It offers a different way to resolve the diagnostic. Usually, there's only one.
public let fixIts: [FixIt]

public init(node: Syntax, message: DiagnosticMessage, highlights: [Syntax] = [], fixIts: [FixIt] = []) {
self.diagMessage = message
public init(node: Syntax, position: AbsolutePosition? = nil, message: DiagnosticMessage, highlights: [Syntax] = [], fixIts: [FixIt] = []) {
self.node = node
self.position = position ?? node.positionAfterSkippingLeadingTrivia
self.diagMessage = message
self.highlights = highlights
self.fixIts = fixIts
}
Expand All @@ -46,7 +51,7 @@ public struct Diagnostic: CustomDebugStringConvertible {

/// The location at which the diagnostic should be displayed.
public func location(converter: SourceLocationConverter) -> SourceLocation {
return node.startLocation(converter: converter)
return converter.location(for: position)
}

public var debugDescription: String {
Expand Down
42 changes: 37 additions & 5 deletions Sources/SwiftParser/Diagnostics/ParseDiagnosticsGenerator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,15 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
// MARK: - Private helper functions

/// Produce a diagnostic.
private func addDiagnostic<T: SyntaxProtocol>(_ node: T, _ message: DiagnosticMessage, highlights: [Syntax] = [], fixIts: [FixIt] = [], handledNodes: [SyntaxIdentifier] = []) {
private func addDiagnostic<T: SyntaxProtocol>(_ node: T, position: AbsolutePosition? = nil, _ message: DiagnosticMessage, highlights: [Syntax] = [], fixIts: [FixIt] = [], handledNodes: [SyntaxIdentifier] = []) {
diagnostics.removeAll(where: { handledNodes.contains($0.node.id) })
diagnostics.append(Diagnostic(node: Syntax(node), message: message, highlights: highlights, fixIts: fixIts))
diagnostics.append(Diagnostic(node: Syntax(node), position: position, message: message, highlights: highlights, fixIts: fixIts))
self.handledNodes.append(contentsOf: handledNodes)
}

/// Produce a diagnostic.
private func addDiagnostic<T: SyntaxProtocol>(_ node: T, _ message: StaticParserError, highlights: [Syntax] = [], fixIts: [FixIt] = [], handledNodes: [SyntaxIdentifier] = []) {
addDiagnostic(node, message as DiagnosticMessage, highlights: highlights, fixIts: fixIts, handledNodes: handledNodes)
private func addDiagnostic<T: SyntaxProtocol>(_ node: T,position: AbsolutePosition? = nil, _ message: StaticParserError, highlights: [Syntax] = [], fixIts: [FixIt] = [], handledNodes: [SyntaxIdentifier] = []) {
addDiagnostic(node, position: position, message as DiagnosticMessage, highlights: highlights, fixIts: fixIts, handledNodes: handledNodes)
}

/// Whether the node should be skipped for diagnostic emission.
Expand Down Expand Up @@ -118,8 +118,40 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
return .skipChildren
}

private func handleMissingSyntax<T: SyntaxProtocol>(_ node: T) -> SyntaxVisitorContinueKind {
if shouldSkip(node) {
return .skipChildren
}
addDiagnostic(node, position: node.endPosition, MissingNodeError(missingNode: Syntax(node)))
return .visitChildren
}

// MARK: - Specialized diagnostic generation

public override func visit(_ node: MissingDeclSyntax) -> SyntaxVisitorContinueKind {
return handleMissingSyntax(node)
}

public override func visit(_ node: MissingExprSyntax) -> SyntaxVisitorContinueKind {
return handleMissingSyntax(node)
}

public override func visit(_ node: MissingPatternSyntax) -> SyntaxVisitorContinueKind {
return handleMissingSyntax(node)
}

public override func visit(_ node: MissingStmtSyntax) -> SyntaxVisitorContinueKind {
return handleMissingSyntax(node)
}

public override func visit(_ node: MissingSyntax) -> SyntaxVisitorContinueKind {
return handleMissingSyntax(node)
}

public override func visit(_ node: MissingTypeSyntax) -> SyntaxVisitorContinueKind {
return handleMissingSyntax(node)
}

public override func visit(_ node: ForInStmtSyntax) -> SyntaxVisitorContinueKind {
if shouldSkip(node) {
return .skipChildren
Expand All @@ -145,7 +177,7 @@ public class ParseDiagnosticsGenerator: SyntaxAnyVisitor {
Syntax(node.unexpectedBetweenWhereClauseAndBody),
Syntax(unexpectedCondition)
] as [Syntax?]).compactMap({ $0 }),
handledNodes: [node.inKeyword.id, unexpectedCondition.id]
handledNodes: [node.inKeyword.id, node.sequenceExpr.id, unexpectedCondition.id]
)
}
return .visitChildren
Expand Down
44 changes: 41 additions & 3 deletions Sources/SwiftParser/Diagnostics/ParserDiagnosticMessages.swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,19 @@ extension SyntaxProtocol {
/// default.
/// Pass inherit: false to prevent this behavior, in which case `nil` will be
/// returned instead.
func nodeTypeNameForDiagnostics(inherit: Bool = true) -> String? {
/// If `allowSourceFile` is `false`, `nil` will be returned if the inherited
/// node type name is "source file".
func nodeTypeNameForDiagnostics(inherit: Bool = true, allowSourceFile: Bool = true) -> String? {
if let name = Syntax(self).as(SyntaxEnum.self).nameForDiagnostics {
return name
if Syntax(self).is(SourceFileSyntax.self) && !allowSourceFile {
return nil
} else {
return name
}
}
if inherit {
if let parent = self.parent {
return parent.nodeTypeNameForDiagnostics(inherit: inherit)
return parent.nodeTypeNameForDiagnostics(inherit: inherit, allowSourceFile: allowSourceFile)
}
}
return nil
Expand Down Expand Up @@ -135,6 +141,38 @@ public struct InvalidIdentifierError: ParserError {
}
}

public struct MissingNodeError: ParserError {
public let missingNode: Syntax

public var message: String {
var message: String
var hasNamedParent = false
if let parent = missingNode.parent,
let childName = parent.childNameForDiagnostics(missingNode.index) {
message = "Expected \(childName)"
if let parentTypeName = parent.nodeTypeNameForDiagnostics(inherit: false) {
message += " of \(parentTypeName)"
hasNamedParent = true
}
} else {
message = "Expected \(missingNode.nodeTypeNameForDiagnostics() ?? "syntax")"
if let missingDecl = missingNode.as(MissingDeclSyntax.self), let lastModifier = missingDecl.modifiers?.last {
message += " after '\(lastModifier.name.text)' modifier"
} else if let missingDecl = missingNode.as(MissingDeclSyntax.self), missingDecl.attributes != nil {
message += " after attribute"
} else if let previousToken = missingNode.previousToken(viewMode: .fixedUp), previousToken.presence == .present {
message += " after '\(previousToken.text)'"
Copy link
Contributor

Choose a reason for hiding this comment

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

If previousToken.parent != missingNode.parent, could we use the parent of the previous token rather than the token itself? That would give both the attribute tests Expected declaration after attribute rather than empty/')', but still keep after <token> for cases where it makes sense (eg. a ? b :).

Actually speaking of the ternary case, shouldn't it have in ternary expression at the end of it?

Copy link
Member Author

@ahoppen ahoppen Sep 21, 2022

Choose a reason for hiding this comment

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

If previousToken.parent != missingNode.parent, could we use the parent of the previous token rather than the token itself? That would give both the attribute tests Expected declaration after attribute rather than empty/')', but still keep after <token> for cases where it makes sense (eg. a ? b :).

after attribute should now be added based.

Actually speaking of the ternary case, shouldn't it have in ternary expression at the end of it?

It doesn’t have in ternary expression at the end of it because the expression is not missing inside the ternary expression but in the sequence expression. For a ? b : the syntax tree looks like

  • IdentifierExpr: a
  • UnresolvedTernaryExpr: ? b :
  • MissingExpr

Filed #824 so we can take care of it later.

}
}
if !hasNamedParent {
if let parent = missingNode.parent, let parentTypeName = parent.nodeTypeNameForDiagnostics(allowSourceFile: false) {
message += " in \(parentTypeName)"
}
}
return message
}
}

public struct MissingTokenError: ParserError {
public let missingToken: TokenSyntax

Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftParser/Directives.swift
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ extension Parser {
do {
var elementsProgress = LoopProgressCondition()
while !self.at(any: [.eof, .poundElseKeyword, .poundElseifKeyword, .poundEndifKeyword]) && elementsProgress.evaluate(currentToken) {
guard let element = parseElement(&self), element.raw.byteLength > 0 else {
guard let element = parseElement(&self), !element.isEmpty else {
break
}
elements.append(element)
Expand Down
18 changes: 12 additions & 6 deletions Sources/SwiftParser/Expressions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1617,21 +1617,27 @@ extension Parser {

switch elementKind! {
case .array(let el):
elements.append(RawSyntax(RawArrayElementSyntax(
expression: el, trailingComma: comma, arena: self.arena)))
if el.is(RawMissingExprSyntax.self) {
let element = RawArrayElementSyntax(
expression: el, trailingComma: comma, arena: self.arena
)
if element.isEmpty {
break COLLECTION_LOOP
} else {
elements.append(RawSyntax(element))
}
case .dictionary(let key, let unexpectedBeforeColon, let colon, let value):
elements.append(RawSyntax(RawDictionaryElementSyntax(
let element = RawDictionaryElementSyntax(
keyExpression: key,
unexpectedBeforeColon,
colon: colon,
valueExpression: value,
trailingComma: comma,
arena: self.arena)))
if key.is(RawMissingExprSyntax.self), colon.isMissing, value.is(RawMissingExprSyntax.self) {
arena: self.arena
)
if element.isEmpty {
break COLLECTION_LOOP
} else {
elements.append(RawSyntax(element))
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftParser/Statements.swift
Original file line number Diff line number Diff line change
Expand Up @@ -865,7 +865,7 @@ extension Parser {
let expr: RawExprSyntax?
if
!self.at(any: [
RawTokenKind.rightBrace, .semicolon, .eof,
.rightBrace, .caseKeyword, .semicolon, .eof,
.poundIfKeyword, .poundErrorKeyword, .poundWarningKeyword,
.poundEndifKeyword, .poundElseKeyword, .poundElseifKeyword
])
Expand Down
2 changes: 1 addition & 1 deletion Sources/SwiftParser/TopLevel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ extension Parser {
let item = self.parseItem(isAtTopLevel: isAtTopLevel)
let semi = self.consume(if: .semicolon)

if item.raw.byteLength == 0 && semi == nil {
if item.isEmpty && semi == nil {
return nil
}
return .init(item: item, semicolon: semi, errorTokens: nil, arena: self.arena)
Expand Down
13 changes: 13 additions & 0 deletions Sources/SwiftSyntax/Misc.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,19 @@ extension Syntax {
% for node in NON_BASE_SYNTAX_NODES:
case .${node.swift_syntax_kind}(let node):
return node
% end
}
}

public func childNameForDiagnostics(_ index: SyntaxChildrenIndex) -> String? {
switch self.as(SyntaxEnum.self) {
case .token(let node):
return node.childNameForDiagnostics(index)
case .unknown(let node):
return node.childNameForDiagnostics(index)
% for node in NON_BASE_SYNTAX_NODES:
case .${node.swift_syntax_kind}(let node):
return node.childNameForDiagnostics(index)
% end
}
}
Expand Down
4 changes: 4 additions & 0 deletions Sources/SwiftSyntax/Raw/RawSyntaxNodeProtocol.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ public extension RawSyntaxNodeProtocol {
func write<Target>(to target: inout Target) where Target : TextOutputStream {
raw.write(to: &target)
}

var isEmpty: Bool {
return raw.byteLength == 0
}
}

/// `RawSyntax` itself conforms to `RawSyntaxNodeProtocol`.
Expand Down
4 changes: 4 additions & 0 deletions Sources/SwiftSyntax/Syntax.swift
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ public protocol SyntaxProtocol: CustomStringConvertible,
/// Converts the given `Syntax` node to this type. Returns `nil` if the
/// conversion is not possible.
init?(_ syntaxNode: Syntax)

/// Return a name with which the child at the given `index` can be referred to
/// in diagnostics.
func childNameForDiagnostics(_ index: SyntaxChildrenIndex) -> String?
}

extension SyntaxProtocol {
Expand Down
4 changes: 4 additions & 0 deletions Sources/SwiftSyntax/SyntaxBaseNodes.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ public struct ${node.name}: ${node.name}Protocol, SyntaxHashable {
public func asProtocol(_: ${node.name}Protocol.Protocol) -> ${node.name}Protocol {
return Syntax(self).asProtocol(${node.name}Protocol.self)!
}

public func childNameForDiagnostics(_ index: SyntaxChildrenIndex) -> String? {
return Syntax(self).childNameForDiagnostics(index)
}
}

extension ${node.name}: CustomReflectable {
Expand Down
4 changes: 2 additions & 2 deletions Sources/SwiftSyntax/SyntaxChildren.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public struct SyntaxChildrenIndexData: Comparable {
fileprivate let offset: UInt32
/// The index of the node in the parent.
/// See `AbsoluteSyntaxPosition.indexInParent`
fileprivate let indexInParent: UInt32
let indexInParent: UInt32
/// Unique value for a node within its own tree.
/// See `SyntaxIdentifier.indexIntree`
fileprivate let indexInTree: SyntaxIndexInTree
Expand Down Expand Up @@ -61,7 +61,7 @@ public struct SyntaxChildrenIndex: Comparable, ExpressibleByNilLiteral {
/// SyntaxChildrenIndex` an enumbecause the optional value can be
/// force-unwrapped when we know that an index is not the end index, saving a
/// switch case comparison.
fileprivate let data: SyntaxChildrenIndexData?
let data: SyntaxChildrenIndexData?

fileprivate init(offset: UInt32, indexInParent: UInt32,
indexInTree: SyntaxIndexInTree) {
Expand Down
4 changes: 4 additions & 0 deletions Sources/SwiftSyntax/SyntaxCollections.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,10 @@ public struct ${node.name}: SyntaxCollection, SyntaxHashable {
self = withTrailingTrivia(newValue ?? [])
}
}

public func childNameForDiagnostics(_ index: SyntaxChildrenIndex) -> String? {
return nil
}
}

/// Conformance for `${node.name}` to the `BidirectionalCollection` protocol.
Expand Down
15 changes: 15 additions & 0 deletions Sources/SwiftSyntax/SyntaxNodes.swift.gyb.template
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,21 @@ public struct ${node.name}: ${base_type}Protocol, SyntaxHashable {
return ${node.name}(newData)
}
% end

public func childNameForDiagnostics(_ index: SyntaxChildrenIndex) -> String? {
switch index.data?.indexInParent {
% for (index, child) in enumerate(node.children):
case ${index}:
% if child.name_for_diagnostics:
return "${child.name_for_diagnostics}"
% else:
return nil
% end
% end
default:
fatalError("Invalid index")
}
}
}

extension ${node.name}: CustomReflectable {
Expand Down
8 changes: 8 additions & 0 deletions Sources/SwiftSyntax/SyntaxOtherNodes.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ public struct UnknownSyntax: SyntaxProtocol, SyntaxHashable {
let data = SyntaxData.forRoot(raw)
self.init(data)
}

public func childNameForDiagnostics(_ index: SyntaxChildrenIndex) -> String? {
return nil
}
}

extension UnknownSyntax: CustomReflectable {
Expand Down Expand Up @@ -192,6 +196,10 @@ public struct TokenSyntax: SyntaxProtocol, SyntaxHashable {
public var totalLength: SourceLength {
return raw.totalLength
}

public func childNameForDiagnostics(_ index: SyntaxChildrenIndex) -> String? {
return nil
}
}

extension TokenSyntax: CustomReflectable {
Expand Down
Loading