Handle missing comma i func params

[kimdv]

I saw this PR: #886

Then I tought I would finish it 😄

[ahoppen]

Thanks for picking up the PR, Kim!

[ahoppen]

I’m fixing the license header check failure here: swiftlang/github-workflows#105

[kimdv]

    assertParse(
      "func f(_: any borrowing 1️⃣~Copyable) {}",
      diagnostics: [
        DiagnosticSpec(
          message: "unexpected code '~Copyable' in parameter clause"
        )
      ]
    )

This test is failing @ahoppen.

failed - Expected 1 diagnostics but received 2:
1:24: expected ',' in parameter
1:25: expected identifier and ':' in parameter

Not sure how to handle this?

[kimdv]

$0.canParseCustomAttribute() return true for ~Copyable

[ahoppen]

Hmm, let’s just remove canParseCustomAttribute then. Attributes on parameters aren’t very common anyway.

[kimdv]

@ahoppen I added an self.at(.atSign) that fixes the issue.
I self only know @ attributes for names. Is there others?

[ahoppen]

Oh, I just looked at canParesCustomAttribute and it assumes that the @ has already been consumed. So, self.at(.atSign) should be a consume(if:).

And we should probably check for the at sign first because canParseTypeIdentifier can modify the Lookahead’s state.

[kimdv]

Changed it to consuming it.

[kimdv]

@swift-ci please test

[kimdv]

@swift-ci please test windows

[kimdv]

Not sure if there is any noteworthy performance hit here?
We have two withLookahead where we potentially only use canParseIdentifier.

Was considering

let canParseIdentifier: Bool = withLookahead {
  $0.canParseTypeIdentifier(allowKeyword: false)
}

if canParseIdentifier {
  trailingComma = Token(missing: .comma, arena: self.arena)
}

let canParseAttribute: Bool = withLookahead {
  $0.consume(if: .atSign) != nil && $0.canParseCustomAttribute()
}

if canParseAttribute {
  trailingComma = Token(missing: .comma, arena: self.arena)
}

[ahoppen]

I think your current implementation is good. This will only be hit for invalid code and we tend to not squeeze the last bit of performance for invalid code because it’s A LOT less common than valid code.

[kimdv]

@swift-ci please test

[kimdv]

@swift-ci please test macOS

[kimdv]

@swift-ci please test Windows

[ahoppen]

Just one nitpick. Feel free to merge after you addressed it.

[ahoppen]

I think your current implementation is good. This will only be hit for invalid code and we tend to not squeeze the last bit of performance for invalid code because it’s A LOT less common than valid code.

[kimdv]

@swift-ci please test

[kimdv]

@swift-ci please test windows

[kimdv]

@swift-ci please test macOS

[kimdv]

@swift-ci please test macOS

[kimdv]

@ahoppen not sure I know why it failed. Tried to dig in the logs, and I don't seem to find anything 🤔

[ahoppen]

Looks like a non-deterministic failure. Let’s try again.

@swift-ci Please test macOS

[kimdv]

@kimdv now it was something else that failed

It still seem unrelated?

Tests Times:
--------------------------------------------------------------------------
[    Range    ] :: [               Percentage               ] :: [   Count   ]
--------------------------------------------------------------------------
[1100s,1200s) :: [                                        ] :: [    1/18842]
[1000s,1100s) :: [                                        ] :: [    0/18842]
[ 900s,1000s) :: [                                        ] :: [    0/18842]
[ 800s, 900s) :: [                                        ] :: [    0/18842]
[ 700s, 800s) :: [                                        ] :: [    0/18842]
[ 600s, 700s) :: [                                        ] :: [    1/18842]
[ 500s, 600s) :: [                                        ] :: [    0/18842]
[ 400s, 500s) :: [                                        ] :: [    0/18842]
[ 300s, 400s) :: [                                        ] :: [    0/18842]
[ 200s, 300s) :: [                                        ] :: [    3/18842]
[ 100s, 200s) :: [                                        ] :: [   35/18842]
[   0s, 100s) :: [*************************************** ] :: [18802/18842]
--------------------------------------------------------------------------
********************
Failed Tests (1):
  Swift-validation(macosx-x86_64) :: Python/swift_build_support.swift

[kimdv]

@swift-ci please test macOS