Skip to content

Implement a few more diagnostics in SwiftParser #1124

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 3 commits into from
Jan 13, 2023
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
2 changes: 1 addition & 1 deletion Sources/SwiftParser/Nominals.swift
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ extension Parser {
introucerHandle: RecoveryConsumptionHandle
) -> T where T: NominalTypeDeclarationTrait {
let (unexpectedBeforeIntroducerKeyword, introducerKeyword) = self.eat(introucerHandle)
let (unexpectedBeforeName, name) = self.expectIdentifier(keywordRecovery: true)
let (unexpectedBeforeName, name) = self.expectIdentifier(allowIdentifierLikeKeywords: false, keywordRecovery: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

When we do the keyword merging it would be nice to have sets for all the different "keyword" tokens we can have.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey we can totally do this now :D. But fine not to for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe in a follow-up PR.

if unexpectedBeforeName == nil && name.isMissing && self.currentToken.isAtStartOfLine {
return T.init(
attributes: attrs.attributes,
Expand Down
7 changes: 6 additions & 1 deletion Tests/SwiftParserTest/Assertions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ func AssertParse(
_ markedSource: String,
substructure expectedSubstructure: Syntax? = nil,
substructureAfterMarker: String = "START",
substructureCheckTrivia: Bool = false,
diagnostics expectedDiagnostics: [DiagnosticSpec] = [],
applyFixIts: [String]? = nil,
fixedSource expectedFixedSource: String? = nil,
Expand All @@ -454,6 +455,7 @@ func AssertParse(
{ SourceFileSyntax.parse(from: &$0) },
substructure: expectedSubstructure,
substructureAfterMarker: substructureAfterMarker,
substructureCheckTrivia: substructureCheckTrivia,
diagnostics: expectedDiagnostics,
applyFixIts: applyFixIts,
fixedSource: expectedFixedSource,
Expand All @@ -470,6 +472,7 @@ func AssertParse<S: SyntaxProtocol>(
_ parse: (inout Parser) -> S,
substructure expectedSubstructure: Syntax? = nil,
substructureAfterMarker: String = "START",
substructureCheckTrivia: Bool = false,
diagnostics expectedDiagnostics: [DiagnosticSpec] = [],
applyFixIts: [String]? = nil,
fixedSource expectedFixedSource: String? = nil,
Expand All @@ -484,6 +487,7 @@ func AssertParse<S: SyntaxProtocol>(
},
substructure: expectedSubstructure,
substructureAfterMarker: substructureAfterMarker,
substructureCheckTrivia: substructureCheckTrivia,
diagnostics: expectedDiagnostics,
applyFixIts: applyFixIts,
fixedSource: expectedFixedSource,
Expand Down Expand Up @@ -515,6 +519,7 @@ func AssertParse<S: SyntaxProtocol>(
_ parse: (String) -> S,
substructure expectedSubstructure: Syntax? = nil,
substructureAfterMarker: String = "START",
substructureCheckTrivia: Bool = false,
diagnostics expectedDiagnostics: [DiagnosticSpec] = [],
applyFixIts: [String]? = nil,
fixedSource expectedFixedSource: String? = nil,
Expand Down Expand Up @@ -545,7 +550,7 @@ func AssertParse<S: SyntaxProtocol>(
if let expectedSubstructure = expectedSubstructure {
let subtreeMatcher = SubtreeMatcher(Syntax(tree), markers: markerLocations)
do {
try subtreeMatcher.assertSameStructure(afterMarker: substructureAfterMarker, Syntax(expectedSubstructure), file: file, line: line)
try subtreeMatcher.assertSameStructure(afterMarker: substructureAfterMarker, Syntax(expectedSubstructure), includeTrivia: substructureCheckTrivia, file: file, line: line)
} catch {
XCTFail("Matching for a subtree failed with error: \(error)", file: file, line: line)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ final class DollarIdentifierTests: XCTestCase {
}
""",
diagnostics: [
DiagnosticSpec(message: "'$' is not a valid identifier")
// TODO: Old parser expected error on line 3: '$' is not an identifier; use backticks to escape it, Fix-It replacements: 9 - 10 = '`$`'
DiagnosticSpec(message: "'$' is not a valid identifier", fixIts: ["if this name is unavoidable, use backticks to escape it"])
]
)
}
Expand Down Expand Up @@ -118,7 +117,6 @@ final class DollarIdentifierTests: XCTestCase {
$(1️⃣$: 24)
""",
diagnostics: [
// TODO: Old parser expected error on line 3: '$' is not an identifier; use backticks to escape it, Fix-It replacements: 3 - 4 = '`$`'
DiagnosticSpec(message: "'$' is not a valid identifier")
]
)
Expand Down Expand Up @@ -171,7 +169,6 @@ final class DollarIdentifierTests: XCTestCase {
}
""",
diagnostics: [
// TODO: Old parser expected error on line 3: expected expression
DiagnosticSpec(message: "unexpected code in function")
]
)
Expand Down
40 changes: 33 additions & 7 deletions Tests/SwiftParserTest/translated/HashbangLibraryTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,47 @@

// This test file has been translated from swift/test/Parse/hashbang_library.swift

import SwiftSyntax

import XCTest

final class HashbangLibraryTests: XCTestCase {
func testHashbangLibrary1() {
// Check that we diagnose and skip the hashbang at the beginning of the file
// when compiling in library mode.
AssertParse(
"""
#!/usr/bin/swift
#!/usr/bin/swift
class Foo {}
// Check that we diagnose and skip the hashbang at the beginning of the file
// when compiling in library mode.
""",
diagnostics: [
// TODO: Old parser expected error on line 1: hashbang line is allowed only in the main file
Copy link
Contributor

Choose a reason for hiding this comment

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

No diagnostic... what tree do we produce here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We parse the shebang as trivia. I would like to parse it as part of the source file at some point (rdar://98309158) but the current implementation isn’t too bad.

Added a substructure test.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW you don't need all those nil's any more. Maybe we should update the dump to not add them either 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, the PR is so old it still required the nils when I created it. I removed them.

]
substructure: Syntax(
SourceFileSyntax(
statements: CodeBlockItemListSyntax([
CodeBlockItemSyntax(
item: .decl(
DeclSyntax(
ClassDeclSyntax(
classKeyword: .keyword(
.class,
leadingTrivia: [
.shebang("#!/usr/bin/swift"),
.newlines(1),
],
trailingTrivia: .space
),
identifier: .identifier("Foo", trailingTrivia: .space),
members: MemberDeclBlockSyntax(
members: MemberDeclListSyntax([])
)
)
)
)
)
]),
eofToken: .eof()
)
),
substructureCheckTrivia: true
)
}

}
24 changes: 21 additions & 3 deletions Tests/SwiftParserTest/translated/IdentifiersTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,29 @@ final class IdentifiersTests: XCTestCase {
func testIdentifiers8b() {
AssertParse(
"""
struct Self {}
struct 1️⃣Self {}
""",
diagnostics: [
// TODO: Old parser expected error on line 3: keyword 'Self' cannot be used as an identifier here
// TODO: Old parser expected note on line 3: if this name is unavoidable, use backticks to escape it, Fix-It replacements: 8 - 12 = '`Self`'
DiagnosticSpec(message: "keyword 'Self' cannot be used as an identifier here")
]
)
}

func testStructNamedLowercaseAny() {
AssertParse(
"""
struct any {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the point of this test to check a keyword or to check Any for anyKeyword?

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, I hate that we have capital any and lowercase any. I also added a test case for capital Any. It’s behaving correctly as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh :) All the more reason to merge keywords!

"""
)
}

func testStructNamedCapitalAny() {
AssertParse(
"""
struct 1️⃣Any {}
""",
diagnostics: [
DiagnosticSpec(message: "keyword 'Any' cannot be used as an identifier here")
]
)
}
Expand Down