-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Const eval changes #15773
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
Const eval changes #15773
Conversation
rustbot has assigned @samueltardieu. Use |
This comment has been minimized.
This comment has been minimized.
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. |
There was a problem hiding this 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.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
clippy_utils/src/consts.rs
Outdated
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 | ||
}); |
There was a problem hiding this comment.
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);
}
}
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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.
This comment has been minimized.
This comment has been minimized.
Lintcheck changes for 47b0f90
This comment will be updated if you push new changes |
That is a lot of lintcheck changes, and from just browsing them it looks like some (many?) of them are unwarranted. |
0db692b
to
51c9fb9
Compare
The |
|
4db4b32
to
245c53f
Compare
Had to reorder this and #15774 since |
The change in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this 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 NaN
s on OSX/aarch64, which makes tests/ui/if_same_then_else
fail at line 76. This doesn't happen on Linux/x86_64.
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.
|
span, | ||
.. | ||
}) = self.tcx.hir_node(body_id.hir_id) | ||
&& is_direct_expn_of(*span, sym::cfg).is_some() |
There was a problem hiding this comment.
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:)
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 likeusize::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 toeval_local
. Most of these were style lints which shouldn't be assuming the value of a constant won't ever change.changelog: none