Const eval changes by Jarcho · Pull Request #15773 · rust-lang/rust-clippy

Jarcho · 2025-09-28T17:13:42Z

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 · 2025-09-28T17:13:46Z

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

samueltardieu · 2025-09-29T11:27:21Z

☔ 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.

samueltardieu

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

samueltardieu · 2025-09-29T11:33:12Z

-    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);


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?

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.

samueltardieu · 2025-09-29T11:39:40Z

+        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
+        });


This is easier to read:

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

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.

samueltardieu · 2025-09-29T11:44:42Z

    /// 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());


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

samueltardieu · 2025-09-29T11:45:26Z

+    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),


This seems to never be tested.

github-actions · 2025-09-30T11:15:57Z

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 · 2025-09-30T11:36:15Z

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

samueltardieu · 2025-09-30T13:38:57Z

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 · 2025-09-30T13:53:21Z

~~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 · 2025-09-30T16:17:17Z

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

Jarcho · 2025-09-30T18:06:00Z

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.

…an argument.

samueltardieu

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 · 2025-10-04T14:24:42Z

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.

* Remove `CoreConstant`. * Treat most constants from core as though they were inlined. * Don't evaluate `is_empty` for named constants.

Jarcho · 2025-10-04T14:42:09Z

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.

ada4a · 2025-10-04T19:32:37Z

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


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:)

friendly ping @Jarcho

Zalathar · 2026-07-22T08:28:19Z

This change seems to have regressed assert!(cfg!(..)) #7319, as seen at rust-lang/rust#159705 (comment).

Jarcho · 2026-07-22T09:17:18Z

This is a more general problem with assertions_on_constants where it can't tell if you want to restrict the condition only a specific code path or in general. The cfg part isn't really important since it applies to any expression where the value is determined by some non-local thing. Should probably just split the non-local part into it's own lint.

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

rustbot assigned samueltardieu Sep 28, 2025

This comment has been minimized.

Sign in to view

Jarcho mentioned this pull request Sep 28, 2025

assertions_on_constants: Suggest using a const block when using a named constant #15774

Merged

samueltardieu requested changes Sep 29, 2025

View reviewed changes

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

Jarcho force-pushed the const_eval1 branch from 0457b28 to d323885 Compare September 30, 2025 11:06

This comment has been minimized.

Sign in to view

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

Jarcho force-pushed the const_eval1 branch from 51c9fb9 to c030b59 Compare September 30, 2025 13:49

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

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 force-pushed the const_eval1 branch from 245c53f to 21de80e Compare September 30, 2025 16:21

This comment has been minimized.

Sign in to view

Jarcho force-pushed the const_eval1 branch from 21de80e to c535b2e Compare September 30, 2025 21:23

This comment has been minimized.

Sign in to view

Jarcho removed the S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work label Sep 30, 2025

Rename eval_simple to eval_local and take the SyntaxContext as …

3e24d50

…an argument.

samueltardieu approved these changes Oct 4, 2025

View reviewed changes

samueltardieu added this pull request to the merge queue Oct 4, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 4, 2025

samueltardieu requested changes Oct 4, 2025

View reviewed changes

Jarcho force-pushed the const_eval1 branch from c535b2e to 1621e49 Compare October 4, 2025 14:24

Jarcho added 2 commits October 4, 2025 10:41

Const eval changes:

3d351c8

* Remove `CoreConstant`. * Treat most constants from core as though they were inlined. * Don't evaluate `is_empty` for named constants.

Use eval_local in more places.

47b0f90

Jarcho force-pushed the const_eval1 branch from 1621e49 to 47b0f90 Compare October 4, 2025 14:42

samueltardieu approved these changes Oct 4, 2025

View reviewed changes

samueltardieu added this pull request to the merge queue Oct 4, 2025

Merged via the queue into rust-lang:master with commit d289009 Oct 4, 2025
11 checks passed

ada4a reviewed Oct 4, 2025

View reviewed changes

samueltardieu mentioned this pull request Mar 10, 2026

match_same_arms false positive with associated constants #16698

Closed

Uh oh!

Uh oh!

Conversation

Jarcho commented Sep 28, 2025

Uh oh!

rustbot commented Sep 28, 2025

Uh oh!

This comment has been minimized.

samueltardieu commented Sep 29, 2025

Uh oh!

samueltardieu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

github-actions Bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu commented Sep 30, 2025

Uh oh!

samueltardieu commented Sep 30, 2025

Uh oh!

Jarcho commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jarcho commented Sep 30, 2025

Uh oh!

Jarcho commented Sep 30, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Oct 4, 2025

Uh oh!

Jarcho commented Oct 4, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zalathar commented Jul 22, 2026

Uh oh!

Jarcho commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

samueltardieu left a comment •

edited

Loading

github-actions Bot commented Sep 30, 2025 •

edited

Loading

Jarcho commented Sep 30, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

Jarcho commented Jul 22, 2026 •

edited

Loading