New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SR-11724] Provide a diagnostic when a closure parameter is declared with type sugar #54133
Comments
@swift-ci create |
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. |
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 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! |
@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 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. @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. |
@AnthonyLatsis Yeah sure that makes sense! Happy to work on the new parser. Where do I find that? |
The new parser lives in the swift-syntax repository. Alex can transfer the issue once they get to it. |
(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? |
That’s a bug. I filed apple/swift-syntax#2331.
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? |
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. |
This happens to be fixed in the old parser (see linked PR). For example:
Let us close this issue and use apple/swift-syntax#2340 to track the analogous problem in the new parser. |
Additional Detail from JIRA
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:
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
The text was updated successfully, but these errors were encountered: