[Parse] Provide a diagnostic when a closure parameter is declared with type sugar

[tapthaker]

Closure parameters are not allowed to be shadow sugared types. This PR tries to provide an error diagnostic for the same.

Resolves #54133.

[tapthaker]

@CodaFi A few questions regarding the PR:

1. Can you explain why closure parameters are not allowed to be shadow sugared types in the first place?
2. Are the optional tokens ( '?' & '!' ) the only things that should be checked for "sugared types"?

[CodaFi]

1. It’s an oversight in the logic we use for parsing parameters that this was ever allowed. We’ve historically been bad about this - the two-phase needlessly abstract parameter parsing logic is to blame in large part. We don’t allow just types in parameter position at all, and never have. But there is code that relies on this oversight in the wild.

2. Checking the type repr kind is the right idea if you want to emit a tailored diagnostic, but you shouldn’t have to inspect the token stream. You can use isa<T> instead of asking for the kind directly.

There’s a third concern here which is that this change is source-breaking. I am aware of at least one client that uses [Foo] as a type in parameter position in an overload set. This patch breaks them. I think we need to go to Swift Evolution about that last part, which is why I say this isn’t quite a starter bug.

[tapthaker]

There’s a third concern here which is that this change is source-breaking. I am aware of at least one client that uses [Foo] as a type in parameter position in an overload set. This patch breaks them. I think we need to go to Swift Evolution about that last part, which is why I say this isn’t quite a starter bug.

I don't think that this change is source-breaking. My understanding is that [Foo] type in parameter position would still pass just like before. We are only fixing the diagnostics for sugar types here, which used to fail before as well.

Please let me know if I misunderstood. I can take it up to swift-evolution if it's source-breaking.

[CodaFi]

Remember that overload resolution is diagnosing this situation today. The non-overloaded case sometimes compiles. The test only contains the overloaded case.

[tapthaker]

Got it, I didn't think about that. Cool, so I will start a discussion on swift-evolution soon.

[CodaFi]

Just one tiny little nit and I think this should be good to take. Thanks!

[tapthaker]

Remember that overload resolution is diagnosing this situation today. The non-overloaded case sometimes compiles. The test only contains the overloaded case.

@CodaFi: I am unable to create an example where the non-overloaded case would compile. Could you give it a try?

My example:

struct Bar {}

struct Foo {
  func registerCallback(_ callback: @escaping (Bar?) -> Void) {}
}

Foo().registerCallback { (Bar?) in }

Fails with:

/tmp/Hello.swift:9:30: error: expected ',' separator
Foo().registerCallback { (Bar?) in }
                             ^
                             ,
/tmp/Hello.swift:9:30: error: expected parameter name followed by ':'
Foo().registerCallback { (Bar?) in }

[CodaFi]

Oh, sorry, let me try to expand here:

The source break stems from the general fix to this bug, which is to make what is currently a warning in that test you just updated a hard error. There are known cases where type sugar in parameter position happens to compile, and the most egregious one I know of uses array sugar: https://swift.godbolt.org/z/gkqhCw. Even more distressingly, that one also accepts [Bar?]

Getting this through the source compatibility suite was insufficient to catch users of this the last time I tried, so my general fear is that we'll regress again. We need to go to evolution to get an up-or-down vote on whether we should be allowed to ban this, which will give us a clearer picture of whether we need to go fixup the failing projects, or continue to warn about them until we add a new language version or whatever.

[CodaFi]

@swift-ci please smoke test

[CodaFi]

@swift-ci please test source compatibility

[CodaFi]

Hm, these jobs never finished

@swift-ci please smoke test

[CodaFi]

@swift-ci please test source compatibility

[CodaFi]

You got a bad builder on macOS

@swift-ci please smoke test macOS platform

[CodaFi]

⛵️

[tapthaker]

@CodaFi did you folks internally conclude to merge this feature ?

Also, I am sorry I couldn’t spend enough time to create a Swift-evolution discussion around this.

[CodaFi]

I merged here because we’ve passed the 5.2 rebranch date. If this breaks any more adopters then we’ll have time to fix them and assess the scope of the issue. We still need to go to evolution about the broader underlying issue.

[tapthaker]

@CodaFi
Stated a discussion in the swift forums: suggestion-dont-allow-closure-parameters-to-shadow-types. Feel free to add more details there.