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

properly elaborate effects implied bounds for super traits #129499

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

fee1-dead
Copy link
Member

@fee1-dead fee1-dead commented Aug 24, 2024

Summary: This PR makes it so that we elaborate <T as Tr>::Fx: EffectsCompat<somebool> into <T as SuperTr>::Fx: EffectsCompat<somebool> when we know that trait Tr: ~const SuperTr.

Some discussion at rust-lang/project-const-traits#2.

r? project-const-traits
@rust-lang/project-const-traits: how do we feel about this approach?

@rustbot

This comment was marked as outdated.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 24, 2024
@fee1-dead fee1-dead added F-const_trait_impl `#![feature(const_trait_impl)]` F-effects `#![feature(effects)]` PG-const-traits Project group: Const traits labels Aug 24, 2024
@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member Author

anyone from @rust-lang/project-const-traits willing to give a vibe check/review? ^^'

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I'm still trying to wrap my head around what's going on here. Perhaps some more comments and detailed explanation would help?

Sorry my life has been too chaotic to sit down and focus on what's going on here. I'll try to respond a bit more quickly moving forward.

compiler/rustc_type_ir/src/elaborate.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/bounds.rs Outdated Show resolved Hide resolved
compiler/rustc_type_ir/src/elaborate.rs Outdated Show resolved Hide resolved
compiler/rustc_type_ir/src/elaborate.rs Outdated Show resolved Hide resolved
compiler/rustc_type_ir/src/elaborate.rs Outdated Show resolved Hide resolved
@fee1-dead
Copy link
Member Author

Perhaps some more comments and detailed explanation would help?

Yeah. I'll write more on this when I get time

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2024
@compiler-errors
Copy link
Member

Also, semi-related: I found a bug when looking at the (pre-existing) changes to push_trait_bound: #130666. See the second commit.

Specifically, we should only push an effects predicate for a trait if we're filtering to implied predicates, not super predicates.

@fee1-dead
Copy link
Member Author

to do with performance

.. Hold on, let me just write stuff here because I'm going to sleep and too lazy right now to open up the code and tweak the comments. (also ping since you might have unsubscribed @compiler-errors)

There is a difference in what this does here vs. push_trait_bound that isn't just performance.

From a distant, high-level look, push_trait_bound implements the ~const modifier for a trait bound in terms of bounds on effects. It desugars to the normal non-const bound and adds requirements for the const context. This is placed there because push_trait_bound has access to HIR and we lose the information (in reality, encoded as a separate bound) whether a bound is ~const or not when we lower to MIR (in pre-effects, we used to attach those to the trait predicate and expected the trait solver to handle them: #114202)

what this PR does is it implements the propagation of ~const super trait implications. If I have trait Bar: ~const Foo and const fn foo<T: ~const Bar>(), I should expect to be able to use T as Foo in a const context as well. For non-const traits, the assumption T: Bar is elaborated to T: Foo in here as well. (during elaboration) We don't implement it in push_trait_bound the same reason we don't push T: Foo to const fn foo<T: Bar>().

@bors

This comment was marked as resolved.

@fee1-dead fee1-dead force-pushed the supereffects branch 2 times, most recently from 242f51c to fc272b5 Compare September 27, 2024 14:34
@fee1-dead
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2024
@compiler-errors
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Sep 29, 2024

📌 Commit 7c2a24b has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 29, 2024
…mpiler-errors

properly elaborate effects implied bounds for super traits

Summary: This PR makes it so that we elaborate `<T as Tr>::Fx: EffectsCompat<somebool>` into `<T as SuperTr>::Fx: EffectsCompat<somebool>` when we know that `trait Tr: ~const SuperTr`.

Some discussion at rust-lang/project-const-traits#2.

r? project-const-traits
`@rust-lang/project-const-traits:` how do we feel about this approach?
@bors
Copy link
Contributor

bors commented Sep 29, 2024

⌛ Testing commit 7c2a24b with merge 0e67c64...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Sep 29, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 29, 2024
@fee1-dead
Copy link
Member Author

@bors retry spurious

@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 Sep 29, 2024
@bors
Copy link
Contributor

bors commented Sep 30, 2024

⌛ Testing commit 7c2a24b with merge 4e91ced...

@bors
Copy link
Contributor

bors commented Sep 30, 2024

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 4e91ced to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 30, 2024
@bors bors merged commit 4e91ced into rust-lang:master Sep 30, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 30, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4e91ced): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.3% [0.3%, 0.3%] 4
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% [-2.0%, -2.0%] 1
All ❌✅ (primary) 0.3% [0.3%, 0.3%] 4

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 768.13s -> 768.577s (0.06%)
Artifact size: 341.39 MiB -> 341.42 MiB (0.01%)

@rustbot rustbot added the perf-regression Performance regression. label Sep 30, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Oct 1, 2024

The small regression looks genuine, but it's on nalgebra, which doesn't have its statistical threshold configured reasonably yet, since it has been in the suite for only a week.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 1, 2024
@fee1-dead fee1-dead deleted the supereffects branch October 1, 2024 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_trait_impl `#![feature(const_trait_impl)]` F-effects `#![feature(effects)]` merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. PG-const-traits Project group: Const traits 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants