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

Implement std::marker::Tuple, use it in extern "rust-call" and Fn-family traits #99943

Merged
merged 6 commits into from
Nov 6, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jul 30, 2022

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 ICE when transmuting to extern "rust-call" fn() #66696.)
  2. Deny all explicit/user impls 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

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 30, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2022
@compiler-errors
Copy link
Member Author

r? compiler

@rust-highfive rust-highfive assigned jackh726 and unassigned thomcc Jul 30, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 30, 2022

☔ The latest upstream changes (presumably #99948) made this pull request unmergeable. Please resolve the merge conflicts.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 30, 2022

This does need @rust-lang/libs-api and @rust-lang/lang review, but as long as all of it is still marked unstable, I don't think it needs an FCP.

For my part, 👍 for lang and for libs-api, for the proposed surface area. Others should chime in as well if they have thoughts or concerns, though.

Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

Can we split this into parts? First part is just adding the Tuple trait and implementing it?

@compiler-errors compiler-errors added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2022
@compiler-errors
Copy link
Member Author

compiler-errors commented Aug 10, 2022

blocking on landing that other one

@cjgillot
Copy link
Contributor

Will this also fix the ICE in #57404?


error: functions with the "rust-call" ABI must take a single non-self argument that is a tuple
--> $DIR/issue-22565-rust-call.rs:9:5
| ^ `i32` is not a tuple
Copy link
Contributor

Choose a reason for hiding this comment

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

This span is weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we could probably pass the arg span into the obligation cause or something to make this better

|
LL | extern "rust-call" fn a() {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^
| ^
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep the def_span here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I think so

@compiler-errors
Copy link
Member Author

compiler-errors commented Aug 10, 2022

Will this also fix the ICE in #57404?

Yes, just checked.

@compiler-errors
Copy link
Member Author

@cjgillot with this PR, that issue now reports:

error[E0277]: `&mut ()` is not a tuple
   --> /home/gh-compiler-errors/test.rs:6:41
    |
6   |     handlers.unwrap().as_mut().call_mut(&mut ());
    |                                -------- -^^^^^^
    |                                |        |
    |                                |        `&mut ()` is not a tuple
    |                                |        help: consider removing the leading `&`-reference
    |                                required by a bound introduced by this call
    |
    = help: the trait `Tuple` is not implemented for `&mut ()`
note: required by a bound in `call_mut`
   --> /home/gh-compiler-errors/rust/library/core/src/ops/function.rs:334:23
    |
334 | pub trait FnMut<Args: Tuple>: FnOnce<Args> {
    |                       ^^^^^ required by this bound in `call_mut`

error: aborting due to previous error

instead of ICEing.

bors added a commit to rust-lang-ci/rust that referenced this pull request 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`
@crlf0710
Copy link
Member

Now that #100251 has landed, seems this is now unblocked.

workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request 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`
@compiler-errors
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 2, 2022

⌛ Trying commit 52655b8d45d36b35cf6b3bcc0174f7aa45406255 with merge 388b725dea7416978ab58ab1f908139d530308a7...

@jackh726
Copy link
Member

jackh726 commented Nov 6, 2022

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Nov 6, 2022

📌 Commit ff8f84c has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 6, 2022
@bors
Copy link
Contributor

bors commented Nov 6, 2022

⌛ Testing commit ff8f84c with merge 7eef946...

@bors
Copy link
Contributor

bors commented Nov 6, 2022

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 7eef946 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 6, 2022
@bors bors merged commit 7eef946 into rust-lang:master Nov 6, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 6, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7eef946): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.4%, -0.6%] 3
Improvements ✅
(secondary)
-3.0% [-4.0%, -0.4%] 7
All ❌✅ (primary) -1.1% [-1.4%, -0.6%] 3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.5% [0.6%, 2.5%] 2
Regressions ❌
(secondary)
2.3% [2.2%, 2.3%] 2
Improvements ✅
(primary)
-1.0% [-1.6%, -0.5%] 2
Improvements ✅
(secondary)
-5.5% [-5.5%, -5.5%] 1
All ❌✅ (primary) 0.2% [-1.6%, 2.5%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
4.6% [4.6%, 4.6%] 1
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
-2.5% [-2.5%, -2.5%] 1
All ❌✅ (primary) 0.0% [-0.5%, 0.5%] 2

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…mpls, r=compiler-errors

Fix `const_fn_trait_ref_impl`, add test for it

rust-lang#99943 broke `#[feature(const_fn_trait_ref_impl)]`, this PR fixes this and adds a test for it.

r? `@fee1-dead`
@Mark-Simulacrum
Copy link
Member

Delta here appears to be noise (but is an improvement anyway).

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…mpls, r=compiler-errors

Fix `const_fn_trait_ref_impl`, add test for it

rust-lang#99943 broke `#[feature(const_fn_trait_ref_impl)]`, this PR fixes this and adds a test for it.

r? ``@fee1-dead``
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…mpls, r=compiler-errors

Fix `const_fn_trait_ref_impl`, add test for it

rust-lang#99943 broke `#[feature(const_fn_trait_ref_impl)]`, this PR fixes this and adds a test for it.

r? ```@fee1-dead```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 8, 2022
…mpls, r=compiler-errors

Fix `const_fn_trait_ref_impl`, add test for it

rust-lang#99943 broke `#[feature(const_fn_trait_ref_impl)]`, this PR fixes this and adds a test for it.

r? ````@fee1-dead````
BGluth added a commit to BGluth/Mocktopus that referenced this pull request Nov 9, 2022
- Caused by `Fn` and related traits now requiring their arguments to
  impl `Tuple`. (PR: rust-lang/rust#99943)
- This PR adds constraints where needed to also require `Tuple` to be
  implemented.
- Also the feature gate `const_fn` no longer exists and was causing a
  compilation error, and this PR also removes its usage.
- Not very familiar with the internals of this crate, so please let me
  know if this PR should fix anything differently!
bors added a commit to rust-lang/chalk that referenced this pull request 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`
Hpmason added a commit to Hpmason/retour-rs that referenced this pull request Nov 14, 2022
trha pushed a commit to trha/detour-rs that referenced this pull request Jan 13, 2023
@compiler-errors compiler-errors deleted the tuple-trait branch August 11, 2023 20:21
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request 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 pull request 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
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: compiler/rustc_middle/src/ty/layout.rs:3199:21: argument to function with "rust-call" ABI is not a tuple