Skip to content
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

Detect possibly non-Rust closure syntax during parse #47763

Closed
wants to merge 1 commit into from

Conversation

estebank
Copy link
Contributor

Other languages have a similar, but not quite the same to Rust, syntax
for closures:

{|<args>| <stmts> }

Even worse, the above code is valid Rust if and only if there's only
one statement, as the block would resolve to the single statement
closure within it. This means that seemingly correct code will fail when
adding a new statement, making modifying something that looks unrelated
cause "value not found in this scope" and "trait bound not satisfied"
errors. Because of this, make the parser detect the specific case where
the code is incorrect and failing, and emit a warning explaining the
correct syntax.

Fix #27300.

@estebank estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2018
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

node: StmtKind::Expr(expr), ..
}) = stmts.last() {
warn.span_note(block_sp, "...while this enclosing block...");
warn.span_note(expr.span, "...implicitely returns");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "implicitly".

Other languages have a similar, but not quite the same to Rust, syntax
for closures:

```
{|<args>| <stmts> }
```

Even worse, the above code *is* valid Rust if and only if there's only
one statement, as the block would resolve to the single statement
closure within it. This means that seemingly correct code will fail when
adding a new statement, making modifying something that looks unrelated
cause "value not found in this scope" and "trait bound not satisfied"
errors. Because of this, make the parser detect the specific case where
the code is incorrect *and* failing, and emit a warning explaining the
correct syntax.
@petrochenkov
Copy link
Contributor

This is a semantic error, so I'd rather not report it in the parser.
Hard-coded warnings should be avoided as well if possible.

Given that, I think there are two possibilities:

  • Attach the message to type errors and resolution errors if they originate from the suspicious closure-like block. That's kinda hard and doesn't worth the effort, IMO.
  • Make it an early warn-by-default lint, then the explanatory message will be shown even if there are resolution errors.

@petrochenkov
Copy link
Contributor

This is also somewhat a policy question, like @Xirdus mentioned in #27300 (comment).
There are large number of popular enough languages with variety of syntaxes, we should not necessarily try to parse them all and report custom messages, especially if it requires going out of our way somehow.
People with Ruby background is a common source of Rust newcomers... for reasons, so I think it would be probably okay if this special diagnostics are implemented, especially as a self-contained lint.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2018
@shepmaster
Copy link
Member

@petrochenkov I'm unclear on why you marked this as waiting on author — what kind of response are you expecting from @estebank? If this needs a "policy decision", then it seems like it needs to go to the appropriate team, instead.

@petrochenkov
Copy link
Contributor

@shepmaster

what kind of response are you expecting from @estebank?

Making this an early warn-by-default lint instead of a hardcoded warning in parser.

If this needs a "policy decision"

I'm not entirely sure this needs a larger policy decision than rubber-stamping from me, but let's tag it as compiler-team-nominated anyway.

@petrochenkov petrochenkov added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 3, 2018
@BatmanAoD BatmanAoD removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 7, 2018
@nikomatsakis
Copy link
Contributor

I'm going to clear the nominated flag. My feeling is that targeted errors of this kind are worth it, if they are not too intrusive -- it's hard to make a hard-and-fast rule, but I love the reputation of rustc as a "helpful compiler", and if we see a place that people stub their toes a lot, we should try to patch that. (In particular, I see no reason to pretend other languages don't exist.)

All that said, I don't have a strong opinion about whether this particular message should be given in the parser or a lint or what. I'd say that if the code does successfully parse, then some sort of lint seems like a better place for it.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 8, 2018
@pietroalbini
Copy link
Member

@estebank the reviewer asked to turn this into an early warn-by-default lint in #47763 (comment), could you do that?

@estebank
Copy link
Contributor Author

@pietroalbini I'll get back to this sometime this week to turn it into a lint.

@shepmaster
Copy link
Member

Would you also submit the opposite patch to Ruby 😇? Surprisingly, that's the biggest stumbling block I have switching between the two...

@pietroalbini
Copy link
Member

@estebank this PR has been stale for more than two weeks, so I'm closing it. If you have time to work on this again in the future feel free to reopen it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit warnings on parameter list in closures after {
9 participants