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

Don't chain method calls in #[derive(Debug)] #29565

Merged
merged 1 commit into from
Nov 5, 2015

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Nov 4, 2015

Closes #29540

r? @huonw

@huonw
Copy link
Member

huonw commented Nov 4, 2015

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2015

📌 Commit d0bc6a1 has been approved by huonw

@bors
Copy link
Contributor

bors commented Nov 4, 2015

⌛ Testing commit d0bc6a1 with merge 3c2d6a7...

@bors
Copy link
Contributor

bors commented Nov 4, 2015

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Wed, Nov 4, 2015 at 3:37 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/6973


Reply to this email directly or view it on GitHub
#29565 (comment).

@bors
Copy link
Contributor

bors commented Nov 4, 2015

⌛ Testing commit d0bc6a1 with merge 9f84cae...

@bors
Copy link
Contributor

bors commented Nov 4, 2015

💔 Test failed - auto-mac-64-nopt-t

@alexcrichton
Copy link
Member

@bors: retry

Hurray git submodules!

This means landing #29546 is not going to be fun.

@bors
Copy link
Contributor

bors commented Nov 5, 2015

💔 Test failed - auto-linux-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Wed, Nov 4, 2015 at 7:12 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-32-opt
http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/7010


Reply to this email directly or view it on GitHub
#29565 (comment).

@bors
Copy link
Contributor

bors commented Nov 5, 2015

⌛ Testing commit d0bc6a1 with merge 3a0409d...

bors added a commit that referenced this pull request Nov 5, 2015
@bors bors merged commit d0bc6a1 into rust-lang:master Nov 5, 2015
@alexcrichton
Copy link
Member

Hm, I think that this unfortunately caused a regression on nightly, @sfackler do you know off hand what may have caused that?

@sfackler
Copy link
Member Author

sfackler commented Nov 8, 2015

@alexcrichton The methods on the builders return a reference to the builder, so if you have #[deny(unused_result)] turned on it'll yell at you:

~ ❯ multirust run nightly rustc test.rs --pretty expanded -Z unstable-options
#![feature(no_std, prelude_import)]
#![no_std]
#![deny(unused_results)]
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std as std;
pub struct Error {
    code: i32,
    msg: &'static str,
}
#[automatically_derived]
#[allow(unused_qualifications)]
impl ::std::fmt::Debug for Error {
    fn fmt(&self, __arg_0: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
        match *self {
            Error { code: ref __self_0_0, msg: ref __self_0_1 } => {
                let mut builder = __arg_0.debug_struct("Error");
                builder.field("code", &&(*__self_0_0));
                builder.field("msg", &&(*__self_0_1));
                builder.finish()
            }
        }
    }
}

fn main() { }
~ ❯ multirust run nightly rustc test.rs --pretty expanded -Z unstable-options | multirust run nightly rustc -
<anon>:5:5: 5:25 warning: unused import, #[warn(unused_imports)] on by default
<anon>:5 use std::prelude::v1::*;
             ^~~~~~~~~~~~~~~~~~~~
<anon>:19:17: 19:56 error: unused result
<anon>:19                 builder.field("code", &&(*__self_0_0));
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:3:9: 3:23 note: lint level defined here
<anon>:3 #![deny(unused_results)]
                 ^~~~~~~~~~~~~~
<anon>:20:17: 20:55 error: unused result
<anon>:20                 builder.field("msg", &&(*__self_0_1));
                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:3:9: 3:23 note: lint level defined here
<anon>:3 #![deny(unused_results)]
                 ^~~~~~~~~~~~~~
error: aborting due to 2 previous errors

We can fix it by adding some let _ = ...s, but #[deny(unused_result)] seems perhaps excessive?

@alexcrichton
Copy link
Member

Oh right, sorry I was mixing up the Result lint and this one! I do agree that this is a little more egregious to fix in that case, but with #29710 cropping up as well I'd perhaps vote to either add an explicit #[allow] or let _ =.

The use case for unused_result at least, making sure results from C libraries aren't dropped on the floor, seems somewhat common at least.

@sfackler
Copy link
Member Author

sfackler commented Nov 9, 2015

#[allow] is insufficient since the crate could have a #[forbid].

@sfackler sfackler deleted the issue-29540 branch November 26, 2016 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants