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

Running rustdoc on code using quote generates false lint warnings for unused_braces #70814

Open
shepmaster opened this issue Apr 5, 2020 · 22 comments
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@shepmaster
Copy link
Member

I tried this code:

[package]
name = "repro"
version = "0.1.0"
edition = "2018"

[dependencies]
quote = "=1.0.3"
use quote::quote;

pub fn repro() {
    let many = [1, 2, 3];

    let _together = quote! {
        #(#many),*
    };
}

I think this is a basic and common usage of the quote crate.

I expected to see no warnings:

% RUSTFLAGS=-Dwarnings cargo +nightly-2020-04-04 build
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

% RUSTFLAGS=-Dwarnings cargo +nightly-2020-04-04 doc
 Documenting repro v0.1.0 (/private/tmp/repro)
    Finished dev [unoptimized + debuginfo] target(s) in 0.82s

Instead, warnings are generated (and my CI build fails due to the deny):

% RUSTFLAGS=-Dwarnings cargo +nightly-2020-04-04 build
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s

% RUSTFLAGS=-Dwarnings cargo +nightly-2020-04-04 doc
 Documenting repro v0.1.0 (/private/tmp/repro)
warning: unnecessary braces around block return value
  |
  = note: `#[warn(unused_braces)]` on by default

    Finished dev [unoptimized + debuginfo] target(s) in 0.82s

Note that the warning only appears when running rustdoc.

Meta

rustc +nightly-2020-04-04 --version --verbose:

rustc 1.44.0-nightly (74bd074ee 2020-04-03)
binary: rustc
commit-hash: 74bd074eefcf4915c73d1ab91bc90859664729e6
commit-date: 2020-04-03
host: x86_64-apple-darwin
release: 1.44.0-nightly
LLVM version: 9.0
@shepmaster shepmaster added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Apr 5, 2020
@shepmaster
Copy link
Member Author

/cc @lcnr

@eddyb
Copy link
Member

eddyb commented Apr 5, 2020

Note that the warning only appears when running rustdoc.

Hang on, could this be because of the transformation rustdoc uses (AFAIK) that replaces fn bodies with loop {}?

@Nemo157
Copy link
Member

Nemo157 commented Apr 7, 2020

Simplified repro source, both inner blocks are necessary for some reason:

pub fn repro() {
    {
        {
            use std;
        }
    }
}

@eddyb
Copy link
Member

eddyb commented Apr 7, 2020

@Nemo157 My guess it's because {{}} is basically {()} - the outer block has the inner block as its final expression. Curious that this is rustdoc-specific.

@tmandry
Copy link
Member

tmandry commented Apr 8, 2020

I just hit a similar error message in rustc (not rustdoc). Unused braces lint with no span or other information. I don't have a reproducer currently, since it's from a big crate and I don't have much to go by.

@grantperry
Copy link

I'm getting unnescessary braces around block return value from check/clippy

warning: unnecessary braces around block return value
   --> src/models/user.rs:137:63
    |
137 |     fn ticket(&self, context : &SharedContext) -> Option<Ticket> { self.get_ticket(context) }
    |                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these braces

Is this related or should I create a new issue?

Using Nightly 1.44.0 2020-04-16

@lcnr
Copy link
Contributor

lcnr commented Apr 19, 2020

@grantperry is this inside a macro?

@grantperry
Copy link

@lcnr yes it is. Sounds like it's something you already know about?

@lcnr
Copy link
Contributor

lcnr commented Apr 19, 2020

If you are using a proc macro using quote then this should be the same issue afaict

@lcnr
Copy link
Contributor

lcnr commented Apr 20, 2020

I tried to fix this by checking if the surrounding expression comes from an expansion, but this didn't actually fix this. 🤔 @matklad do you have an idea/can this be fixed in quote?

My approach stilled warned for the following, which seems to be the same issue afaict.

#[inline]
#[allow(unused_variables, unused_braces)]
fn cache_on_disk(
#tcx: TyCtxt<'tcx>,
#key: Self::Key,
#value: Option<&Self::Value>
) -> bool {
#expr
}

@matklad
Copy link
Member

matklad commented Apr 20, 2020

I don't think I am familiar with the code in question @lcnr

@lcnr
Copy link
Contributor

lcnr commented Apr 20, 2020

Sorry, mb 😓 meant @dtolnay as he is the owner of quote.

@dtolnay dtolnay added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Apr 20, 2020
@dtolnay
Copy link
Member

dtolnay commented Apr 20, 2020

Labeling regression-from-stable-to-nightly. I would consider accepting a workaround in quote if somebody who is not me sends a PR and it is a very simple fix, but really this needs to be fixed in rustdoc / rustc.

@jonas-schievink jonas-schievink added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 20, 2020
@LeSeulArtichaut LeSeulArtichaut added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Apr 20, 2020
@LeSeulArtichaut
Copy link
Contributor

Let’s try to find the cause of the regression
@rustbot ping cleanup

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2020

Error: Only Rust team members can ping teams.

Please let @rust-lang/release know if you're having trouble with this bot.

@Dylan-DPC-zz
Copy link

@rustbot ping cleanup

@rustbot
Copy link
Collaborator

rustbot commented Apr 20, 2020

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @jakevossen5 @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

@rustbot rustbot added the ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections label Apr 20, 2020
@AminArria
Copy link
Contributor

searched nightlies: from nightly-2020-03-29 to nightly-2020-04-05
regressed nightly: nightly-2020-04-02
searched commits: from a5b09d3 to 76b1198
regressed commit: 58dd1ce

I guess the PR would be #70081

martell added a commit to martell/diesel that referenced this issue Apr 25, 2020
martell added a commit to martell/diesel that referenced this issue Apr 25, 2020
martell added a commit to martell/diesel that referenced this issue Apr 25, 2020
martell added a commit to martell/diesel that referenced this issue Apr 25, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 25, 2020
 Quick and dirty fix of the unused_braces lint

cc @lcnr

Adresses rust-lang#70814

This at least prevents lint output, if no span is available. Even though this also prevents the `unused_parens` lint from emitting, when the `DUMMY_SP` is used there, but I think that should be ok, since error messages without a span are quite useless anyway.

Clippy CI is currently blocked on this bug. If this quick and dirty fix should be rejected, I could try to work around this in Clippy.

r? @shepmaster
lawliet89 added a commit to lawliet89/biscuit that referenced this issue Apr 29, 2020
* Remove serde-derive dependency

* Fix clippy

* Fix nightly clippy

* Remove some extern crate

* 0.5.0 is unreleased!

* Tweak deny warnings

- According to
https://users.rust-lang.org/t/how-to-fail-a-build-with-warnings/2687/7

* Check in Cargo.lock for more reproducible tests

* Tighten ring dependency version requirement

* Fix url crate to 2.1.0

- Pending regression

* Fix doctest

* Workaround regression

cf. rust-lang/rust#70814

* Fix format

* Rearrange steps to check fmt first

* Relax url and exclude 2.1.1

* Fix tests
martell added a commit to martell/diesel that referenced this issue May 2, 2020
martell added a commit to martell/diesel that referenced this issue May 2, 2020
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue May 9, 2020
@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jan 21, 2021
@osa1
Copy link
Contributor

osa1 commented Jan 30, 2021

I think e42337b a fixed this issue. It didn't add a test though so perhaps mark as needs-test?

exrook pushed a commit to exrook/futures-rs that referenced this issue Apr 5, 2021
@Igosuki
Copy link

Igosuki commented Sep 13, 2021

@osa1 it didn't :

warning: unnecessary braces around block return value
  --> strategies/src/types.rs:67:34
   |
67 |     fn asset_type(&self) -> &str { self.asset_type.as_ref() }
   |                                  ^^                        ^^
   |
help: remove these braces
   |
67 -     fn asset_type(&self) -> &str { self.asset_type.as_ref() }
67 +     fn asset_type(&self) -> &str self.asset_type.as_ref()

kraktus added a commit to kraktus/rust that referenced this issue Jan 6, 2023
The modified lint correctly provide a span in its suggestion.

```shell
error: unnecessary braces around block return value
  --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:9:5
   |
LL | /     {
LL | |         {
   | |________^
LL |               use std;
LL |           }
   |  __________^
LL | |     }
   | |_____^
   |
note: the lint level is defined here
  --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:6:9
   |
LL | #![deny(unused_braces)]
   |         ^^^^^^^^^^^^^
help: remove these braces
   |
LL ~     {
LL |             use std;
LL ~         }
   |
```

It is unclear to which extend rust-lang#70814 is still an issue, as the inital MCVE does not trigger the lint on stable
 either,[rust playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b6ff31a449c0b73a08daac8ee43b1fa6)
kraktus added a commit to kraktus/rust that referenced this issue Jan 13, 2023
Currently the lint faces a severe limitation: since it only catches single-line block, running rustfmt beforehand will remove all occurences of it, because it breaks them into multiline blocks.

We do not check match `Arm` for two reasons:
- In case it does not use commas to separate arms, removing the block would result in a compilation error
Example:
```
match expr {
    pat => {()}
    _ => println!("foo")
}
```
- Do not lint multiline match arms used for formatting reasons
```
match expr {
   pat => {
       somewhat_long_expression
   }
 // ...
}

Delete `unused-braces-lint` test

The modified lint correctly provide a span in its suggestion.

```shell
error: unnecessary braces around block return value
  --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:9:5
   |
LL | /     {
LL | |         {
   | |________^
LL |               use std;
LL |           }
   |  __________^
LL | |     }
   | |_____^
   |
note: the lint level is defined here
  --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:6:9
   |
LL | #![deny(unused_braces)]
   |         ^^^^^^^^^^^^^
help: remove these braces
   |
LL ~     {
LL |             use std;
LL ~         }
   |
```

It is unclear to which extend rust-lang#70814 is still an issue, as the inital MCVE does not trigger the lint on stable
 either,[rust playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b6ff31a449c0b73a08daac8ee43b1fa6)

Fix code with expanded `unused_braces` lint

Also allow `unused_braces` on tests
kraktus added a commit to kraktus/rust that referenced this issue Jan 27, 2023
Currently the lint faces a severe limitation: since it only catches single-line block, running rustfmt beforehand will remove all occurences of it, because it breaks them into multiline blocks.

We do not check match `Arm` for two reasons:
- In case it does not use commas to separate arms, removing the block would result in a compilation error
Example:
```
    match expr {
        pat => {()}
        _ => println!("foo")
    }
    ```
    - Do not lint multiline match arms used for formatting reasons
    ```
    match expr {
       pat => {
           somewhat_long_expression
       }
     // ...
    }
```

Delete `unused-braces-lint` test

The modified lint correctly provide a span in its suggestion.

```shell
    error: unnecessary braces around block return value
      --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:9:5
       |
    LL | /     {
    LL | |         {
       | |________^
    LL |               use std;
    LL |           }
       |  __________^
    LL | |     }
       | |_____^
       |
    note: the lint level is defined here
      --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:6:9
       |
    LL | #![deny(unused_braces)]
       |         ^^^^^^^^^^^^^
    help: remove these braces
       |
    LL ~     {
    LL |             use std;
    LL ~         }
       |
```

It is unclear to which extend rust-lang#70814 is still an issue, as the inital MCVE does not trigger the lint on stable either,[rust playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b6ff31a449c0b73a08daac8ee43b1fa6)

Fix code with expanded `unused_braces` lint

Also allow `unused_braces` on tests

Mute `unused_braces` on `match_ast!`
kraktus added a commit to kraktus/rust that referenced this issue Jan 27, 2023
Currently the lint faces a severe limitation: since it only catches single-line block, running rustfmt beforehand will remove all occurences of it, because it breaks them into multiline blocks.

We do not check match `Arm` for two reasons:
- In case it does not use commas to separate arms, removing the block would result in a compilation error
Example:
```
    match expr {
        pat => {()}
        _ => println!("foo")
    }
    ```
    - Do not lint multiline match arms used for formatting reasons
    ```
    match expr {
       pat => {
           somewhat_long_expression
       }
     // ...
    }
```

Delete `unused-braces-lint` test

The modified lint correctly provide a span in its suggestion.

```shell
    error: unnecessary braces around block return value
      --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:9:5
       |
    LL | /     {
    LL | |         {
       | |________^
    LL |               use std;
    LL |           }
       |  __________^
    LL | |     }
       | |_____^
       |
    note: the lint level is defined here
      --> /rust/src/test/rustdoc-ui/unused-braces-lint.rs:6:9
       |
    LL | #![deny(unused_braces)]
       |         ^^^^^^^^^^^^^
    help: remove these braces
       |
    LL ~     {
    LL |             use std;
    LL ~         }
       |
```

It is unclear to which extend rust-lang#70814 is still an issue, as the inital MCVE does not trigger the lint on stable either,[rust playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b6ff31a449c0b73a08daac8ee43b1fa6)

Fix code with expanded `unused_braces` lint

Also allow `unused_braces` on tests

Mute `unused_braces` on `match_ast!`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. ICEBreaker-Cleanup-Crew Helping to "clean up" bugs with minimal examples and bisections P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests