Skip to content

remove FIXME block from has_significant_drop, can handle inference #145181

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tnuha
Copy link
Contributor

@tnuha tnuha commented Aug 9, 2025

The FIXME block in Ty::has_significant_drop is outdated as related queries can now handle type inference.

// FIXME(#86868): We should be canonicalizing, or else moving this to a method of inference
// context, or *something* like that, but for now just avoid passing inference
// variables to queries that can't cope with them. Instead, conservatively
// return "true" (may change drop order).
if query_ty.has_infer() {
return true;
}
// This doesn't depend on regions, so try to minimize distinct
// query keys used.
let erased = tcx.normalize_erasing_regions(typing_env, query_ty);
tcx.has_significant_drop_raw(typing_env.as_query_input(erased))

Closes #86868 (other places mentioned in the issue have been resolved, or moved to other issues)

r? types

@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 Aug 9, 2025
@compiler-errors
Copy link
Member

Let's double check this with crater.

@bors2 try

@rust-bors
Copy link

rust-bors bot commented Aug 10, 2025

⌛ Trying commit 75ec279 with merge 1383df0

To cancel the try build, run the command @bors try cancel.

rust-bors bot added a commit that referenced this pull request Aug 10, 2025
remove FIXME block from `has_significant_drop`, can handle inference
@rust-bors
Copy link

rust-bors bot commented Aug 10, 2025

☀️ Try build successful (CI)
Build commit: 1383df0 (1383df06117fd218068d1f39fd5326256dd4a348, parent: ca77504943887037504c7fc0b9bf06dab3910373)

@compiler-errors
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-145181 created and queued.
🤖 Automatically detected try build 1383df0
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-145181 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@compiler-errors
Copy link
Member

[...] as related queries can now handle type inference.

I think the more likely thing here is that we're now being more responsible and only calling has_significant_drop on types that don't have infer vars.

@tnuha
Copy link
Contributor Author

tnuha commented Aug 10, 2025

I did actually try this- the inference block I removed is certainly being hit in a few cases. I can rerun that experiment to find exactly which if you'd like.

Are you thinking the related queries still don't cope with infer vars? If so, do you know of a way to test this? (locally, not with craterbot ofc)

@tnuha
Copy link
Contributor Author

tnuha commented Aug 10, 2025

@compiler-errors https://github.com/rust-lang/rust/blob/master/tests/incremental/issue-86753.rs is an example for where has_significant_drop is called with an infer var, although admittedly a complex one. Works fine with the block removed.

I do see an argument for favoring canonicalization regardless- I just see this as an avenue for reducing complexity. Whatever seems to be the most appropriate.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-145181 is completed!
📊 12 regressed and 6 fixed (679566 total)
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor queries that don't accept inference variables to use canonicalization
5 participants