Generate token spec sets

[kimdv]

Context: #1727 (comment)

[kimdv]

@ahoppen opened this PR to start a discussion if I'm on the right way. Added someinline comments.

I would also like a first round of some initial toughts of yours

[kimdv]

I found that this doesn't contain the same as in AvailabilityArgumentKind

It have also a special handling

https://github.com/apple/swift-syntax/blob/5474478ccf6bd3319c5c3564d2eed15dc3e45eac/Sources/SwiftParser/Availability.swift#L57-L96

[ahoppen]

These are exactly the interesting cases. A short-term solution would probably be to define the case from AvailabilityArgumentKind that aren’t in AvailabilityLabeledArgumentLabel in a separate TokenSpecSet and then use self.at(anyIn:or:) in Availability.swift:104. In general for these cases, we might need to consider if there is a bug in the parser or if we can model the syntax tree better so that the generated TokenSpecSet matches the parser.

[ahoppen]

Thank you. This is great. I wonder how many previously handwritten TokenSpecSet we can replace by the generated ones.

I also wonder whether we can use the generated TokenSpecSet in RawSyntaxValidation.swift.

[ahoppen]

These are exactly the interesting cases. A short-term solution would probably be to define the case from AvailabilityArgumentKind that aren’t in AvailabilityLabeledArgumentLabel in a separate TokenSpecSet and then use self.at(anyIn:or:) in Availability.swift:104. In general for these cases, we might need to consider if there is a bug in the parser or if we can model the syntax tree better so that the generated TokenSpecSet matches the parser.

[ahoppen]

Looks good to me. Only one comment inline.

CC @bnbarham because we talked about this at some point as well.

[kimdv]

@ahoppen we get this TokenSpecSet

extension SimpleTypeIdentifierSyntax {
  enum NameOptions: TokenSpecSet {
    case identifier
    case `self`
    case `Self`
    case `Any`
    case keyword
    case wildcard
    
    init?(lexeme: Lexer.Lexeme) {
      switch PrepareForKeywordMatch(lexeme) {
      case TokenSpec(.identifier):
        self = .identifier
      case TokenSpec(.`self`):
        self = .`self`
      case TokenSpec(.`Self`):
        self = .`Self`
      case TokenSpec(.`Any`):
        self = .`Any`
      case TokenSpec(.keyword):
        self = .keyword
      case TokenSpec(.wildcard):
        self = .wildcard
      default:
        return nil
      }
    }
    
    var spec: TokenSpec {
      switch self {
      case .identifier:
        return .identifier
      case .`self`:
        return .keyword(.`self`)
      case .`Self`:
        return .keyword(.`Self`)
      case .`Any`:
        return .keyword(.`Any`)
      case .keyword:
        return .keyword
      case .wildcard:
        return .wildcard
      }
    }
  }
}

The . keyword is not valid TokenSpec.
What do you think we should do?
Omit it?

This is the node

Node(
    kind: .simpleTypeIdentifier,
    base: .type,
    nameForDiagnostics: "type",
    children: [
      Child(
        name: "Name",
        kind: .token(choices: [
          .token(tokenKind: "IdentifierToken"),
          .keyword(text: "self"),
          .keyword(text: "Self"),
          .keyword(text: "Any"),
          .token(tokenKind: "KeywordToken"),
          .token(tokenKind: "WildcardToken"),
        ]),
        classification: "TypeIdentifier"
      ),
      Child(
        name: "GenericArgumentClause",
        kind: .node(kind: .genericArgumentClause),
        isOptional: true
      ),
    ]
  )

[ahoppen]

This is the solution: #1769

[kimdv]

I found very few usage of the new generated spec sets that matched 1-1.
I can try to find more.

And second, I found also some cases where there was a mismatch between the generated ones and the handwritten ones.
Should I try to apply your suggestion above, so we can see how many cases there are so we need what to do?

[ahoppen]

I found very few usage of the new generated spec sets that matched 1-1.

🤩 Nice. Those were more than I expected, honestly. I like it.

And second, I found also some cases where there was a mismatch between the generated ones and the handwritten ones.
Should I try to apply your suggestion above, so we can see how many cases there are so we need what to do?

Trying to move more parser calls to the generated TokenSpecSet would be great. I’d just like to do it in a follow-up PR do make it easier to review.

[ahoppen]

Do you know where these changes come from?

[kimdv]

#1781 (comment)

[bnbarham]

Hmm... maybe my local CodeGen did not resolve the HEAD correctly. Will try tomorrow again

I run into this as well @kimdv but apparently @ahoppen does not 🤷. I just delete .build and Package.resolved each time.

[kimdv]

I normally also run swift package update and swift package resolve each time.
But forgot it 😅

But thanks for the tip!

Should we file a bug in spm repo?
It would be nice if it checked that automatically.

[ahoppen]

Does swift package update and swift package resolve always fix the issue for you? Recently I didn’t have the feeling but I think I only ever ran one of the two commands, not both.

I think it’s expected that you need to run swift package update. That matches the SwiftPM behavior when you depend on branches where you also only want to update your dependencies (and thus Package.resolved) when you explicitly request it. We should add that to our documentation.

If swift package resolve is also necessary, I would say that’s a but in SwiftPM. If you could file an issue for that, that’d be great.

[bnbarham]

Hmmm. Just had a look at the docs, maybe swift package resolve is what we should be running? swift package update will update Package.resolved with the new hash and then swift package resolve would update the packages to match that version:

The swift package resolve command resolves the dependencies, taking into account the current version restrictions in the Package.swift manifest and Package.resolved resolved versions file, and issuing an error if the graph cannot be resolved. For packages which have previously resolved versions recorded in the Package.resolved file, the resolve command will resolve to those versions as long as they are still eligible. If the resolved version's file changes (e.g., because a teammate pushed a new version of the file) the next resolve command will update packages to match that file. After a successful resolve command, the checked out versions of all dependencies and the versions recorded in the resolved versions file will match. In most cases the resolve command will perform no changes unless the Package.swift manifest or Package.resolved file have changed.

But having said that:

Most SwiftPM commands will implicitly invoke the swift package resolve functionality before running, and will cancel with an error if dependencies cannot be resolved.

So maybe it's a bug if we have to run it, unsure.

[kimdv]

@swift-ci please test

[kimdv]

@swift-ci please test windows

[kimdv]

@swift-ci please test

[kimdv]

Will remove this @ahoppen as the parser fails in some test cases.
We are now allowed to provide e as a label.

Will be added in #1727

[kimdv]

@swift-ci please test

[kimdv]

@swift-ci please test windows