Add checking for unnecessary delims in closure body

[chenyukang]

Fixes #136741

[rustbot]

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

[chenyukang]

any better check for this?
seems the AST generated by contracts are the same with normal closures.

[oli-obk]

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)

[chenyukang]

Alternatively you could check that it's expanded code in general and just ignore all of this.

The following check here https://github.com/rust-lang/rust/pull/136906/files#diff-e25b2c6c1fa434ab8078db81003888370f06b9da5503d1590757350be8e31103L945-L946 already checked it?

I also tried to use closure.body.span.can_be_used_for_suggestions(), seems can not exclude this scenario.

[chenyukang]

for the solution we could generate an #[allow] for this lint as part of the ast we generate.

I tried to add it here:
428b251

which will break the parser.

seems we only add simple tokentree for the contract and parse it here:
https://github.com/chenyukang/rust/blob/428b251feb3e87e41c9e46656cc96a3b94d88ea5/compiler/rustc_parse/src/parser/generics.rs#L302-L325

we do not expect too much change just to generate an #[allow(unused_parens)], where is the proper point to inject it.

[oli-obk]

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

[chenyukang]

I tried to mark the span with DesugaringKind::Contract at here: https://github.com/chenyukang/rust/blob/9ec13a62cafd1448b2d43d887034ae6501fca6d0/compiler/rustc_ast_lowering/src/item.rs#L1082-L1089

But found that early lint check is run before this point.

I think this solution with span check maybe enough: 64f1f52

the generated closure span are all the same, which is different from real code.

[oli-obk]

Ah right this is due to the macro expansion from

rust/compiler/rustc_builtin_macros/src/contracts.rs

Line 47 in f78099e

fn expand_contract_clause(

maybe the span can be annotated there?

[chenyukang]

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`

[chenyukang]

I think the span checking here is enough for avoid lint for contract syntax here?
https://github.com/rust-lang/rust/pull/136906/files#diff-e25b2c6c1fa434ab8078db81003888370f06b9da5503d1590757350be8e31103R917-R918

it's a bit hacky, but user written code will always true on : e.span != closure.body.span.

And also considering the fact that contract syntax is temporary, if we can not find a perfect way to mark the span we should not introduce more change for it. #137134 (comment)

[oli-obk]

You're absolutely right. Sorry for holding it up

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[oli-obk]

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?

[jieyouxu]

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).

[oli-obk]

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?

[jieyouxu]

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

[jieyouxu]

Since you're already looking at it... r? @oli-obk

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[chenyukang]

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

[chenyukang]

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 };

[bors]

📌 Commit 1e33cd0 has been approved by oli-obk

It is now in the queue for this repository.

[oli-obk]

@bors r- oops should ping T-lang

[oli-obk]

Hey @rust-lang/lang this PR changes the unnecessary parens lint to detect || {0} and || (0) and suggests removing the braces and parens. Rustfmt already does this usually, but the lint will probably still trigger somewhat commonly across the ecosystem judging by the fallout in rustc

[joshtriplett]

@oli-obk I think when rustfmt breaks a closure across lines, it will always have braces. So, it would be possible to end up with:

func(|| {
    long_expr_that_does_not_require_braces
})

It would be a false positive if the lint flags that.

[chenyukang]

@oli-obk I think when rustfmt breaks a closure across lines, it will always have braces. So, it would be possible to end up with:

func(|| {
    long_expr_that_does_not_require_braces
})

It would be a false positive if the lint flags that.

The stable rustfmt will try to remove the { and } in closure body?
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=158d05ca51fe5e08dd79200b73f9f0aa

use the Tools/ Rustfmt to format it.

[oli-obk]

That example just wasn't long enough: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1866ae953cb9d49d8b3845204201ef23

[chenyukang]

That example just wasn't long enough: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1866ae953cb9d49d8b3845204201ef23

emm, if so can we just add lint warning for one line of closure?
from the changes in compiler source code in this PR, all of them are from line of code.

[chenyukang]

I just find this PR will not lint for multiple lines braces, see the new added testcases:

345d8d9

and 89c1195

seems the function check_unused_delims_expr already take care for it.

[joshtriplett]

Excellent. Then in that case, 👍 for this change, personally.

[bors]

☔ The latest upstream changes (presumably #137927) made this pull request unmergeable. Please resolve the merge conflicts.

[traviscross]

While this seemed OK to me, by way of double checking, I was still trying to convince myself this wasn't actually too opinionated somehow. Maybe it's OK if people want to disambiguate here? But then I recalled that these two lines

_ = || -> _ { 0 }.clone();
_ = || { 0 }.clone();

are quite different. That is, starting a closure body with an opening brace doesn't really serve to disambiguate the scope of that body in the way that one might expect.

So since the braces aren't really good for that purpose, it makes sense to treat them as we do other unnecessary braces.
That's enough to convince me this is OK.

This is going to be a meaningful lint expansion (so heads-up to @tmandry), but the fix is automated, so that seems fine too (and I'm glad to not have to come up with another lint name). Therefore, I propose:

@rfcbot fcp merge

[rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[RalfJung]

these two lines [...] are quite different

Wait, what?^^ Could you spell this out / link to some place where it is spelled out?

[traviscross]

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
}

Playground link

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.

[RalfJung]

Oh wow... That is subtle. But one case does not build so at least it does not lead to semantic ambiguity.

[traviscross]

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)));
}

Playground link

There are probably other ways.