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

Warn must_use in tuple #29815

Closed
wants to merge 2 commits into from
Closed

Conversation

sanxiyn
Copy link
Member

@sanxiyn sanxiyn commented Nov 13, 2015

cc #26291.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@sanxiyn
Copy link
Member Author

sanxiyn commented Nov 23, 2015

Ping.

@bors
Copy link
Contributor

bors commented Nov 26, 2015

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

@sanxiyn
Copy link
Member Author

sanxiyn commented Dec 7, 2015

Another ping.

@arielb1
Copy link
Contributor

arielb1 commented Dec 8, 2015

I am not sure this is a bug.

@arielb1 arielb1 added I-needs-decision Issue: In need of a decision. T-tools labels Dec 8, 2015
@arielb1
Copy link
Contributor

arielb1 commented Dec 8, 2015

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned arielb1 Dec 8, 2015
@@ -45,4 +45,7 @@ fn main() {
let _ = foo::<isize>();
let _ = foo::<MustUse>();
let _ = foo::<MustUseMsg>();

foo::<(MustUse, ())>(); //~ ERROR: unused result which must be used
foo::<(MustUseMsg, ())>(); //~ ERROR: unused result which must be used: some message
Copy link
Member

Choose a reason for hiding this comment

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

Can you add checks that when we do use the result, then it is not an error?

@nrc
Copy link
Member

nrc commented Dec 8, 2015

The code looks fine (other than the minor things mentioned). But this would be a breaking change, so I would like to check with other memebers of the tools team that this should land.

Whilst it seems like a good idea, the must use lint is a little bit questionable in this case - if a field on a struct was must_use, we wouldn't assume the whole struct is must_use, so it is not clear if a tuple should be either.

cc @rust-lang/tools

@sanxiyn
Copy link
Member Author

sanxiyn commented Dec 8, 2015

cc @nagisa who reported the original issue.

@sanxiyn
Copy link
Member Author

sanxiyn commented Dec 8, 2015

Does strengthening a lint count as a breaking change? I thought it specifically didn't count as one. I'd like a clarification since I have another PR(#30021) that strengthens a lint.

@nagisa
Copy link
Member

nagisa commented Dec 8, 2015

Since it is a warning, I wouldn’t count it a breaking change in any imaginable way. There was a lengthy discussion about users of #[deny(...)] in context of new warnings and we basically decided to just ignore they exist AFAIR.

if result.is_some() {
return result;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, something like this might work:

tys.iter()
   .filter_map(|ty| check_must_use(cx, ty))
   .next()

@alexcrichton
Copy link
Member

We discussed this during the tools triage meeting today (sorry for the delay!) and the conclusion was that we don't want to merge this at this time. This arguably isn't idiomatic to return a tuple of results (vs a result of tuples) and it also doesn't extend well to other type compositions like enums/structs (you can ignore a struct which contains a #[must_use]. Thanks for the PR though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-needs-decision Issue: In need of a decision.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants