feature-gate #[must_use] for functions as `fn_must_use`#43776

zackmdavis · 2017-08-09T22:10:17Z

@eddyb I was dithering on this, but your comment makes it sound like we do want a feature gate for this? Please advise.

kennytm · 2017-08-10T01:19:47Z

src/test/compile-fail/feature-gate-fn_must_use.rs

The test is written wrongly, it should add an //~ ERROR #[must_use] on function is experimental to one of these two lines (depends on which span is used to report the error).

kennytm · 2017-08-10T01:19:55Z

src/test/compile-fail/feature-gate-fn_must_use.rs

tidy error: /checkout/src/test/compile-fail/feature-gate-fn_must_use.rs:11: The file is already marked as gate test through its name, no need for a 'gate-test-fn_must_use' comment

bors · 2017-08-10T13:50:51Z

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

bluss · 2017-08-11T16:27:11Z

It's a breaking change -- Rust 1.19 thinks #[must_use] on a function is ok to compile.

eddyb · 2017-08-11T16:38:53Z

r? @rust-lang/libs

zackmdavis · 2017-08-11T17:24:13Z

Rust 1.19 thinks #[must_use] on a function is ok to compile.

The behavior as of the current revision of this pull request (Travis is currently failing because of tidy and the feature-gate test, but we can fix that after we understand what we want) would be the same with respect to allowing #[must_use] on functions to compile, but the warning would only be emitted if the feature is enabled.

it should add an //~ ERROR #[must_use] on function is experimental

@kennytm Are we allowed to make it a warning rather than a hard error? (To avoid it being a breaking change, see @bluss's comment.)

kennytm · 2017-08-11T17:45:14Z

@zackmdavis

A future-incompatible warning like E0122 can be issued. But that's up to the lang team to decide.

If fn_must_use is disabled, #[must_use] on function is no-op (allowed or future-incompatible-linted), and ignoring the return value will produce no lint.
If fn_must_use is enabled, #[must_use] on function does what we expected (allowed), and ignoring the return value will produce lint.

alexcrichton · 2017-08-11T19:54:06Z

Thanks for the PR @zackmdavis! I think @kennytm's suggestions are all that's needed to land this!

zackmdavis · 2017-08-16T05:53:59Z

(Updated—includes reworking some feature-gate functionality to be able to only emit a warning. On the other hand, if we decide we want a hard error (is there really going to be much code in the wild that has a #[must_use] on a function even though it's currently a no-op?—does such silly code not deserve what it gets, or are our stability guarantees meant to be taken that literally?), a937dca could be excluded.)

alexcrichton

Thanks! I wonder if it would be possible to gate as a "hard" error by default? That may help reduce the surrounding changes here!

alexcrichton · 2017-08-16T16:12:43Z

src/test/compile-fail/feature-gate-fn_must_use.rs

We also have #[rustc_error] used throughout the test suite which may help here?

zackmdavis · 2017-08-16T17:02:10Z

I wonder if it would be possible to gate as a "hard" error by default?

Fine with me (pull request updated). A compatibility bullet point in the release notes seems appropriate, though.

alexcrichton · 2017-08-16T17:28:37Z

Oh oops sorry! I think I miscommunicated a bit. I think it's probably a good idea to avoid turning #[must_use] on functions into hard errors immediately, what I meant was that instead of specifying Hard to all the gating invocations that was just done by default, but #[must_use] still generated warnings.

I wonder if perhaps though there's a more targeted way of "deprecating" the #[must_use] attribute on functions?

zackmdavis · 2017-08-16T18:13:09Z

what I meant was that instead of specifying Hard to all the gating invocations that was just done by default

Oh, right. I did do some defaulting for the macros (which can be variadic), but I was trying to avoid too much code duplication. Looking at it today, I'm sure a much better trade-off between avoiding duplication and avoiding polluting existing feature_err and emit_feature_err callsites with a new argument is possible. I'm out of town for a few days (RustConf!), but I can probably get to this on Monday?

alexcrichton · 2017-08-17T16:02:16Z

Yeah the behavior of the current patch is fine by me, thanks! I'd just recommend a littl erefactoring to avoid needing to pass GateStrength::Hard so often, and other than that r=me!

We'll actually want a new "soft" warning-only gate to maintain backwards-compatibility, but it's cleaner to start out with the established, well-understood gate before implementing the alternative warn-only behavior in a later commit. This is in the matter of rust-lang#43302.

The featureck.py that this comment referred to was removed in 9dd3c54 (March 2016).

Before `#[must_use]` for functions was implemented, a `#[must_use]` attribute on a function was a no-op. To avoid a breaking change in this behavior, we add an option for "this-and-such feature is experimental" feature-gate messages to be a mere warning rather than a compilation-halting failure (so old code that used to have a useless no-op `#[must_use]` attribute now warns rather than breaking). When we're on stable, we add a help note to clarify that the feature isn't "on." This is in support of rust-lang#43302.

This continues to be in the matter of rust-lang#43302.

zackmdavis · 2017-08-23T05:23:47Z

@alexcrichton updated (the only Travis subjobs that haven't passed yet are all macOS stuff)

alexcrichton · 2017-08-23T14:29:50Z

@bors: r+

bors · 2017-08-23T14:29:51Z

📌 Commit 35c4494 has been approved by alexcrichton

bors · 2017-08-24T06:31:50Z

⌛ Testing commit 35c4494 with merge 1e20ae244f66a819647a597712fee38706ca1356...

kennytm · 2017-08-24T09:43:30Z

@bors ping

Hello, both Travis and AppVeyor have finished successfully but nothing happens?

bors · 2017-08-24T09:43:31Z

😪 I'm awake I'm awake

kennytm · 2017-08-24T09:45:01Z

@bors retry

bors · 2017-08-24T09:45:10Z

⌛ Testing commit 35c4494 with merge abd03494a40b02d6fa04f4cd1d025bb04423c61c...

bors · 2017-08-24T10:02:14Z

💔 Test failed - status-appveyor

kennytm · 2017-08-24T10:08:39Z

@bors retry rollup

Spurious failure Tracking issue for Spurious failure for "InnoSetup not installed" on AppVeyor #43985
Rollup because it is known to pass CI.

@eddyb

…, r=alexcrichton feature-gate #[must_use] for functions as `fn_must_use` @eddyb I [was](rust-lang#43728 (comment)) [dithering](rust-lang#43728 (comment)) on this, but [your comment](rust-lang#43302 (comment)) makes it sound like we do want a feature gate for this? Please advise. r? @eddyb

Rollup of 7 pull requests - Successful merges: #43776, #43966, #43979, #44072, #44086, #44090, #44091 - Failed merges:

SimonSapin · 2017-08-31T12:15:43Z

I think this should be reverted: #44213 (comment)

rust-highfive assigned eddyb Aug 9, 2017

kennytm reviewed Aug 10, 2017

View reviewed changes

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 10, 2017

rust-highfive unassigned eddyb Aug 11, 2017

alexcrichton self-assigned this Aug 11, 2017

zackmdavis force-pushed the feature_gate_fn_must_use branch 4 times, most recently from bad6afc to a937dca Compare August 16, 2017 04:35

zackmdavis mentioned this pull request Aug 16, 2017

Tracking issue for RFC 1940: allow #[must_use] on functions #43302

Closed

3 tasks

alexcrichton reviewed Aug 16, 2017

View reviewed changes

zackmdavis force-pushed the feature_gate_fn_must_use branch from a937dca to 398f414 Compare August 16, 2017 17:00

zackmdavis force-pushed the feature_gate_fn_must_use branch from 398f414 to 0f385f4 Compare August 16, 2017 17:47

zackmdavis added 2 commits August 22, 2017 17:30

correct comment re feature-checking tooling

7b6e9b4

The featureck.py that this comment referred to was removed in 9dd3c54 (March 2016).

zackmdavis force-pushed the feature_gate_fn_must_use branch 2 times, most recently from 8f6724b to 6ca5490 Compare August 23, 2017 02:30

zackmdavis added 2 commits August 22, 2017 20:37

fn_must_use soft feature-gate warning on methods too, not only functions

35c4494

This continues to be in the matter of rust-lang#43302.

zackmdavis force-pushed the feature_gate_fn_must_use branch from 6ca5490 to 35c4494 Compare August 23, 2017 03:40

alexcrichton mentioned this pull request Aug 26, 2017

Rollup of 14 pull requests #44093

Closed

frewsxcv mentioned this pull request Aug 26, 2017

Rollup of 7 pull requests #44098

Merged

bors added a commit that referenced this pull request Aug 26, 2017

Auto merge of #44098 - frewsxcv:rollup, r=frewsxcv

32f6056

Rollup of 7 pull requests - Successful merges: #43776, #43966, #43979, #44072, #44086, #44090, #44091 - Failed merges:

bors merged commit 35c4494 into rust-lang:master Aug 26, 2017

zackmdavis mentioned this pull request Aug 26, 2017

add compatibility bullet point for fn_must_use feature warning to 1.20 release notes #44106

Closed

SimonSapin mentioned this pull request Aug 31, 2017

#[must_use] now suddenly warns in nightly #44213

Closed

zackmdavis deleted the feature_gate_fn_must_use branch January 13, 2018 07:44

zackmdavis mentioned this pull request Jan 16, 2021

feature_gate: Remove GateStrength #68018

Merged

Uh oh!

Conversation

zackmdavis commented Aug 9, 2017

Uh oh!

kennytm Aug 10, 2017

Choose a reason for hiding this comment

Uh oh!

kennytm Aug 10, 2017

Choose a reason for hiding this comment

Uh oh!

bors commented Aug 10, 2017

Uh oh!

bluss commented Aug 11, 2017

Uh oh!

eddyb commented Aug 11, 2017

Uh oh!

zackmdavis commented Aug 11, 2017

Uh oh!

kennytm commented Aug 11, 2017

Uh oh!

alexcrichton commented Aug 11, 2017

Uh oh!

zackmdavis commented Aug 16, 2017

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

alexcrichton Aug 16, 2017

Choose a reason for hiding this comment

Uh oh!

zackmdavis commented Aug 16, 2017

Uh oh!

alexcrichton commented Aug 16, 2017

Uh oh!

zackmdavis commented Aug 16, 2017

Uh oh!

alexcrichton commented Aug 17, 2017

Uh oh!

zackmdavis commented Aug 23, 2017

Uh oh!

alexcrichton commented Aug 23, 2017

Uh oh!

bors commented Aug 23, 2017

Uh oh!

bors commented Aug 24, 2017

Uh oh!

kennytm commented Aug 24, 2017

Uh oh!

bors commented Aug 24, 2017

Uh oh!

kennytm commented Aug 24, 2017

Uh oh!

bors commented Aug 24, 2017

Uh oh!

bors commented Aug 24, 2017

Uh oh!

kennytm commented Aug 24, 2017

Uh oh!

SimonSapin commented Aug 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants