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

merged 3 commits into from
Sep 29, 2023

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 13, 2023

Otherwise clippy tries to use the query in ways that incremental caching will inevitably cause problems with.

Hopefully gets rid of #83085 for good

@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@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 Sep 13, 2023
@y21 y21 mentioned this pull request Sep 13, 2023
Closed
@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 14, 2023
pnkfelix
pnkfelix reviewed Sep 14, 2023
View reviewed changes
src/tools/clippy/clippy_utils/src/consts.rs
&& 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.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2023
@oli-obk oli-obk added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 21, 2023
@oli-obk oli-obk mentioned this pull request Sep 21, 2023
Closed
3 tasks
@petrochenkov
Copy link
Contributor

r=me with a FIXME on query try_destructure_mir_constant_for_diagnostics telling that it is only used for a dynamic dependency, and should not be used as a query.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2023
@apiraino
Copy link
Contributor

Beta backport accepterd (pending merge) as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 21, 2023
@bors
Copy link
Contributor

bors commented Sep 21, 2023

☔ The latest upstream changes (presumably #116027) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk mentioned this pull request Sep 22, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2023
@bors
Auto merge of rust-lang#116052 - oli-obk:ceci_nest_pas_une_query, r=W…
bf98263 
…affleLapkin

Add a way to decouple the implementation and the declaration of a TyCtxt method.

properly addresses rust-lang#115819

accepted MCP: rust-lang/compiler-team#395
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 24, 2023
@bors
Auto merge of #116052 - oli-obk:ceci_nest_pas_une_query, r=WaffleLapkin
212dae1 
Add a way to decouple the implementation and the declaration of a TyCtxt method.

properly addresses rust-lang/rust#115819

accepted MCP: rust-lang/compiler-team#395
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Sep 25, 2023
@bors
Auto merge of #116052 - oli-obk:ceci_nest_pas_une_query, r=WaffleLapkin
3da69c7 
Add a way to decouple the implementation and the declaration of a TyCtxt method.

properly addresses rust-lang/rust#115819

accepted MCP: rust-lang/compiler-team#395
@oli-obk oli-obk changed the base branch from master to beta September 28, 2023 14:11
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 28, 2023

aaa whoops, sorry y'all

@oli-obk
Expose try_destructure_mir_constant_for_diagnostics directly to clippy
b76caa1 
Otherwise clippy tries to use the query in ways that incremental caching will inevitably cause problems with.
@oli-obk oli-obk force-pushed the try_destructure_mir_constant_for_diagnostics_strikes_again branch from bbb38ea to b76caa1 Compare September 28, 2023 14:13
@rustbot
Copy link
Collaborator

rustbot commented Sep 28, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 28, 2023

As discussed in the compiler team triage, we're landing this directly on beta, as it is a much simpler fix than backporting the more complex #116052 that fixes the issue on master.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 28, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 28, 2023

📌 Commit b76caa1 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 28, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Sep 29, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 29, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2023

📌 Commit f425b57 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 29, 2023
@bors
Copy link
Contributor

bors commented Sep 29, 2023

⌛ Testing commit f425b57 with merge 3ad792d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 29, 2023
@bors
Auto merge of rust-lang#115819 - oli-obk:try_destructure_mir_constant…
3ad792d 
…_for_diagnostics_strikes_again, r=oli-obk

Expose try_destructure_mir_constant_for_diagnostics directly to clippy

Otherwise clippy tries to use the query in ways that incremental caching will inevitably cause problems with.

Hopefully gets rid of rust-lang#83085 for good
@bors
Copy link
Contributor

bors commented Sep 29, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 29, 2023
@cuviper
Copy link
Member

cuviper commented Sep 29, 2023 •
edited
Loading

Sheesh -- probably also need 9c84757 and 7900923 as we did for stable in #115787 and master #115902.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 29, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2023

📌 Commit af9328c has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2023
@bors
Copy link
Contributor

bors commented Sep 29, 2023

⌛ Testing commit af9328c with merge 975e63f...

@bors
Copy link
Contributor

bors commented Sep 29, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 975e63f to beta...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 29, 2023
@bors bors merged commit 975e63f into rust-lang:beta Sep 29, 2023
12 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Sep 29, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 29, 2023
@github-actions github-actions bot mentioned this pull request Oct 10, 2023
Closed
@github-actions github-actions bot mentioned this pull request Oct 26, 2023
Merged
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 26, 2023
@bors-ferrocene @Mark-Simulacrum
@bors @cuviper @jackh726 @weihanglo @tmiasko @dtolnay @scottmcm @cjgillot @notriddle @lcnr @klensy @fmease @oli-obk @bvanjoi @ouz-a @g0djan @compiler-errors
Merge #67
78b3b5b 
67: Automated pull from upstream `stable` r=pietroalbini a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#116364
* rust-lang/rust#116279
* rust-lang/rust#115819
* rust-lang/rust#116044
* rust-lang/rust#115901
* rust-lang/rust#115722
* rust-lang/rust#115442
* rust-lang/rust#115282
* rust-lang/rust#115195
* rust-lang/rust#115056



Co-authored-by: Mark Rousskov <mark.simulacrum@gmail.com>
Co-authored-by: bors <bors@rust-lang.org>
Co-authored-by: Josh Stone <jistone@redhat.com>
Co-authored-by: Jack Huey <31162821+jackh726@users.noreply.github.com>
Co-authored-by: Weihang Lo <me@weihanglo.tw>
Co-authored-by: Tomasz Miąsko <tomasz.miasko@gmail.com>
Co-authored-by: David Tolnay <dtolnay@gmail.com>
Co-authored-by: Scott McMurray <scottmcm@users.noreply.github.com>
Co-authored-by: Camille GILLOT <gillot.camille@gmail.com>
Co-authored-by: Michael Howell <michael@notriddle.com>
Co-authored-by: lcnr <rust@lcnr.de>
Co-authored-by: klensy <klensy@users.noreply.github.com>
Co-authored-by: León Orell Valerian Liehr <me@fmease.dev>
Co-authored-by: Oli Scherer <git-spam-no-reply9815368754983@oli-obk.de>
Co-authored-by: bohan <bohan-zhang@foxmail.com>
Co-authored-by: ouz-a <ouz.agz@gmail.com>
Co-authored-by: Georgii Rylov <g0dj4n@gmail.com>
Co-authored-by: Michael Goulet <michael@errs.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pnkfelix pnkfelix pnkfelix left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.
9 participants
@oli-obk @rustbot @petrochenkov @apiraino @bors @cuviper @rust-log-analyzer @pnkfelix @compiler-errors