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

[RFC] Explicit ABI in extern #3722

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Oct 30, 2024

@m-ou-se m-ou-se added T-lang Relevant to the language team, which will review and decide on the RFC. A-defaults Proposals relating to defaults / provided definitions I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. A-edition-2024 Area: The 2024 edition A-maybe-future-edition Changes of edition-relevance not targeted for a specific edition. T-edition Relevant to the edition team, which will review and decide on this RFC. labels Oct 30, 2024
@joshtriplett
Copy link
Member

joshtriplett commented Oct 30, 2024

Very much a fan of making this change. As noted in the RFC, it's very late for the 2024 edition. That said, I would personally not object to trying for 2024 if we can get the necessary migration lint in place.

One way or another, I personally think T-lang should approve this RFC, with the guidance of "whenever it's ready", and defer to those wrangling the edition to decide whether this happens in 2024 or 2027.

@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 30, 2024

Implementation, including migration lint: rust-lang/rust#132357

@traviscross
Copy link
Contributor

@rustbot labels -A-edition-2024

This makes sense to me as a language matter also.

With the edition hat on, though, and after consulting with the rest of the edition team, it's unfortunately just too late to accept a new language item like this for Rust 2024. This is due to the testing and release timeline, where we are in it, and how these kind of changes affect that.

Let's make sure it makes the next edition, assuming the rest of the lang team likes this also.

@rustbot rustbot removed the A-edition-2024 Area: The 2024 edition label Oct 30, 2024
@tmandry tmandry removed the T-edition Relevant to the edition team, which will review and decide on this RFC. label Oct 30, 2024
@tmandry
Copy link
Member

tmandry commented Oct 30, 2024

The lang team discussed this in our meeting today and everyone was in favor of the change. Since it's too late for 2024 Edition, this RFC is not time sensitive, but we might as well approve it for the next edition.

Meanwhile, there is an existing missing_abi lint that we should upgrade to warn-by-default in all editions. We should do that in a separate FCP. (There was some discussion of rolling this out starting with 2024 – but that should not happen as part of the initial edition release, or it would fall under the testing and documentation requirements that we already said it was too late to do for 2024.)

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Oct 30, 2024

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Oct 30, 2024
@tmandry tmandry added the T-edition Relevant to the edition team, which will review and decide on this RFC. label Oct 30, 2024
# Future possibilities

In the future, we might want to add a new default ABI.
For example, if `extern "stable-rust-abi"` becomes a thing and e.g. dynamically linking Rust from Rust becomes very popular, it might make sense to make that the default when writing `extern fn` without an ABI.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think, if there is a default, it should be anything other than "C" because it raises the stakes of getting the edition wrong.

It would mean that getting the edition parameter wrong causes a breakage in ABI. Which is fine if it's caught by an error but this is not guaranteed, it could also just lead to silent UB, segfaults, etc.

There is some such flags in Rust already, but flags have been removed with it affecting the ABI being one of the reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. The future possibilities are a separate discussion if/when they are proposed. Definitely not part of what I'm proposing now. :)

Copy link
Member

@est31 est31 Nov 1, 2024

Choose a reason for hiding this comment

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

It is easy to read anything in the future possibilities section as an endorsement of that idea. It would be nice to either remove that paragraph or incorporate the known issues into the text.

Also I doubt that dynamically linking Rust to Rust will become more popular than interfacing with the non-Rust world, which (mostly) goes via the "C" ABI.

Copy link
Member

@joshtriplett joshtriplett Nov 20, 2024

Choose a reason for hiding this comment

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

It is easy to read anything in the future possibilities section as an endorsement of that idea.

This is something that we really, really should try to avert in the RFC process. Accepting an RFC is not any kind of endorsement or directional approval of things in the future possibilities section.

@scottmcm
Copy link
Member

I like being explicit here. The burden of also saying "C" is small compared to also having to have said extern, so might as well -- especially as more people actually want "C-unwind".

@rfcbot reviewed

I agree that we probably shouldn't change to a new default for some time, if ever.

@traviscross
Copy link
Contributor

@rfcbot reviewed
@rustbot labels -I-lang-nominated

We discussed this in lang triage today, resulting in tmandry's proposed FCP above, which sounds right to me.

@rustbot rustbot removed the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Oct 30, 2024
@m-ou-se
Copy link
Member Author

m-ou-se commented Oct 31, 2024

Meanwhile, there is an existing missing_abi lint that we should upgrade to warn-by-default in all editions. We should do that in a separate FCP.

This PR makes the lint warn-by-default: rust-lang/rust#132397

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 31, 2024
…rrors

Improve missing_abi lint

This is for the migration lint for rust-lang/rfcs#3722

It is not yet marked as an edition migration lint, because `Edition2027` doesn't exist yet.

The lint now includes a machine applicable suggestion:

```
warning: extern declarations without an explicit ABI are deprecated
 --> src/main.rs:3:1
  |
3 | extern fn a() {}
  | ^^^^^^ help: explicitly specify the C ABI: `extern "C"`
  |
```
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 1, 2024
…rrors

Improve missing_abi lint

This is for the migration lint for rust-lang/rfcs#3722

It is not yet marked as an edition migration lint, because `Edition2027` doesn't exist yet.

The lint now includes a machine applicable suggestion:

```
warning: extern declarations without an explicit ABI are deprecated
 --> src/main.rs:3:1
  |
3 | extern fn a() {}
  | ^^^^^^ help: explicitly specify the C ABI: `extern "C"`
  |
```
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2024
Rollup merge of rust-lang#132357 - m-ou-se:explicit-abi, r=compiler-errors

Improve missing_abi lint

This is for the migration lint for rust-lang/rfcs#3722

It is not yet marked as an edition migration lint, because `Edition2027` doesn't exist yet.

The lint now includes a machine applicable suggestion:

```
warning: extern declarations without an explicit ABI are deprecated
 --> src/main.rs:3:1
  |
3 | extern fn a() {}
  | ^^^^^^ help: explicitly specify the C ABI: `extern "C"`
  |
```
@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Nov 20, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 20, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔


- This is a breaking change and needs to be done in a new edition.

# Prior art
Copy link
Contributor

@madsmtm madsmtm Nov 29, 2024

Choose a reason for hiding this comment

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

Other prior art: rustfmt converts extern {} to extern "C" {}, see the docs.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 30, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 30, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-defaults Proposals relating to defaults / provided definitions A-maybe-future-edition Changes of edition-relevance not targeted for a specific edition. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-edition Relevant to the edition team, which will review and decide on this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants