Skip to content

Make String a must_use type #75934

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

Closed
wants to merge 2 commits into from
Closed

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Aug 26, 2020

Make String a must_use type to lint again unused String: #75742

@@ -472,7 +472,7 @@ impl DocFolder for Cache {
});

if pushed {
self.stack.pop().expect("stack already empty");
let _ = self.stack.pop().expect("stack already empty");
Copy link
Member

Choose a reason for hiding this comment

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

I think lines like this are a good example of why String and Vec<T> aren't must_use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These cases are rare in Rustc codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @scottmcm here. If it's on String, then it should be on Vec too, and if it's on Vec, then it should also be on every container. At which point you'd have to somewhat argue why it's not on basically everything that's not Copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With all respects, I just want some actions on resolving #75742 . If the team doesn't like this approach,
what about marking String::from, str::into, fmt::format with must_use attr?

@scottmcm scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 26, 2020
@tesuji tesuji force-pushed the must_use_String branch 3 times, most recently from a3d3a08 to 0bcc7d6 Compare August 26, 2020 14:54
@tesuji tesuji marked this pull request as ready for review August 29, 2020 04:39
@tesuji
Copy link
Contributor Author

tesuji commented Aug 29, 2020

@rustbot modify labels: T-libs +S-waiting-on-review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2020
@tesuji
Copy link
Contributor Author

tesuji commented Aug 29, 2020

r? @dtolnay

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I agree with @scottmcm above. A String being present in a return value does not mean it's a newly allocated string that you could hope to optimize away when unused, as implied by the PR's must_use message of "unused allocation might not be optimized out by compiler". I am concerned that must_use has been trending noisier and noisier as it appears in increasingly questionable places and I would prefer to be more cautious and attentive about false positives when applying it.

@dtolnay dtolnay closed this Aug 29, 2020
@dtolnay
Copy link
Member

dtolnay commented Aug 29, 2020

With all respects, I just want some actions on resolving #75742 . If the team doesn't like this approach,
what about marking String::from, str::into, fmt::format with must_use attr?

I don't see the connection of those suggestions toward resolving #75742. That issue is motivated by f(&format!(...)) where f is inlinable and does not access its argument. As far as must_use would be concerned it's still a use of the value because the value gets bound to a variable (the function's formal argument).

Independent of #75742, I would still recommend against going for must_use on those functions unless there is some evidence of pattern of misuse of those functions that we are hoping to prevent. Aggressively tagging must_use without a pattern of misuse just increases the false positive rate and general nuisance of must_use without delivering practical benefit.

@tesuji tesuji deleted the must_use_String branch August 29, 2020 07:02
@tesuji
Copy link
Contributor Author

tesuji commented Aug 29, 2020

I don't see the connection of those suggestions toward resolving #75742.

I meant: format! expands to fmt::format(Arguments), so marking fmt::format result must_use
will make people be aware of unused allocated String. So they will use the String or remove the useless expression
or replace it with to_string method or so.
I have never seen the meaning of unused fmt::format result, unless in test code.

But yeah, if the lib team has other ways to resolve the issue, I am happy to wait for it.

@scottmcm
Copy link
Member

scottmcm commented Sep 1, 2020

@lzutao An idea you might be interested to play with: the unused_results lint is allow-by-default because it's so noisy. Maybe there's some subset of it -- perhaps "unused result where the unused type happens to have drop glue and came from a &impl Freeze method", but exact rules obviously TBD -- that could be worth experimenting with to see if there's a version of it that people would be more inclined to actually turn on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants