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

Get rid of HIR const checker #133321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Nov 22, 2024

As far as I can tell, the HIR const checker was implemented in #66170 because we were not able to issue useful const error messages in the MIR const checker.

This seems to have changed in the last 5 years, probably due to work like #90532. I've tweaked the diagnostics slightly and think the error messages have gotten better in fact.

Thus I think the HIR const checker has reached the end of its usefulness, and we can retire it.

cc @RalfJung

@rustbot
Copy link
Collaborator

rustbot commented Nov 22, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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 Nov 22, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 22, 2024

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@RalfJung
Copy link
Member

I didn't even know we had another const checker...
Cc @rust-lang/wg-const-eval

| |_____^
|
= note: see issue #87575 <https://github.com/rust-lang/rust/issues/87575> for more information
= help: add `#![feature(const_for)]` to the crate attributes to enable
Copy link
Member

Choose a reason for hiding this comment

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

This helpful hint seems to have been lost?

Copy link
Member Author

Choose a reason for hiding this comment

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

Notably, enabling this gate is neither necessary nor sufficient under const traits. I should have noted that.

If and when we work out how cons stability works for const traits it may, but it's just misleading today.

Copy link
Member

Choose a reason for hiding this comment

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

It's just quite frustrating to get a nightly feature error without any hint about what the feature gate is. If I first get an error saying to enable const_for, and then another error saying to enable const_trait_impl, that's a lot better IMO.

Or does the const_for gate not actually do anything right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAICT it does not do anything.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. Then it should mention const_trait_impl, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Iterator is not a const trait yet.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see.

Yeah then the new error is fine. Maybe entirely remove the const_for symbol (and the others that were used only in the HIR const checker).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants