Skip to content

Use constness query for const_trait, too #134114

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
Closed
wants to merge 2 commits into from
Closed

Use constness query for const_trait, too #134114

wants to merge 2 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 10, 2024 •
edited
Loading

We also use that query for impls, so it seems kinda nice to put everything into one location

cc @compiler-errors

@rustbot
Copy link
Collaborator

rustbot commented Dec 10, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Dec 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 10, 2024

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@fee1-dead
Copy link
Member

I think for the long term we should be moving away from using the constness query for everything and try to decouple them, so I'm not sure if this is an improvement strictly speaking..

Also, if we move from #[const_trait] trait Tr to ~const trait Tr or something else, it is more natural to me to stick that to the hir ItemKind variant, then get it lowered into the trait_def in ty.

@oli-obk oli-obk closed this Dec 10, 2024
@oli-obk oli-obk deleted the push-lrkzrnqsrtkq branch December 10, 2024 11:59
@oli-obk oli-obk mentioned this pull request Dec 10, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 13, 2024
@bors
Auto merge of rust-lang#134122 - oli-obk:push-zqnyznxtpnll, r=petroch…
e217f94 
…enkov

Move impl constness into impl trait header

This PR is kind of the opposite of the rejected rust-lang#134114

Instead of moving more things into the `constness` query, we want to keep them where their corresponding hir nodes are lowered. So I gave this a spin for impls, which have an obvious place to be (the impl trait header). And surprisingly it's also a perf improvement (likely just slightly better query & cache usage).

The issue was that removing anything from the `constness` query makes it just return `NotConst`, which is wrong. So I had to change it to `bug!` out if used wrongly, and only then remove the impl blocks from the `constness` query. I think this change is good in general, because it makes using `constness` more robust (as can be seen by how few sites that had to be changed, so it was almost solely used specifically for the purpose of asking for functions' constness). The main thing where this change was not great was in clippy, which was using the `constness` query as a general DefId -> constness map. I added a `DefKind` filter in front of that. If it becomes a more common pattern we can always move that helper into rustc.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 15, 2024
@bors
Auto merge of #134122 - oli-obk:push-zqnyznxtpnll, r=petrochenkov
d613c11 
Move impl constness into impl trait header

This PR is kind of the opposite of the rejected rust-lang/rust#134114

Instead of moving more things into the `constness` query, we want to keep them where their corresponding hir nodes are lowered. So I gave this a spin for impls, which have an obvious place to be (the impl trait header). And surprisingly it's also a perf improvement (likely just slightly better query & cache usage).

The issue was that removing anything from the `constness` query makes it just return `NotConst`, which is wrong. So I had to change it to `bug!` out if used wrongly, and only then remove the impl blocks from the `constness` query. I think this change is good in general, because it makes using `constness` more robust (as can be seen by how few sites that had to be changed, so it was almost solely used specifically for the purpose of asking for functions' constness). The main thing where this change was not great was in clippy, which was using the `constness` query as a general DefId -> constness map. I added a `DefKind` filter in front of that. If it becomes a more common pattern we can always move that helper into rustc.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Dec 15, 2024
@bors
Auto merge of #134122 - oli-obk:push-zqnyznxtpnll, r=petrochenkov
8a09650 
Move impl constness into impl trait header

This PR is kind of the opposite of the rejected rust-lang/rust#134114

Instead of moving more things into the `constness` query, we want to keep them where their corresponding hir nodes are lowered. So I gave this a spin for impls, which have an obvious place to be (the impl trait header). And surprisingly it's also a perf improvement (likely just slightly better query & cache usage).

The issue was that removing anything from the `constness` query makes it just return `NotConst`, which is wrong. So I had to change it to `bug!` out if used wrongly, and only then remove the impl blocks from the `constness` query. I think this change is good in general, because it makes using `constness` more robust (as can be seen by how few sites that had to be changed, so it was almost solely used specifically for the purpose of asking for functions' constness). The main thing where this change was not great was in clippy, which was using the `constness` query as a general DefId -> constness map. I added a `DefKind` filter in front of that. If it becomes a more common pattern we can always move that helper into rustc.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Dec 23, 2024
@bors
Auto merge of #134122 - oli-obk:push-zqnyznxtpnll, r=petrochenkov
c6288ab 
Move impl constness into impl trait header

This PR is kind of the opposite of the rejected rust-lang/rust#134114

Instead of moving more things into the `constness` query, we want to keep them where their corresponding hir nodes are lowered. So I gave this a spin for impls, which have an obvious place to be (the impl trait header). And surprisingly it's also a perf improvement (likely just slightly better query & cache usage).

The issue was that removing anything from the `constness` query makes it just return `NotConst`, which is wrong. So I had to change it to `bug!` out if used wrongly, and only then remove the impl blocks from the `constness` query. I think this change is good in general, because it makes using `constness` more robust (as can be seen by how few sites that had to be changed, so it was almost solely used specifically for the purpose of asking for functions' constness). The main thing where this change was not great was in clippy, which was using the `constness` query as a general DefId -> constness map. I added a `DefKind` filter in front of that. If it becomes a more common pattern we can always move that helper into rustc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@fee1-dead fee1-dead

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oli-obk @rustbot @fee1-dead