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.

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

Expose try_destructure_mir_constant_for_diagnostics directly to clippy #115819

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
@oli-obk oli-obk added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 14, 2023
&& 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

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2023
…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
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
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

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.

@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
…_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

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.

@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
11 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
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 26, 2023
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 <[email protected]>
Co-authored-by: bors <[email protected]>
Co-authored-by: Josh Stone <[email protected]>
Co-authored-by: Jack Huey <[email protected]>
Co-authored-by: Weihang Lo <[email protected]>
Co-authored-by: Tomasz Miąsko <[email protected]>
Co-authored-by: David Tolnay <[email protected]>
Co-authored-by: Scott McMurray <[email protected]>
Co-authored-by: Camille GILLOT <[email protected]>
Co-authored-by: Michael Howell <[email protected]>
Co-authored-by: lcnr <[email protected]>
Co-authored-by: klensy <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Oli Scherer <[email protected]>
Co-authored-by: bohan <[email protected]>
Co-authored-by: ouz-a <[email protected]>
Co-authored-by: Georgii Rylov <[email protected]>
Co-authored-by: Michael Goulet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants