Add checking for unnecessary delims in closure body by chenyukang · Pull Request #136906 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
r? @jieyouxu
rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
| if matches!(closure.fn_decl.output, FnRetTy::Default(_)) |
|---|
| // skip `#[core::contracts::requires(...)]` and `#[core::contracts::ensures(...)]` which generate closure |
| && !cx |
| .sess() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any better check for this?
seems the AST generated by contracts are the same with normal closures.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could generate an #[allow] for this lint as part of the ast we generate. Alternatively you could check that it's expanded code in general and just ignore all of this. Likely some macros will hit this issue, too (add a test for that, too)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already checked it?
ah that means contract expansion doesn't add the expansion info to the spans. I did indeed forget to check for that at all when it was introduced
cc @celinval
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right this is due to the macro expansion from
| fn expand_contract_clause( |
|---|
maybe the span can be annotated there?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to follow mark_with_reason https://github.com/chenyukang/rust/blob/59281e98d0af7ed5071d141235ce5ddff1f60253/compiler/rustc_span/src/hygiene.rs#L906-L919
to create a span with annotation:
59281e9#diff-0843065cebe8379cca975e505c67c393f9933c8d9019fb6a6e3b3d5654157bc1R70
The problem is how can we get a ctx which meet the trait here, seems we're at a very early stage:
error[E0277]: the trait bound SyntaxContext: rustc_span::HashStableContext is not satisfied
--> compiler/rustc_builtin_macros/src/contracts.rs:70:49
|
70 | let expn_id = LocalExpnId::fresh(expn_data, attr_span.ctxt());
| ------------------ ^^^^^^^^^^^^^^^^ the trait rustc_span::HashStableContext is not implemented for SyntaxContext
| |
| required by a bound introduced by this call
|
note: required by a bound in LocalExpnId::fresh
--> /Users/yukang/rust/compiler/rustc_span/src/hygiene.rs:200:53
|
200 | pub fn fresh(mut expn_data: ExpnData, ctx: impl HashStableContext) -> LocalExpnId {
| ^^^^^^^^^^^^^^^^^ required by this bound in LocalExpnId::fresh
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right. Sorry for holding it up
This comment has been minimized.
This comment has been minimized.
Some changes occurred to MIR optimizations
cc @rust-lang/wg-mir-opt
Some changes occurred in coverage instrumentation.
cc @Zalathar
This comment has been minimized.
The Miri subtree was changed
cc @rust-lang/miri
Some changes occurred in src/tools/clippy
cc @rust-lang/clippy
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a lot of changes in unrelated tests. Changing tests for such reasons isn't too great and the value of this change in tests isn't really there either. Maybe add the lint to the list of lints that compiletest allows for all tests?
This comment has been minimized.
That's a lot of changes in unrelated tests. Changing tests for such reasons isn't too great and the value of this change in tests isn't really there either.
IMO we may want to just not warn on this case. Looking at the diffs both in the compiler and in the tests, it doesn't seem like a strict readability improvement to me (at times I find it even a bit harder to read). unused_parens is also a warn-by-default lint, so we'd be warning on a lot of existing code (judging by quite a few instances of compiler code getting newly linted on when we expand this lint).
Maybe add the lint to the list of lints that compiletest allows for all tests?
I think we should avoid adding blanket allows for all tests where feasible (many tests getting affected is itself a decent indicator, albeit rough, for if a change may impact a lot of cases).
Looking at the diffs both in the compiler and in the tests, it doesn't seem like a strict readability improvement to me (at times I find it even a bit harder to read)
in the compiler and tools it was always an improvement to me. What specific examples do you dislike?
in the compiler and tools it was always an improvement to me. What specific examples do you dislike?
This probably comes down to personal preference, but I don't really find these specific examples to be strictly better (I find the { } helps with visual separation of the body expr. I don't feel too strongly about this so 🤷
-.all(|(node, span_edge)| { span_edge.is_some() <= self.is_supernode(node) }), +.all(|(node, span_edge)| span_edge.is_some() <= self.is_supernode(node)),
-write!(&mut counter, "{:#}", fmt::from_fn(|f| { self.inner_full_print(None, f, cx) })) +write!(&mut counter, "{:#}", fmt::from_fn(|f| self.inner_full_print(None, f, cx)))
Removing parens on the very short exprs do look more readable to me. So I guess I feel mixed about it? lol
Since you're already looking at it... r? @oli-obk
Area: The compiletest test runner
Area: The testsuite used to check the correctness of rustc
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
Some changes occurred in src/tools/compiletest
cc @jieyouxu
This comment has been minimized.
that's a lot of changes in unrelated tests. Changing tests for such reasons isn't too great and the value of this change in tests isn't really there either. Maybe add the lint to the list of lints that compiletest allows for all tests?
done with commit 3a34ad7
Yes, this seems comes down to personal preference, but I just found rustfmt will remove the braces in this closure:
let some_predicate = |x: &mut i32| { *x == 2 || *x == 3 || *x == 6 };
these two lines [...] are quite different
Wait, what?^^ Could you spell this out / link to some place where it is spelled out?
E.g.:
fn main() {
let x = &mut ();
_ = move || { let x = x; }.clone(); //~ OK
let x = &mut ();
_ = move || -> _ { let x = x; }.clone();
//~^ ERROR the trait bound &mut (): Clone is not satisfied
}
The first one clones unit. The second one tries to clone the closure.
That is, if the return type is not annotated, then we parse an expression. If it is, then we require an explicit block.
Oh wow... That is subtle. But one case does not build so at least it does not lead to semantic ambiguity.
Certainly if we try hard enough, we can produce that. E.g.:
struct W(u8); impl Clone for W { fn clone(&self) -> Self { W(1) } }
fn main() { let f = move |x: W| { x }.clone(); assert!(matches!(f(W(0)), W(1))); let f = move |x: W| -> _ { x }.clone(); assert!(matches!(f(W(0)), W(0))); }
There are probably other ways.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
labels
This comment has been minimized.
@rfcbot concern ambiguous-block-parsing
Thinking more about this, if someone writes an apparently redundant set of braces for the body of a closure without an ascribed return type but where a second block starts immediately inside the first but ends before the first block does, e.g.,
_ = || { { 0 }.clone() };
then I don't want to suggest removing those outer braces. In fact, I'd like to lint against their absence and suggest adding them, e.g.:
_ = || { 0 }.clone(); //~ WARN
The reason this case is such a problem is that it'd be pretty reasonable for people to expect that the opening brace at the start of the closure body would force block parsing of that body. If it did, then the closing brace would end the closure body (as it does for -> _ closures). In my view, it's worth warning anywhere this reasonable expectation would be violated -- we at least shouldn't push people away from this disambiguation -- and I'm interested in whether we might be able to improve this behavior over an edition.
for this code:
_ = || { { 0 }.clone() };
our current rustfmt will remove the outer brace automatically.
cc @rust-lang/rustfmt @rust-lang/style
@traviscross is there a heuristic we can reliably use to force rustfmt not to remove the surrounding blocks? If so, then we could try to modify is_block_closure_forced_inner in rustfmt, though I think we might need to gate the change unless we're considering this a bugfix for rustfmt.
| fn is_block_closure_forced(context: &RewriteContext<'_>, expr: &ast::Expr) -> bool { | |
|---|---|
| // If we are inside macro, we do not want to add or remove block from closure body. | |
| if context.inside_macro() { | |
| false | |
| } else { | |
| is_block_closure_forced_inner(expr, context.config.style_edition()) | |
| } | |
| } | |
| fn is_block_closure_forced_inner(expr: &ast::Expr, style_edition: StyleEdition) -> bool { | |
| match expr.kind { | |
| ast::ExprKind::If(..) | ast::ExprKind::While(..) | ast::ExprKind::ForLoop { .. } => true, |
| ast::ExprKind::Loop(..) if style_edition >= StyleEdition::Edition2024 => true, | |
| ast::ExprKind::AddrOf(_, _, ref expr) | |
| | ast::ExprKind::Try(ref expr) | |
| | ast::ExprKind::Unary(_, ref expr) | |
| | ast::ExprKind::Cast(ref expr, _) => is_block_closure_forced_inner(expr, style_edition), | |
| _ => false, | |
| } | |
| } |
This comment was marked as duplicate.
Probably what we'd want is something along the lines of, "if the expression forming the closure body starts with two opening braces in sequence, and then ends with a closing brace (matching the outer starting brace) without an immediately preceding inner closing brace (matching the inner starting brace), then don't remove the outer braces."
That is, we're looking for:
_ || { { .. }/* Something here */ };
If we were to add disambiguating braces, the rule would be along the lines of, "if the expression forming the closure body starts with an opening brace and does not end with a closing brace (matching the opening one),then wrap the expression in a set of outer braces.
I'm pretty sure that rustfmt only removes the outer braces if the closure body contains a single expression, and doesn't contain any comments. The only way I can think to write code that starts with two opening braces and ends with a single closing brace is in this case of a method call receiver being a block expression. If you can think of other ways we might end up with a pattern like || { { .. }/* Something here */ }; let me know.
For this case, I think something like this could work:
fn is_block_closure_forced_inner(expr: &ast::Expr, style_edition: StyleEdition) -> bool { match expr.kind {
ast::ExprKind::MethodCall(ref method_call)if style_edition >= StyleEdition::Edition2027 =>{// Prevent rustfmt from removing outer braces in `_ = || { { 0 }.clone() };`,// which changes the semantics of the code.matches!(method_call.receiver.kind, ast::ExprKind::Block(..))} ast::ExprKind::If(..) | ast::ExprKind::While(..) | ast::ExprKind::ForLoop { .. } => true, ast::ExprKind::Loop(..) if style_edition >= StyleEdition::Edition2024 => true, ast::ExprKind::AddrOf(_, _, ref expr)
With that in place the following
fn main() { _ = || { { 0 }.clone() }; }
is formatted like this.
fn main() { _ = || { { 0 }.clone() }; }
Probably what we'd want is something along the lines of, "if the expression forming the closure body starts with two opening braces in sequence, and then ends with a closing brace (matching the outer starting brace) without an immediately preceding inner closing brace (matching the inner starting brace), then don't remove the outer braces."
That is, we're looking for:
_ || { { .. }/* Something here */ };
If we were to add disambiguating braces, the rule would be along the lines of, "if the expression forming the closure body starts with an opening brace and does not end with a closing brace (matching the opening one),then wrap the expression in a set of outer braces.
I plan to change this PR to not remove outer braces, but only remove unnecessary outer parens for closure (like the scenario from the original issue #136741). is it OK?
I plan to change this PR to not remove outer braces, but only remove unnecessary outer parens for closure (like the scenario from the original issue #136741). is it OK?
Yes, thanks; that sounds OK.
Labels
Area: The compiletest test runner
Area: The testsuite used to check the correctness of rustc
This issue / PR is in PFCP or FCP with a disposition to merge it.
Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination
Nominated for discussion during a lang team meeting.
Proposed to merge/close by relevant subteam, see T- label. Will enter FCP once signed off.
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Relevant to the language team, which will review and decide on the PR/issue.
Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.