Produce a meaningful diagnostic for "#elif" typo

[TiagoMaiaL]

Purpose

To fix the second part of issue #1221: emit a proper diagnostic and fixit for the common #elif typo.

Implementation details

I've considered two approaches:

1. Add a new elif case to the TokenKind enums, and later on use it to generate the proper diagnostic
2. Use the existing pound TokenKind to determine if the user has a typo in the code and generate the diagnostic

I chose the second option because it didn't require changes to the TokenKind enums. I'm now adding unexpected tokens for the .pound + .identifier cases. If the identifier text is elif, I synthesize a poundElseIf token. I don't know how we could also check for .pound + .keywords, or if this would be needed.

For the diagnostic generations, I'm adding the proper fixit for the elif case, and if the identifier is any other word, I just add a generic diagnostic for it.

Any tip or help to get this right is really appreciated. Thank you in advance.

[ahoppen]

Thank you. I think you are going in the right direction but macros are making this harder because #elif could be a macro invocation. See my comment inline.

[ahoppen]

Sorry for the delay. I started a review but forgot to submit the comments 😬

[ahoppen]

This is looking good now. I’ve got a couple stylistic comments inline, otherwise this is ready to be merged.

[ahoppen]

Thank you. Let’s 🚢it

[ahoppen]

@swift-ci Please test

[TiagoMaiaL]

@ahoppen I'm not sure why this is failing. Here's what I'm seeing from the pipeline:

I'm also seeing some tests fail on main:

I'm using Xcode 14.0.1, and MacOS 13.0

[ahoppen]

Backtracing/CrashAsync.swift is currently a known failure that also fails on main. You can see the current status of main on https://ci.swift.org/job/oss-swift-pr-test-macoss/. So that’s nothing you need to worry about. It should be resolved soon.

Parse/ConditionalCompilation/language_version.swift appears to be a real failure caused by your changes. You should be able to reproduce it by

• Cloning https://github.com/apple/swift/
• Building your branch of SwiftSyntax and running swift-parser-cli dump-parse path/to/swift/Parse/ConditionalCompilation/language_version.swift
• You should also be able to create a smaller reproducer of the problem using swift-parser-cli reduce path/to/swift/Parse/ConditionalCompilation/language_version.swift
• You should then add a test case with the reduced source outputted by the command above and fix wherever it’s crashing

Those other test cases look odd. I can’t reproduce them on my machine. Can you expand the test failures and send me the difference between the expected and generated source?

[TiagoMaiaL]

Hey @ahoppen, I've performed the steps you suggested above, and I've created some issues as a result.

The failing tests on main are being described by issue #1410

Regarding the parsing error on Parse/ConditionalCompilation/language_version.swift, issue #1409 directly affects this branch. What was supposed to be parsed as a type, is being parsed as a macro, and later on we have a single pound (#) char that's being parsed in parsePoundIfDirective and hitting the precondition in the default case of the switch.

Any ideas on how to proceed?

[ahoppen]

Hey @TiagoMaiaL,
I replied to your comments on the two issues. I think both of them might behaving as expected. Regarding the Parse/ConditionalCompilation/language_version.swift failure is the following command crashing/hitting an assertion for you (I would expect that based on the test failure) or does it just output parser failures?

swift-parser-cli dump-parse path/to/swift/Parse/ConditionalCompilation/language_version.swift

[ahoppen]

@swift-ci Please test

[TiagoMaiaL]

Hey @TiagoMaiaL, I replied to your comments on the two issues. I think both of them might behaving as expected. Regarding the Parse/ConditionalCompilation/language_version.swift failure is the following command crashing/hitting an assertion for you (I would expect that based on the test failure) or does it just output parser failures?

swift-parser-cli dump-parse path/to/swift/Parse/ConditionalCompilation/language_version.swift

Hey @ahoppen, dump-parse is not a valid sub-command. I believe you meant print-tree, right? If I run this command on my branch, the parser hits the preconditionFailure on `Directives.swift, line 125:

          (unexpectedBeforePound, pound) = self.eat(poundIfHandle)
          firstIteration = false
          switch pound.tokenKind {
          case .poundIfKeyword, .poundElseifKeyword:
            // ...
          case .poundElseKeyword:
            // ...
          default:
            // pound.tokenKind is simply `#`.
            preconditionFailure("The loop condition should guarantee that we are at one of these tokens")
          }

This is happening when we parse the following source from that file:

let x: @#$()%&*)@#$(%&*

The parser interprets the # as a macro expansion, which I think is not right. The code then exits the elements parsing loop (on line 159), with the currentToken being a pound, which enters the preconditionFailure branch in the default switch case.

On main, the parser doesn't crash. Here's the relevant portion of the print-tree output:

    17: CodeBlockItemSyntax children=1
      0: StructDeclSyntax children=3
        0: keyword(SwiftSyntax.Keyword.struct)
        1: identifier("S")
        2: MemberDeclBlockSyntax children=3
          0: leftBrace
          1: MemberDeclListSyntax children=5
            0: MemberDeclListItemSyntax children=2
              0: IfConfigDeclSyntax children=2
                0: IfConfigClauseListSyntax children=2
                  0: IfConfigClauseSyntax children=3
                    0: poundIfKeyword
                    1: FunctionCallExprSyntax children=4
                      0: IdentifierExprSyntax children=1
                        0: identifier("swift")
                      1: leftParen
                      2: TupleExprElementListSyntax children=1
                        0: TupleExprElementSyntax children=1
                          0: PrefixOperatorExprSyntax children=2
                            0: prefixOperator(">=")
                            1: FloatLiteralExprSyntax children=1
                              0: floatingLiteral("2.2")
                      3: rightParen
                    2: MemberDeclListSyntax children=1
                      0: MemberDeclListItemSyntax children=1
                        0: VariableDeclSyntax children=2
                          0: keyword(SwiftSyntax.Keyword.let)
                          1: PatternBindingListSyntax children=1
                            0: PatternBindingSyntax children=2
                              0: IdentifierPatternSyntax children=1
                                0: identifier("x")
                              1: TypeAnnotationSyntax children=2
                                0: colon
                                1: SimpleTypeIdentifierSyntax children=1
                                  0: identifier("Int")
                  1: IfConfigClauseSyntax children=2
                    0: poundElseKeyword
                    1: MemberDeclListSyntax children=2
                      0: MemberDeclListItemSyntax children=2
                        0: VariableDeclSyntax children=2
                          0: keyword(SwiftSyntax.Keyword.let)
                          1: PatternBindingListSyntax children=1
                            0: PatternBindingSyntax children=2
                              0: IdentifierPatternSyntax children=1
                                0: identifier("x")
                              1: TypeAnnotationSyntax children=2
                                0: colon
                                1: AttributedTypeSyntax children=2
                                  0: AttributeListSyntax children=1
                                    0: AttributeSyntax children=2
                                      0: atSign
                                      1: MissingTypeSyntax
                                  1: MissingTypeSyntax
                        1: semicolon MISSING
                      1: MemberDeclListItemSyntax children=1
                        0: MacroExpansionDeclSyntax children=6
                          0: pound
                          1: UnexpectedNodesSyntax children=1
                            0: dollarIdentifier("$")
                          2: identifier("") MISSING
                          3: leftParen
                          4: TupleExprElementListSyntax
                          5: rightParen
                1: poundEndifKeyword MISSING
              1: semicolon MISSING
            1: MemberDeclListItemSyntax children=2
              0: FunctionDeclSyntax children=3
                0: keyword(SwiftSyntax.Keyword.func) MISSING
                1: binaryOperator("%&*")
                2: FunctionSignatureSyntax children=1
                  0: ParameterClauseSyntax children=3
                    0: leftParen MISSING
                    1: FunctionParameterListSyntax
                    2: rightParen
              1: semicolon MISSING
            2: MemberDeclListItemSyntax children=2
              0: MissingDeclSyntax children=1
                0: AttributeListSyntax children=1
                  0: AttributeSyntax children=2
                    0: atSign
                    1: MissingTypeSyntax
              1: semicolon MISSING
            3: MemberDeclListItemSyntax children=2
              0: MacroExpansionDeclSyntax children=6
                0: pound
                1: UnexpectedNodesSyntax children=1
                  0: dollarIdentifier("$")
                2: identifier("") MISSING
                3: leftParen
                4: TupleExprElementListSyntax children=1
                  0: TupleExprElementSyntax children=1
                    0: MissingExprSyntax
                5: rightParen MISSING
              1: semicolon MISSING
            4: MemberDeclListItemSyntax children=1
              0: FunctionDeclSyntax children=3
                0: keyword(SwiftSyntax.Keyword.func) MISSING
                1: binaryOperator("%&*")
                2: FunctionSignatureSyntax children=1
                  0: ParameterClauseSyntax children=3
                    0: leftParen MISSING
                    1: FunctionParameterListSyntax
                    2: rightParen MISSING
          2: rightBrace MISSING
  1: UnexpectedNodesSyntax children=20
    0: poundEndifKeyword
    1: rightBrace
    2: poundIfKeyword
    3: identifier("swift")
    4: leftParen
    5: prefixOperator(">=")
    6: floatingLiteral("2.2")
    7: rightParen
    8: keyword(SwiftSyntax.Keyword.var)
    9: identifier("zzz")
    10: equal
    11: stringQuote
    12: stringSegment("zzz")
    13: stringQuote
    14: poundElseKeyword
    15: keyword(SwiftSyntax.Keyword.var)
    16: identifier("zzz")
    17: equal
    18: identifier("zzz")
    19: poundEndifKeyword
  2: eof

[ahoppen]

Yes, that makes sense to me. I think it’s fine that the parser parsers the # as a macro expansion. There isn’t really any better way to parse it.

The problem here is that you have loosened the loop condition by adding pound to IfConfigContinuationClauseStartKeyword and now the precondition is wrong just as it’s saying. It might be sufficient to just break out of the loop for case .pound because you have handled the #elif case above and we haven’t consumed any tokens when we break when we’re at pound.

So, I think you would need to add a label LOOP before while let in line 109 and the following to the switch.

case .pound:
  break LOOP

I haven’t tested it though.

[TiagoMaiaL]

Hey @ahoppen, thank you for the help. I did as you suggested, and tests are passing. I noted, however, that the swift-parser-cli reduce command prints the following on my branch:
#if@#%

As opposed to main, which prints the following:
Error: Source file passed to reduce subcommand does not fail to roundtrip

After investigating a little bit, if I assign condition to nil in the case .pound, the parser round trips:

          switch pound.tokenKind {
          case .poundIfKeyword, .poundElseifKeyword:
            // ...
          case .poundElseKeyword:
            // ...
          case .pound:
            condition = nil
          default:
            preconditionFailure("The loop condition should guarantee that we are at one of these tokens")
          }

I'm not sure if this is ideal, though. Any guidance on how to fix this is appreciated.

[ahoppen]

What you are describing sounds as expected.

swift-parser-cli reduce will only do something useful to a test that crashes the compiler or fails to round-trip. If this is the case, it produces a smaller test case that has the same problem. So when swift-parser-cli reduce outputs Error: Source file passed to reduce subcommand does not fail to roundtrip on main, it’s telling you that it didn’t find one of these issues.

With your branch, it created the reduced #if@#% test case. You should be able to add an AssertParse test case to the test suite for that. It doesn’t really matter what diagnostics that test case produces (since it’s just a silly example) as long as it doesn’t crash the parser or fail to round-trip.

[ahoppen]

Oh, I see now. The real problem here is that the condition of the while loop is just really complicated. If it was just a while let poundIfHandle = self.canRecoverTo, we could switch over the returned match and eat the handle inside each case , like we do everywhere else in the parser.

Could you try the following?

• As a first step, try refactoring the pound if directive parsing to simplify the condition (not taking care of #elif yet)
  • Add poundIf too IfConfigContinuationClauseStartKeyword (and rename that type appropriately)
  • Remove the ternary operator from the loop and just have while let (match, handle) = self.canRecoverTo(….)
  • Switch over the match and push self.eat into each case
  • If we see a #if but firstIteration == false, we should break out of the loop without consuming #if
• Once that’s passing, you should be able to add pound to IfConfigContinuationClauseStartKeyword (or whatever you renamed it to before) and perform the atElifTypo check there.

[ahoppen]

Thanks. That looks a lot cleaner to me now. I’ve got a few minor comments inline. If tests are passing, I think we’re getting close to merging this now.

[ahoppen]

Two minor comments, then this will be ready to go 🚀

[ahoppen]

Let’s run it through CI again 🤞🏽

[ahoppen]

@swift-ci Please test

[TiagoMaiaL]

I was checking the causes for the pipeline failures:

1. Linux pipeline - It says fetching swift-argument-parser failed:
   

2. MacOS pipeline - It says one of the tests is failing, but all of them are passing locally 😅:
   

I've rebased my branch on top of latest main, and tests are still passing.

@ahoppen Do you know what might be happening?

[ahoppen]

CI is running a few additional tests that are disabled locally by default because they have significant performance impact on the parser. Essentially what these tests do, is to modify the source passed to assertParse in interesting™ ways and making sure we don’t hit assertions or round-trip failures after that. (#1340 has some information). The easiest way to enable them in Xcode is to change Package.swift as follows

--- a/Package.swift
+++ b/Package.swift
@@ -22,14 +22,14 @@ if ProcessInfo.processInfo.environment["SWIFT_BUILD_SCRIPT_ENVIRONMENT"] != nil
     ]),
   ]
 }
-if ProcessInfo.processInfo.environment["SWIFTSYNTAX_ENABLE_RAWSYNTAX_VALIDATION"] != nil {
+if ProcessInfo.processInfo.environment["SWIFTSYNTAX_ENABLE_RAWSYNTAX_VALIDATION"] != nil || true {
   swiftSyntaxSwiftSettings += [
     .define("SWIFTSYNTAX_ENABLE_RAWSYNTAX_VALIDATION")
   ]
 }
 
 var swiftParserSwiftSettings: [SwiftSetting] = []
-if ProcessInfo.processInfo.environment["SWIFTPARSER_ENABLE_ALTERNATE_TOKEN_INTROSPECTION"] != nil {
+if ProcessInfo.processInfo.environment["SWIFTPARSER_ENABLE_ALTERNATE_TOKEN_INTROSPECTION"] != nil || true {
   swiftParserSwiftSettings += [
     .define("SWIFTPARSER_ENABLE_ALTERNATE_TOKEN_INTROSPECTION")
   ]

If you then run testBogusSwitchStatement without SKIP_LONG_TESTS=1 you will hit the assertion failure. In one of the stack frames further down you can print the source code that hits the assertion using e print(mutatedSource). In this case it’s the following (which you can probably reduce a bit further using swift-parser-cli reduce).

switch x {
  #()
#if true
  bar()
#endif
  case .A, .B:
    break
}

[TiagoMaiaL]

Thanks for the information, @ahoppen. I did some comparisons between main and my branch, using a test case involving the code you shared, and here's what happens:

1. on main, the tokens #() are parsed as unexpectedBeforePoundKeyword
2. on my branch, these tokens (#()) are skipped due to the break LOOP on line 159, inside case .pound (after checking if it was the firstIteration)

If I try to recover by parsing poundIfKeyword, it works normally. So I was considering two approaches:

1. Having the case .poundIfKeyword as the last case in the switch, and then fallthrough to it, instead of breaking the loop in the .pound case
2. Having an internal function that tries to parse the initial .poundIfKeyword when we hit the other cases, if firstIteration is true

My understanding is limited, so I would like to see if you have some suggestions, if possible.

I'll try to run the tests the way you suggested. When you say "running without SKIP_LONG_TESTS=1", where do I set this configuration?

[ahoppen]

Oh, I see what’s going wrong. My suggestion of how to unify the loop condition was simply wrong. I’m sorry for that. But I’m making up suggestions of how to fix this as we go as well and don’t know the final outcome myself…

Your analysis was correct, in parseSwitchCases we assume that we will make progress because we can recover to a #if but then we bail out of parsePoundIfDirective without consuming anything because we execute break LOOP after hitting the standalone #.

---

I did some testing myself now and am fairly confident that the following should now really work (it does in my little prototype implementation I did locally at least). The idea is to just handle all the parsing of the initial #if block before the loop. That way the first call inside parsePoundIfDirective will be self.expect(.poundIf), which will guarantee to make progress as expected by parseSwitchCases:

• I would extract the parsing of the elements into a standalone function parseIfconfigClauseElements. That way it can be shared between the parsing of the initial #if block and any consecutive blocks https://github.com/apple/swift-syntax/blob/36b0f23c4b0a3baa928b71aeacf08c17e998838a/Sources/SwiftParser/Directives.swift#L129-L141
• Start by self.expect(.poundIfKeyword) followed by parsing the condition and the elements (using the extracted function), outside the loop and add the first element to clauses
• For the loop, remove #if from PoundIfDirectiveKeywords again, since it’s handled before now (and rename that type back)
• And since you are touching this already: As you encounter do statements, you can just remove them and flatten their contents into the surrounding function. I don’t think they provide any benefit here.

---

I'll try to run the tests the way you suggested. When you say "running without SKIP_LONG_TESTS=1", where do I set this configuration?

https://github.com/apple/swift-syntax/blob/main/CONTRIBUTING.md#xctests under Tip has some documentation on this environment variable. But it looks like you haven’t set it so far, so there’s nothing you needed to change.

[TiagoMaiaL]

I did as you suggested. I've extracted that new elements parsing method to be out of the parsePoundIfDirective one. This made me change the atElifTypo computed variable to be a method as well. If this was not what you had in mind, I can change these two to be contained inside parsePoundIfDirective.

Tests are passing, at least locally (including running them with the assertParse transformations option).

[ahoppen]

Nice. This reads so much better better with the #if parsing extracted than what we currently have in main. 😍

[ahoppen]

@swift-ci Please test

[ahoppen]

@swift-ci Please test Windows

[TiagoMaiaL]

@ahoppen If I'm not mistaken, the CI failures don't seem related to the changes in this PR. Is there anything I can do?

[ahoppen]

Oh, yes. I think we just hit bad timing where CI picked up one of the PRs related to #1496 but not the other one. I’ll just trigger CI again.

[ahoppen]

@swift-ci Please test macOS

[ahoppen]

@swift-ci Please test Windows