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.

Sign up for GitHub

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

Jump to bottom

Fix PartialEq args when #[const_trait] is enabled #118379

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Fix PartialEq args when #[const_trait] is enabled #118379

merged 3 commits into from
Nov 30, 2023

Conversation

compiler-errors
Copy link
Member

This is based off of your PR that enforces effects on all methods, so just see the last commits.

r? fee1-dead

@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 Nov 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 27, 2023

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

compiler-errors
compiler-errors commented Nov 27, 2023
View reviewed changes
compiler/rustc_mir_build/src/build/matches/test.rs Outdated

let mut args: Vec<ty::GenericArg<'tcx>> = vec![ty.into(), ty.into()];
// If `PartialEq` is `#[const_trait]`, then add a const effect param
if self.tcx.generics_of(eq_def_id).host_effect_index.is_some() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I should make a helper fn that appends a const param and interns the generic args list, something like:

fn with_opt_const_effect_param(tcx: TyCtxt<'tcx>, caller_def_id: LocalDefId, callee_def_id: DefId, args: impl IntoIterator<Item = impl Into<GenericArg>>) -> &'tcx GenericArgs<'tcx> {}

That checks if the callee_def_id has a const effect param, and if so, constructs it based off of the caller_def_id, appends it, then interns the args...

Copy link
Member

@fee1-dead fee1-dead Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that would be very nice for more lang item migrations in the future.

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, it ended up being a lot better in #118386. I'll uplift it into this PR, which I think we can land after #118282.

fee1-dead
fee1-dead reviewed Nov 28, 2023
View reviewed changes
compiler/rustc_hir_typeck/src/callee.rs
}
None => tcx.consts.true_,
};
let effect = tcx.expected_const_effect_param_for_body(self.body_id);
Copy link
Member

Choose a reason for hiding this comment

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

we probably should have an assert here that self.body_id == tcx.hir().enclosing_body_owner(call_expr_hir)

@bors
Copy link
Contributor

bors commented Nov 28, 2023

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the const-params-for-partialeq branch from fd4ab16 to d3404d2 Compare November 28, 2023 21:18
@fee1-dead
Copy link
Member

fee1-dead commented Nov 29, 2023 •
edited
Loading

Thanks :)

@bors r+

@bors
Copy link
Contributor

bors commented Nov 29, 2023

📌 Commit d3404d2 has been approved by fee1-dead

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

bors commented Nov 29, 2023

⌛ Testing commit d3404d2 with merge 8ff3d1b...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2023
@bors
Auto merge of rust-lang#118379 - compiler-errors:const-params-for-par…
8ff3d1b 
…tialeq, r=fee1-dead

Fix `PartialEq` args when `#[const_trait]` is enabled

This is based off of your PR that enforces effects on all methods, so just see the last commits.

r? fee1-dead
@bors
Copy link
Contributor

bors commented Nov 29, 2023

💔 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 Nov 29, 2023
@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)

@compiler-errors
Copy link
Member Author

spurious
@bors retry

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

bors commented Nov 29, 2023

⌛ Testing commit d3404d2 with merge 4591bf1...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2023
@bors
Auto merge of rust-lang#118379 - compiler-errors:const-params-for-par…
4591bf1 
…tialeq, r=fee1-dead

Fix `PartialEq` args when `#[const_trait]` is enabled

This is based off of your PR that enforces effects on all methods, so just see the last commits.

r? fee1-dead
@bors
Copy link
Contributor

bors commented Nov 29, 2023

💔 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 Nov 29, 2023
@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)

@compiler-errors
Copy link
Member Author

@bors retry

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

bors commented Nov 30, 2023

⌛ Testing commit d3404d2 with merge e55544c...

@bors
Copy link
Contributor

bors commented Nov 30, 2023

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing e55544c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 30, 2023
@bors bors merged commit e55544c into rust-lang:master Nov 30, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 30, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e55544c): 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)		 - - 0
Improvements ✅
(secondary)		 -0.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) - - 0

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)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.3% [-2.7%, -2.0%] 3
All ❌✅ (primary) - - 0

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: 673.65s -> 673.827s (0.03%)
Artifact size: 313.41 MiB -> 313.40 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fee1-dead fee1-dead fee1-dead approved these changes

Assignees

@fee1-dead fee1-dead

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.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.
6 participants
@compiler-errors @rustbot @bors @fee1-dead @rust-log-analyzer @rust-timer