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

Introduce core::marker::Tuple to properly type-check extern "rust-call" calls #537

Closed
1 of 3 tasks
compiler-errors opened this issue Jul 29, 2022 · 5 comments
Closed
1 of 3 tasks
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@compiler-errors
Copy link
Member

compiler-errors commented Jul 29, 2022

Proposal

Introduce core::marker::Tuple as a (for now, perma-unstable) builtin marker trait that is auto-implemented for all tuples, and require that calls to (and probably other related operations, like ptr coercion to) the extern "rust-call" ABI enforces that the tupled argument type implements core::marker::Tuple. Generic "rust-call" APIs will need to constrain their arguments, such as:

-impl<Args, F: FnOnce<Args> + ?Sized, A: Allocator> FnOnce<Args> for Box<F, A> {
+impl<Args: Tuple, F: FnOnce<Args> + ?Sized, A: Allocator> FnOnce<Args> for Box<F, A> {
     type Output = <F as FnOnce<Args>>::Output;

     extern "rust-call" fn call_once(self, args: Args) -> Self::Output {
         <F as FnOnce<Args>>::call_once(*self, args)
     }
 }

This really should be reflected in the type system, since we can't fully enforce that nested calls involving type parameters are actually tuples until monomorphization, but span information is lacking during monomorphization (and so is bubbling up errors at that point) and properly denying bad "rust-call" usages has to be threaded through each codegen crate.

For now, the scope of this marker trait is limited just to properly type check this quirk of the "rust-call" ABI. Further proposals will be needed if interest exists to flesh out this marker trait into something that can be used to introspect more tuple info (e.g. arity).

See: rust-lang/rust#99820, bjorn3/rustc_codegen_cranelift#1236, rust-lang/rust#66696. More probably exist.

Mentors or Reviewers

Anyone on t-compiler{,-contributors} who's familiar with rustc_typeck and rustc_trait_selection should be able to review. Changes should be pretty trivial, and I've got a working branch already in compiler-errors/rust@tuple-marker. it needs some diagnostic cleanup, but that's basically it...

Process

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

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.

@compiler-errors compiler-errors added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Jul 29, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 29, 2022

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.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jul 29, 2022
@compiler-errors
Copy link
Member Author

compiler-errors commented Jul 29, 2022

Also not sure if this is better as a t-lang proposal. On one hand, the Fn* traits are pretty well known, on the other, there are not one but two unstable features you need to enable to use Fn* traits in your own (nightly) code.

@eddyb
Copy link
Member

eddyb commented Jul 29, 2022

@rustbot second (I've been wanting to do some things in this area, and was expecting to remain WF-unsound for a while w/o a Tuple trait)

@crlf0710
Copy link
Member

cc @rust-lang/types

@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Aug 10, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 25, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 12, 2022
…kh726

Implement `std::marker::Tuple`

Split out from rust-lang#99943 (rust-lang#99943 (review)).

Implements part of rust-lang/compiler-team#537
r? `@jackh726`
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Implement `std::marker::Tuple`

Split out from #99943 (rust-lang/rust#99943 (review)).

Implements part of rust-lang/compiler-team#537
r? `@jackh726`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 6, 2022
Implement `std::marker::Tuple`, use it in `extern "rust-call"` and `Fn`-family traits

Implements rust-lang/compiler-team#537

I made a few opinionated decisions in this implementation, specifically:
1. Enforcing `extern "rust-call"` on fn items during wfcheck,
2. Enforcing this for all functions (not just ones that have bodies),
3. Gating this `Tuple` marker trait behind its own feature, instead of grouping it into (e.g.) `unboxed_closures`.

Still needing to be done:
1. Enforce that `extern "rust-call"` `fn`-ptrs are well-formed only if they have 1/2 args and the second one implements `Tuple`. (Doing this would fix ICE in rust-lang#66696.)
2. Deny all explicit/user `impl`s of the `Tuple` trait, kinda like `Sized`.
3. Fixing `Tuple` trait built-in impl for chalk, so that chalkification tests are un-broken.

Open questions:
1. Does this need t-lang or t-libs signoff?

Fixes rust-lang#99820
bors added a commit to rust-lang/chalk that referenced this issue Nov 10, 2022
Implement support for the `Tuple` trait

Necessary to due to new trait bounds required by rust-lang/compiler-team#537 / rust-lang/rust#99943.

r? `@jackh726`
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
Implement `std::marker::Tuple`, use it in `extern "rust-call"` and `Fn`-family traits

Implements rust-lang/compiler-team#537

I made a few opinionated decisions in this implementation, specifically:
1. Enforcing `extern "rust-call"` on fn items during wfcheck,
2. Enforcing this for all functions (not just ones that have bodies),
3. Gating this `Tuple` marker trait behind its own feature, instead of grouping it into (e.g.) `unboxed_closures`.

Still needing to be done:
1. Enforce that `extern "rust-call"` `fn`-ptrs are well-formed only if they have 1/2 args and the second one implements `Tuple`. (Doing this would fix ICE in #66696.)
2. Deny all explicit/user `impl`s of the `Tuple` trait, kinda like `Sized`.
3. Fixing `Tuple` trait built-in impl for chalk, so that chalkification tests are un-broken.

Open questions:
1. Does this need t-lang or t-libs signoff?

Fixes #99820
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Implement `std::marker::Tuple`

Split out from #99943 (rust-lang/rust#99943 (review)).

Implements part of rust-lang/compiler-team#537
r? `@jackh726`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Implement `std::marker::Tuple`

Split out from #99943 (rust-lang/rust#99943 (review)).

Implements part of rust-lang/compiler-team#537
r? `@jackh726`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

5 participants