c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks
#143619
Conversation
|
|
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
This PR changes a file inside |
|
@bors2 try for the crater |
|
Unknown command "tryfor". |
|
|
wild @bors2 try |
`c_variadic`: Make `fn f(...) {}` error like `fn f(u32) {}` outside of `extern` blocks
This PR makes unnamed `...` parameters (such as the one in `unsafe extern "C" fn f(...) {}`) a parse error to be consistent with `unsafe extern "C" fn f(u32) {}`: this is a source of confusion for programmers coming from C, where the `...` parameter is never named and instead calling `va_start` is required; disallowing unnamed `...` parameters also improves the overall consistency of the Rust language by matching the treatment of other unnamed parameters. Unnamed `...` parameters in `extern` blocks (such as `unsafe extern "C" { fn f(...); }`) continue to compile after this PR, as they are already stable and heavily used (and don't cause the mentioned confusion as they are just being used in function declarations).
As all the syntax gating for `c_variadic` has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:
```rust
#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}
```
Since this is more or less a stability hole and is unlikely to be used in practice, I think it would be ok to break this, but this will definitely require both a crater check run and a lang FCP.
Tracking issue: #44930
cc `@folkertdev` `@workingjubilee`
r? `@joshtriplett`
|
So, should we start that crater run here? |
|
@compiler-errors could you |
|
👍 @craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚨 Report generation of 🆘 If you need assistance dealing with this failure, please ask in t-infra on Zulip |
|
not sure I have the permissions for this, but let's attempt @craterbot retry-report |
|
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚨 Report generation of 🆘 If you need assistance dealing with this failure, please ask in t-infra on Zulip |
|
let's try again with rust-lang/crater#787 merged @craterbot retry-report |
|
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
removed
the
S-waiting-on-crater
|
The changes in #147442 create a conflict here. That test should ignore the warning because the whole thing it's testing is that it's an argument without a pattern. |
|
@rustbot author for that then |
rustbot
added
S-waiting-on-author
folkertdev
force-pushed
the
varargs-named
branch
from
0d1e5ae to
d0e3e8b
Compare
This comment has been minimized.
This comment has been minimized.
rustbot
added
S-waiting-on-review
| @future_incompatible = FutureIncompatibleInfo { | ||
| reason: FutureIncompatibilityReason::FutureReleaseError, | ||
| reference: "issue #145544 <https://github.com/rust-lang/rust/issues/145544>", | ||
| report_in_deps: false, |
There was a problem hiding this comment.
right, as per the FCP, I've verified that we set report_in_deps to false for now. Feel free to assign another PR to me in the future when this changes
|
@folkertdev or @beetrees r=me after rebase :) |
|
or if I see it, I'll r+ ofc |
|
@rustbot author |
rustbot
added
S-waiting-on-author
folkertdev
force-pushed
the
varargs-named
branch
from
d0e3e8b to
e32cb1e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
varargs-named
branch
from
e32cb1e to
6e28ba9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…without a pattern outside of `extern` blocks
folkertdev
force-pushed
the
varargs-named
branch
from
6e28ba9 to
02e1f44
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Thanks for the review! @bors r=jdonszelmann |
bors
added
S-waiting-on-bors
…mann
`c_variadic`: Add future-incompatibility warning for `...` arguments without a pattern outside of `extern` blocks
This PR makes `...` arguments without a pattern in non-foreign functions (such as the argument in `unsafe extern "C" fn f(...) {}`) a future-compatibility warning; making this error would be consistent with how `unsafe extern "C" fn f(u32) {}` is handled. Allowing `...` arguments without a pattern in non-foreign functions is a source of confusion for programmers coming from C, where the `...` parameter is never named and instead calling `va_start` is required; disallowing `...` arguments without a pattern also improves the overall consistency of the Rust language by matching the treatment of other arguments without patterns. `...` arguments without a pattern in `extern` blocks (such as `unsafe extern "C" { fn f(...); }`) continue to compile without warnings after this PR, as they are already stable and heavily used (and don't cause the mentioned confusion as they are just being used in function declarations).
As all the syntax gating for `c_variadic` has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:
```rust
#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}
```
Since this is more or less a stability hole and a Crater run shows only the `binrw` crate is using this, I think it would be ok to break this. This will require a lang FCP.
The idea of rejecting `...` pre-expansion was first raised here rust-lang#143546 (comment).
Tracking issue: rust-lang#44930
cc `@folkertdev` `@workingjubilee`
r? `@joshtriplett`
14 participants
This PR makes
...arguments without a pattern in non-foreign functions (such as the argument inunsafe extern "C" fn f(...) {}) a future-compatibility warning; making this error would be consistent with howunsafe extern "C" fn f(u32) {}is handled. Allowing...arguments without a pattern in non-foreign functions is a source of confusion for programmers coming from C, where the...parameter is never named and instead callingva_startis required; disallowing...arguments without a pattern also improves the overall consistency of the Rust language by matching the treatment of other arguments without patterns....arguments without a pattern inexternblocks (such asunsafe extern "C" { fn f(...); }) continue to compile without warnings after this PR, as they are already stable and heavily used (and don't cause the mentioned confusion as they are just being used in function declarations).As all the syntax gating for
c_variadichas been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:Since this is more or less a stability hole and a Crater run shows only the
binrwcrate is using this, I think it would be ok to break this. This will require a lang FCP.The idea of rejecting
...pre-expansion was first raised here #143546 (comment).Tracking issue: #44930
cc @folkertdev @workingjubilee
r? @joshtriplett