[SR-11724] Provide a diagnostic when a closure parameter is declared with type sugar

[CodaFi]

Previous ID	SR-11724
Radar	rdar://problem/56950572
Original Reporter	@CodaFi
Type	Bug

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, DiagnosticsQoI, StarterBug
Assignee	None
Priority	Medium

md5: 48f0d1bec40f4cb6b0df056014613eb7

Issue Description:

Closure parameters are already allowed to shadow types. What they are not allowed to do is shadow sugared types, or attempt to destructure them:

struct Bar {}

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

Foo().registerCallback { ([Bar]) in }
Foo().registerCallback { ([String: Bar]) in }
Foo().registerCallback { (Bar?) in }

This emits a pile of diagnostics, and none of them are good. We should be able to catch this during Parse and emit a much better diagnostic. Ideally one that inserts what the user probably meant

Foo().registerCallback { (_: [Bar]) in }
Foo().registerCallback { (_: [String: Bar]) in }
Foo().registerCallback { (_: Bar?) in }

[CodaFi]

@swift-ci create

[CodaFi]

Tagging this as a starter bug, but it's kind of an open-ended intermediate bug. The parsing logic for ParamDecls is a mess. But we should at least be emitting some kind of diagnostic consistently when we come across these kinds of parameters. Today, it's up to overload resolution to make sure this is rejected, which is way too late.

[michaelleejl]

Hey @CodaFi - I'd like to start contributing to Swift.

I think I'll need some initial orientation. Where is the parsing logic for ParamDecls? I've had a brief look at ParsePattern.cpp, which looks like a good place to start digging.

Full disclosure: I'm in university right now, so progress might be quite bursty.

If this is ok, I'm wondering if I could be assigned this issue? Thanks!

[AnthonyLatsis]

@michaelleejl Would you be interested in addressing this in the new parser instead, which is written in Swift? Diagnostic improvements to the old parser are somewhat undesirable at this point because we would still have to pick them over to the new parser before shipping it.

---

Interestingly, the old parser accepts the following case with a warning, and rejects the Int? case:

func test(_: ([Int]) -> Void) {}
test { ([Int]) in } // warning: unnamed parameters must be written with the empty name '_' [parameter_unnamed_warn]

The new parser rejects them all with e.g. error: unexpected code '[Int]' in parameter clause.

@ahoppen Is this expected? If it is, we should modify the warning message to say that it will become an error in, I suppose, Swift 6.

[michaelleejl]

@AnthonyLatsis Yeah sure that makes sense! Happy to work on the new parser. Where do I find that?

[AnthonyLatsis]

The new parser lives in the swift-syntax repository. Alex can transfer the issue once they get to it.

[michaelleejl]

(Very low priority) Out of curiosity, I see that the parser is a recursive descent parser. Is there a reason this is done, instead of say an LR parser? I assume there are no left-recursive productions in the grammar?

[ahoppen]

Is this expected? If it is, we should modify the warning message to say that it will become an error in, I suppose, Swift 6.

That’s a bug. I filed swiftlang/swift-syntax#2331.

(Very low priority) Out of curiosity, I see that the parser is a recursive descent parser. Is there a reason this is done, instead of say an LR parser? I assume there are no left-recursive productions in the grammar?

I have never implemented an LR parser but my impression has always been that recursive decent parsers are a lot easier to maintain and it’s a lot easier to recover from parsing errors than with LR parsers. @CodaFi Do you have anything to add? Did we ever consider an LR parser for Swift?

[CodaFi]

I don't think we really considered some underlying formal model for the parser when we brought up either the C++ or the Swift parsers. I think a lot of the C-family compiler folk have settled naturally on recursive descent for its ease of maintainability and the ability to more easily interleave diagnostics with the natural flow of control while parsing. (Now, for the Swift parser where we take pains to separate the diagnostics pass from the parsing pass, this "interleaving problem" is a moot point). It's also worth noting we're not actually sure what a purely properly formal model of Swift's grammar is! The grammar outlined in the Swift book, for example, is descriptive, not prescriptive. If you translated it line for line, you would certainly parse a subset of Swift. But as authors of parsers for programming languages of any notable size inevitably come to discover: the parser accepts a larger grammar than its authors intend.

And this is not always a bad thing! I consider it table stakes for a parser for a modern programming language to accept a larger "semantic shadow grammar" than the formal language spec to enable users in the throes of software development to have decent errors in their IDEs of choice. For a recursive descent parser, it's far easier to carve out those extra niches than in my experience with LR or other kinds of (not always, but frequently) table/tool-driven approaches.