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.

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

Expose try_destructure_mir_constant_for_diagnostics directly to clippy #115819

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Expose try_destructure_mir_constant_for_diagnostics directly to clippy #115819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b76caa1
Expose try_destructure_mir_constant_for_diagnostics directly to clippy
oli-obk Sep 13, 2023
f425b57
Fix jq in CI
compiler-errors Sep 25, 2023
af9328c
Move to older, mirrored redox install and to newer ubuntu
oli-obk Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/const_eval/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub(crate) fn eval_to_valtree<'tcx>(
}

#[instrument(skip(tcx), level = "debug")]
pub(crate) fn try_destructure_mir_constant_for_diagnostics<'tcx>(
pub fn try_destructure_mir_constant_for_diagnostics<'tcx>(
tcx: TyCtxt<'tcx>,
val: ConstValue<'tcx>,
ty: Ty<'tcx>,
Expand Down
2 changes: 1 addition & 1 deletion src/ci/docker/host-x86_64/arm-android/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM ubuntu:22.10
FROM ubuntu:23.04

ARG DEBIAN_FRONTEND=noninteractive
COPY scripts/android-base-apt-get.sh /scripts/
Expand Down
2 changes: 1 addition & 1 deletion src/ci/docker/host-x86_64/dist-android/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM ubuntu:22.10
FROM ubuntu:23.04

COPY scripts/android-base-apt-get.sh /scripts/
RUN sh /scripts/android-base-apt-get.sh
Expand Down
2 changes: 1 addition & 1 deletion src/ci/docker/host-x86_64/dist-various-1/install-x86_64-redox.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@

set -ex

curl https://static.redox-os.org/toolchain/x86_64-unknown-redox/relibc-install.tar.gz | \
curl https://ci-mirrors.rust-lang.org/rustc/2022-11-27-relibc-install.tar.gz | \
tar --extract --gzip --directory /usr/local
2 changes: 1 addition & 1 deletion src/ci/scripts/setup-environment.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ source "$(cd "$(dirname "$0")" && pwd)/../shared.sh"

# Load extra environment variables
vars="${EXTRA_VARIABLES-}"
echo "${vars}" | jq '' >/dev/null # Validate JSON and exit on errors
echo "${vars}" | jq '.' >/dev/null # Validate JSON and exit on errors
for key in $(echo "${vars}" | jq "keys[]" -r); do
# On Windows, for whatever reason, $key contains the BOM character in it,
# and that messes up `jq ".${key}"`. This line strips the BOM from the key.
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_utils/src/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ fn field_of_struct<'tcx>(
field: &Ident,
) -> Option<mir::ConstantKind<'tcx>> {
if let mir::ConstantKind::Val(result, ty) = result
&& let Some(dc) = lcx.tcx.try_destructure_mir_constant_for_diagnostics((result, ty))
&& let Some(dc) = rustc_const_eval::const_eval::try_destructure_mir_constant_for_diagnostics(lcx.tcx, result, ty)
Copy link
Member

Choose a reason for hiding this comment

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

Hey @oli-obk , what intuition should a developer use to determine which of these two methods they are supposed to call in a given context? How can I know if the query-based lcx.tcx.$METHOD is the appropriate thing to call?

Is it a matter of "If you see an ICE with 'forcing query with already existing DepNode', then you are doing it wrong" ? Unfortunately the error output I see on #83085 doesn't clearly point one to this code, so I don't know how someone would track it down...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think anyone should be calling this query. It was wrong of me to implement the traits that allowed ConstValue (an argument to the query), to be used in query arguments, as it can produce equal stable hashes for things that are not identical via PartialEq (in fact, this is very easy to cause, just by allocating two different AllocIds for the same Allocation, as all AllocIds are stable-hashed by the allocation they refer to).

But we have no way to avoid it from pretty printing at present, because that's implemented in rustc_middle while the query provider is in rustc_const_eval (which depends on rustc_middle).

Copy link
Contributor

@petrochenkov petrochenkov Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But we have no way to avoid it from pretty printing at present, because that's implemented in rustc_middle while the query provider is in rustc_const_eval (which depends on rustc_middle).

Other cases like this are typically addressed by non-query dyn Traits or function pointers, rather than queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really have a good place/system to add such function pointers to TyCtxt yet. I could look into making it part of the whole provider scheme.

If that's desirable, I will do that, but this PR should be landed quickly so we can have a small patch to beta, instead of a larger change (even if benign)

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't really have a good place/system to add such function pointers to TyCtxt yet.

Well, lint_store and formerly cstore lived right in GlobalCtxt.
This PR seems fine for backport, though.

&& let Some(dc_variant) = dc.variant
&& let Some(variant) = adt_def.variants().get(dc_variant)
&& let Some(field_idx) = variant.fields.iter().position(|el| el.name == field.name)
Expand Down
Loading