-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Add explicit_extern_abis
Feature and Enforce Explicit ABIs
#135340
base: master
Are you sure you want to change the base?
Conversation
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
This comment has been minimized.
This comment has been minimized.
938c721
to
b327d2d
Compare
This comment has been minimized.
This comment has been minimized.
I do not think we will accept the edition changes being folded into this PR, @obeis |
b327d2d
to
e25d0af
Compare
e25d0af
to
20f28b1
Compare
This comment has been minimized.
This comment has been minimized.
fe8b62b
to
bc8ed0e
Compare
This comment has been minimized.
This comment has been minimized.
bc8ed0e
to
e1db710
Compare
This PR modifies cc @jieyouxu |
d8ddeb1
to
008c19c
Compare
☔ The latest upstream changes (presumably #139269) made this pull request unmergeable. Please resolve the merge conflicts. |
Let's please update that comment as suggested. @rustbot author After that, this looks OK to me as a lang and edition matter, and the impl looks OK to me, so r? compiler |
Reminder, once the PR becomes ready for a review, use |
also add `explicit-extern-abis` feature section to the unstable book.
008c19c
to
76c779f
Compare
@rustbot ready |
@obeis: 🔑 Insufficient privileges: Not in reviewers |
@obeis: 🔑 Insufficient privileges: not in try users |
@obeis: That part of my note there was directed at the compiler reviewer I assigned. |
I would like this to be implemented as an allow-by-default lint, likely called Having it as a lint makes it easy to deny on a later edition and makes the edition migration trivial. With the current implementation, I don't think we can easily do the "first make the crate muti-edition-valid, then update the edition" process, whereas with a lint one can just |
There already is a lint here ( |
Making it a hard error is in accordance with RFC 3722. In terms of process, once we add a new particular numbered edition (e.g. "Rust 2025"), the edition team will need to accept this change into that edition, and lang will need to make a decision to stabilize this in that edition. Until then, this hard error must remain under a feature flag and can only be present in the |
Oh, I see, I went a bit fast. All I want is a @future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::EditionError(Edition::EditionFuture),
reference: "issue #xxxx",
}; to the lint declaration. EDIT: oh, the lint is already warn-by-default so then there's no need for a new test. Still, this annotation would improve the error message. |
#[derive(Diagnostic)] | ||
#[diag(ast_passes_extern_without_abi)] | ||
pub(crate) struct MissingAbi { | ||
#[primary_span] | ||
pub span: Span, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hard error should have a suggestion too, in case someone changes the edition string by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about a suggestion in the new edition. One purpose of making this a hard error is that we don't want to suggest that "C" is the default any longer (and we may want to later consider making "Rust" the default). We do machine-applicable suggestions when we're really sure that's the right fix, but once we move away from this default, it's no longer actually clear that is what the user meant.
If we want to add some help text here telling the user that old editions inferred a default and that this is no longer true, and so the user needs to pick one explicitly, that'd of course be OK. If we want to go further and add a machine-applicable fix to this hard error, then I'd want a lang decision here to that effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help text sounds good, it solves the problem of "user updates edition without thinking, doesn't understand the new error".
This cannot be done. Please see my earlier review comment: |
The unstable
explicit_extern_abis
feature is introduced, requiring explicit ABIs inextern
blocks. Hard errors will be enforced with this feature enabled in a future edition.RFC rust-lang/rfcs#3722
Update #134986