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

Require ABI for extern in 2021 #46

Closed
Mark-Simulacrum opened this issue Aug 4, 2020 · 4 comments
Closed

Require ABI for extern in 2021 #46

Mark-Simulacrum opened this issue Aug 4, 2020 · 4 comments
Labels
disposition-merge The FCP starter wants to merge (accept) this final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang

Comments

@Mark-Simulacrum
Copy link
Member

Proposal

Summary and problem statement

We do not currently require users to specify which ABI an extern corresponds to, in any context. This can be confusing (e.g., see rust-lang/rust#75030), and seems like an obvious case to fix. Requiring users to specify the ABI forces them to think through which ABI they want.

Motivation, use-cases, and solution sketches

The primary motivation is to ease reading code. Particularly with the introduction of "C-unwind" ABI, it seems increasingly true that knowing up front which ABI is associated with function pointers and function declarations is useful.

Prioritization

I think this best fits the targeted ergonomic wins -- similar to dyn, at least for me, seeing extern "C" is much clearer than just a bare extern. Though related to C Parity / embedded, it does not enable any new behavior that was absent previously.

Links and related work

C++ mandates only C and C++ ABIs, and requires an ABI to be specified on extern blocks, but not extern functions.

Initial people involved

No one beyond myself, at this point.

What happens now?

This issue is part of the experimental MCP process described in RFC 2936. Once this issue is filed, a Zulip topic will be opened for discussion, and the lang-team will review open MCPs in its weekly triage meetings. You should receive feedback within a week or two.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@Mark-Simulacrum Mark-Simulacrum added major-change Major change proposal T-lang labels Aug 4, 2020
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2020

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@rustbot rustbot added the to-announce Not yet announced MCP proposals label Aug 4, 2020
@nikomatsakis nikomatsakis added disposition-close The FCP starter wants to close this final-comment-period The FCP has started, most (if not all) team members are in agreement labels Aug 17, 2020
@nikomatsakis
Copy link
Contributor

Discussed in the lang-team team triage meeting today.

  • This should be a fairly simple change to make.
  • Some of us felt this was relatively poorly motivated, but @joshtriplett felt that implicit ABI was confusing to them when learning.
  • For the moment, the choice of C still feels likely to be the predominant thing people want. However, it is true that "C-unwind" or other ABIs may make "C" feel less obvious.
  • We're inclined to revisit this once "C-unwind" is implemented and stable.
  • We think we should kick this to the @rust-lang/wg-ffi-unwind project group -- perhaps a first step would be adding an "allow by default" lint to discuss, and move it to warn by default later if it makes sense? (That move would require a lang team FCP.)

Tagging for close since this can be addressed in @rust-lang/wg-ffi-unwind for now. Will revisit next triage meeting.

@nikomatsakis
Copy link
Contributor

Discussed in the meeting today. We're going to propose moving to an experimental implementation. Implement the lint and we can assess the impact via crater and make a decision whether to land with either allow-by-default or warn-by-default. From there, we could decide to make it an warning or error in a new edition. So closing as "approved" but without the requirement for a project group or RFC.

Also relevant, rustfmt already adds the ABI by default.

Note for future: There are also some discussion on Zulip on "related but orthogonal" issues such as name mangling and symbol export. We are not trying to solve those via this lint but we may want to address them at some other point.

@nikomatsakis nikomatsakis added disposition-merge The FCP starter wants to merge (accept) this and removed disposition-close The FCP starter wants to close this labels Aug 31, 2020
@Mark-Simulacrum Mark-Simulacrum added disposition-close The FCP starter wants to close this disposition-merge The FCP starter wants to merge (accept) this and removed disposition-merge The FCP starter wants to merge (accept) this to-announce Not yet announced MCP proposals disposition-close The FCP starter wants to close this labels Aug 31, 2020
@Mark-Simulacrum
Copy link
Member Author

For those subscribed to this thread, dropping a link to a PR implementing this: rust-lang/rust#76219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge The FCP starter wants to merge (accept) this final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang
Projects
None yet
Development

No branches or pull requests

3 participants