[Parse] Diagnose missing 'in' after closure signature

[kwikysw]

Teach the parser to recognize a top-level '->' in a closure as a signature and
produce a targeted diagnostic ("expected 'in' after the closure signature")
instead of treating it as a type expression.

Adds a parser test:

• test/Parse/closure-missing-in.swift

Resolves #59928

[kwikysw]

Hi all. This is a small parser diagnostic change (+ test),
opened over a week ago. CI is pending but green locally.
Happy to address any feedback. Thanks!

[rintaro]

Thank you for working on this!

Unfortunately this is source breaking. For example, consider { [x, y] }, this is currently a valid closure returning an array of two elements. However with your change, it would be parsed as a closure with a capture list and a missing in.

As I mentioned in #59928 (comment) you could check whether consumeIf(tok::arrow) above returned true, so we know the skipped tokens actually look like a closure signature (because there's no valid expression that contain -> at top-level). That won't resolve swiftlang/swift-syntax#2204 case, but it would at least address the cases covered by your tests.

[kwikysw]

Thanks for the guidance, as well as the advice about needing the tests for swift-syntax too. I’ll gate the diagnostic on seeing a top-level -> as you said, during speculative scan (so if consumeIf(tok::arrow) succeeds at depth 0 and canParseType() accepts the following type). if no top-level ->, we treat it as an expression so the { [x, y] } remains an array literal.

Could you please confirm if we should not use bare async/throws as a gate and that they're only considered when a top-level (not inside any nested (...)/[...]/{...}) -> is present. Also if -> is seen but canParseType() fails, we should bail out an expression to avoid false positives ?

[rintaro]

Could you please confirm if we should not use bare async/throws as a gate and that they're only considered when a top-level (not inside any nested (...)/[...]/{...}) -> is present.

I'd say no for async et al because async is a contextual keyword hence var async = 1; _ = { async } is valid. Although throws is a hard keyword, but handling differently between async and throws sounds weird.

Also if -> is seen but canParseType() fails, we should bail out an expression to avoid false positives ?

Yes, that sounds good to me. That keeps the current behavior and safer.

[rintaro]

Also you'd need a corresponding fix (and test cases) for https://github.com/swiftlang/swift-syntax repo.
Specifically Parser.Lookahead.canParseClosureSignature()

[kwikysw]

Hi @rintaro. I updated the patch so that we only treat it as a closure signature in lookahead when we see a top-level ->. Bare async/throws are not used as gates and if we don't see a top-level arrow, we bail to expression (restoring the previous makeParserSucess() behavior). I had to gate the new diagnostic behind isSwiftVersionAtLeast(5) given some parser tests run in swift 4 mode. If this looks correct, I'll follow up with the corresponding change + tests in swift-syntax's Parser.Lookahead.canParseClosureSignature.

[bnbarham]

I had to gate the new diagnostic behind isSwiftVersionAtLeast(5) given some parser tests run in swift 4 mode

What tests were failing here? And what was the failure? I wouldn't have expected needing to gate this at all 🤔

[kwikysw]

I had to gate the new diagnostic behind isSwiftVersionAtLeast(5) given some parser tests run in swift 4 mode

What tests were failing here? And what was the failure?

I misstated earlier, no parser tests broke semantically. The only failures were in a swift 4 mode verify test, not swift 5+. In test/Parse/type_expr.swift, it runs with RUN: %target-typecheck-verify-swift -swift-version 4 -module-name test and in testInvalidArrowWithClosure, the line _ = { undefined -> undefined2 } has the parser take the type-expression path and expects the old diagnostics:

• cannot find 'undefined' in scope
• cannot find 'undefined2' in scope
• expected type before '->'
• expected type after '->' but my change in lookahead sees a top-level '->' and treats this as a closure signature then emits "expected 'in' after closure signature".

[rintaro]

That sounds like expected changes. Please remove the Context.LangOpts.isSwiftVersionAtLeast(5) check and update the existing test cases as needed.

[rintaro]

Could you add some comment like:

// Even if 'in' is missing, the presence of '->' makes this look like a closure signature. // There’s no other valid syntax that could legally contain '->' at this position. 

(Please add newlines as needed, C++ code in this repository have 80 columns limit)

[kwikysw]

I've just made the requested changes, let me know if anything else is needed.

[rintaro]

@swift-ci Please smoke test

[rintaro]

@swift-ci Please smoke test

[rintaro]

The change looks fine to me, thank you!
Please go ahead and create corresponding swift-syntax PR.

[rintaro]

I don't think shouldTreatAsClosure is needed here. Let's just use sawTopLevelArrowInLookahead.

[kwikysw]

Yep, I've removed it. I'll create the swift-syntax PR.

[rintaro]

@swift-ci Please smoke test

[rintaro]

@swift-ci Please smoke test Windows

[rintaro]

@swift-ci Please smoke test Linux

[rintaro]

@swift-ci Please smoke test Windows

[rintaro]

@kwikysw This PR can be merged independently, but I'll hold off on merging it until the corresponding swift-syntax PR is up, to prevent divergence.

[kwikysw]

@kwikysw This PR can be merged independently, but I'll hold off on merging it until the corresponding swift-syntax PR is up, to prevent divergence.

I'm working on it right now, sorry for the delay

[rintaro]

Nothing to sorry, thank you for working on this!