Skip to content

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Sep 28, 2025

First commit treats all constants containing a macro call as non-local and renames eval_simple to be a little clearer.

Second commit removes CoreConstant and treats most of them as though they were local. A couple of the constants like usize::MAX are treated as non-local as different targets may have different values. const_is_empty will now ignore non-local constants since there's no guarantee that they are the same across all targets/features/versions.

The third commit just changes some eval calls to eval_local. Most of these were style lints which shouldn't be assuming the value of a constant won't ever change.

changelog: none

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 28, 2025

r? @samueltardieu

rustbot has assigned @samueltardieu.
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

This comment has been minimized.

@samueltardieu
Copy link
Member

☔ The latest upstream changes made this pull request unmergeable. Please resolve the merge conflicts.

It would be great if you could rebase before opening a new PR, many of them seem to be out-of-date recently as soon as they are created which makes the review more involved.

Copy link
Member

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

Rebasing should also let lintcheck run.

I've also noted that some (preexisting) code paths are never exercized, such as ExprKind::ConstBlock or ExprKind::DropTemps constant folding, as removing the cases from ConstEvalCtxt::expr() doesn't change the output of the tests.

View changes since this review

pub fn eval_with_source(&self, e: &Expr<'_>) -> Option<(Constant<'tcx>, ConstantSource)> {
pub fn eval_with_source(&self, e: &Expr<'_>, ctxt: SyntaxContext) -> Option<(Constant<'tcx>, ConstantSource)> {
self.source.set(ConstantSource::Local);
self.ctxt.set(ctxt);
Copy link
Member

Choose a reason for hiding this comment

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

Using e.span.ctxt() here instead of ctxt still passes 100% of the tests and does not require this extra parameter. If this new parameter is needed, can you document it and produce tests in which it makes a difference? If it is not, can you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a prerequisite for making lints work with macros better. Tests will have to wait until the lints themselves have been gone through to handle macros correctly.

Docs have been added to explain what the parameter is for.

Comment on lines 576 to 580
self.source.set(if self.ctxt.get() == ctxt {
self.source.get()
} else {
false
};
self.fetch_path_and_apply(qpath, hir_id, self.typeck.node_type(hir_id), |self_, result| {
let result = mir_to_const(self_.tcx, result)?;
// If source is already Constant we wouldn't want to override it with CoreConstant
self_.source.set(
if is_core_crate && !matches!(self_.source.get(), ConstantSource::Constant) {
ConstantSource::CoreConstant
} else {
ConstantSource::Constant
},
);
Some(result)
})
ConstantSource::Constant
});
Copy link
Member

Choose a reason for hiding this comment

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

This is easier to read:

    fn check_ctxt(&self, ctxt: SyntaxContext) {
        if self.ctxt.get() != ctxt {
            self.source.set(ConstantSource::Constant);
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow github is displaying that range poorly. This is leftover from when I was trying to convince llvm to generate a cmov for this. This has been changed.

/// Simple constant folding: Insert an expression, get a constant or none.
fn expr(&self, e: &Expr<'_>) -> Option<Constant<'tcx>> {
fn expr(&self, e: &Expr<'_>) -> Option<Constant> {
self.check_ctxt(e.span.ctxt());
Copy link
Member

Choose a reason for hiding this comment

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

Removing this call to self.check_ctxt() has no effect. Could you add a counter-example in tests or remove this call?

fn expr(&self, e: &Expr<'_>) -> Option<Constant> {
self.check_ctxt(e.span.ctxt());
match e.kind {
ExprKind::ConstBlock(ConstBlock { body, .. }) => self.expr(self.tcx.hir_body(body).value),
Copy link
Member

Choose a reason for hiding this comment

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

This seems to never be tested.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 29, 2025
@rustbot

This comment has been minimized.

Copy link

github-actions bot commented Sep 30, 2025

Lintcheck changes for 47b0f90

Lint Added Removed Changed
clippy::float_cmp 0 2 1
clippy::float_cmp_const 2 0 0
clippy::manual_is_infinite 0 2 0

This comment will be updated if you push new changes

@samueltardieu
Copy link
Member

That is a lot of lintcheck changes, and from just browsing them it looks like some (many?) of them are unwarranted.

@Jarcho Jarcho force-pushed the const_eval1 branch 2 times, most recently from 0db692b to 51c9fb9 Compare September 30, 2025 12:42
@samueltardieu
Copy link
Member

The clippy::float_cmp_const and clippy::invalid_upcast_comparisons lintcheck changes are suspicious, and the clippy::assertions_on_constants lintcheck change should probably not happen.

@Jarcho
Copy link
Contributor Author

Jarcho commented Sep 30, 2025

clippy::float_cmp_const is more correct than it was before since they used to trigger on clippy::float_cmp. Fixing it requires changing the lint itself.

clippy::float_cmp_const is technically wrong, but not really a problem. This is the only lint that cares about named constants like this and there's a PR to change how it works.

clippy::assertions_on_constants was a change that I forgot to remove (oops).

clippy::invalid_upcast_comparisons is partially from the lint not handling macros, the old const eval macro handling was just hiding it. It should be fixed by some more aggressive context checks, but the lint still needs to be fixed at some point.

@Jarcho Jarcho force-pushed the const_eval1 branch 2 times, most recently from 4db4b32 to 245c53f Compare September 30, 2025 16:15
@Jarcho
Copy link
Contributor Author

Jarcho commented Sep 30, 2025

Had to reorder this and #15774 since cfg based constants are now treated as non-local rather than non-evaluable.

@Jarcho Jarcho added the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Sep 30, 2025
@Jarcho
Copy link
Contributor Author

Jarcho commented Sep 30, 2025

The change in clippy::manual_is_infinite is unfortunate, but not really a problem. Changing $ty::INFINITY isn't really guaranteed to work, but core::$module::INFINITY could be changed without an issue.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@Jarcho Jarcho removed the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Sep 30, 2025
@samueltardieu samueltardieu added this pull request to the merge queue Oct 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 4, 2025
Copy link
Member

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

Those changes consider NaN and 0.0 / 0.0 as being equivalent NaNs on OSX/aarch64, which makes tests/ui/if_same_then_else fail at line 76. This doesn't happen on Linux/x86_64.

View changes since this review

@rustbot
Copy link
Collaborator

rustbot commented Oct 4, 2025

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Jarcho added 2 commits October 4, 2025 10:41
* Remove `CoreConstant`.
* Treat most constants from core as though they were inlined.
* Don't evaluate `is_empty` for named constants.
@Jarcho
Copy link
Contributor Author

Jarcho commented Oct 4, 2025

Constant::eq was comparing floats by bits without checking for NaN. There's still a bug where == and != consider 0.0 and -0.0 to be different. I have that fixed in a rewrite which I'll have finished at some point.

span,
..
}) = self.tcx.hir_node(body_id.hir_id)
&& is_direct_expn_of(*span, sym::cfg).is_some()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @Jarcho, I had a PR (#15665) that touched this part of code, and I'm not really sure how I should adapt the changes to this PR, so I wanted to ask for some help.

Basically, the code right here used to check whether the const was defined as const C: _ = cfg!(..);, and what I added was another check for #[cfg(..)] const C: _ = ..;. But it looks like your changes completely removed the former check, and so I'm not really sure where to add the latter.

Thanks in advance:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants