Fix type resolution of associated const equality bounds (take 2) #119385

fmease · 2023-12-28T17:54:46Z

Instead of trying to re-resolve the type of assoc const bindings inside the type_of query impl in an incomplete manner, transfer the already (correctly) resolved type from add_predicates_for_ast_type_binding to type_of/anon_type_of through query feeding.

Together with #118668 (merged) and #121258, this supersedes #118360.
Fixes #118040.

r? @ghost

fmease · 2024-01-10T14:10:55Z

compiler/rustc_hir_analysis/src/astconv/mod.rs

+                                let term: ty::Term<'_> = match term {
+                                    hir::Term::Ty(ty) => self.ast_ty_to_ty(ty).into(),
+                                    hir::Term::Const(ct) => {
+                                        ty::Const::from_anon_const(tcx, ct.def_id).into()


This might leak {type error} since we haven't actually resolved & feed the type to type_of(AnonConst) if we reach this case. The pretty-printer doesn't seem to care though (not sure if it can actually crash on certain inputs involving ty::TyKind::Error though).

Alternatively, I could pretty-print the HIR of the const but I'd need to create a public helper, const_to_string, since rustc_hir_pretty doesn't seem to expose anything like that yet?

compiler/rustc_hir_analysis/src/collect/type_of.rs

fmease · 2024-01-10T15:59:21Z

Best reviewed commit by commit.
r? compiler-errors (no rush though)

compiler/rustc_hir_analysis/src/collect/type_of.rs

oli-obk · 2024-02-21T10:59:52Z

We can directly feed the AnonConst's type instead of adding another indirection.

Ideally we'd only create the AnonConst's DefId right here, but since that can't be done for the other uses of AnonConsts yet, we still have to wait a bit before cleaning this up.

compiler/rustc_hir_analysis/src/astconv/bounds.rs

fmease · 2024-02-23T05:51:02Z

We can directly feed the AnonConst's type instead of adding another indirection.

Oh, that makes perfect sense, thanks! Apart from eliminating indirection, does your change affect incremental query evaluation? Just out of curiosity I'd like to ask if the previous approach was unsound?

As for next steps, is this change ready to be merged? Who should review it lol?

oli-obk · 2024-02-23T07:32:20Z

Just out of curiosity I'd to ask if the previous approach was unsound?

It fed a hir id query, which we haven't considered yet at all. We could have marked whichever query did that as depending on the forever red node, meaning it would get rerun always. That would have been sound, but horrible for perf.

Then again, it's not really worse than what I did with the other query, just fewer moving parts. Also DefIds are a bit more stable than hir ids, due to their tree style relative nature.

I did not think about hir id query feeding soundness too much, so it may have been fine

oli-obk · 2024-02-23T07:33:31Z

Just out of curiosity I'd to ask if the previous approach was unsound?

It fed a hir id query, which we haven't considered yet at all. We could have marked whichever query did that as depending on the forever red node, meaning it would get rerun always. That would have been sound, but horrible for perf.

Then again, it's not really worse than what I did with the other query, just fewer moving parts. Also DefIds are a bit more stable than hir ids, due to their tree style relative nature.

I did not think about hir id query feeding soundness too much, so it may have been fine

As for next steps, is this change ready to be merged? Who should review it lol?

We can cross review each other's changes 😅 or get cjgillot to chime in

fmease · 2024-02-27T08:49:58Z

Hey, @cjgillot. If you have time and energy, could you skim this PR and double-check if the query feeding performed here roughly makes sense from a soundness perspective wrt. incr comp? Thanks a lot in advance!

We can cross review each other's changes

Otherwise the changes look good to me!

cjgillot · 2024-03-10T12:51:07Z

LGTM. Sorry for the delay.
@bors r=oli-obk,cjgillot

bors · 2024-03-10T12:51:09Z

📌 Commit 858d336 has been approved by oli-obk,cjgillot

It is now in the queue for this repository.

…li-obk,cjgillot Fix type resolution of associated const equality bounds (take 2) Instead of trying to re-resolve the type of assoc const bindings inside the `type_of` query impl in an incomplete manner, transfer the already (correctly) resolved type from `add_predicates_for_ast_type_binding` to `type_of`/`anon_type_of` through query feeding. --- Together with rust-lang#118668 (merged) and rust-lang#121258, this supersedes rust-lang#118360. Fixes rust-lang#118040. r? `@ghost`

…kingjubilee Rollup of 15 pull requests Successful merges: - rust-lang#116791 (Allow codegen backends to opt-out of parallel codegen) - rust-lang#116793 (Allow targets to override default codegen backend) - rust-lang#117458 (LLVM Bitcode Linker: A self contained linker for nvptx and other targets) - rust-lang#119385 (Fix type resolution of associated const equality bounds (take 2)) - rust-lang#121438 (std support for wasm32 panic=unwind) - rust-lang#121893 (Add tests (and a bit of cleanup) for interior mut handling in promotion and const-checking) - rust-lang#122080 (Clarity improvements to `DropTree`) - rust-lang#122152 (Improve diagnostics for parenthesized type arguments) - rust-lang#122166 (Remove the unused `field_remapping` field from `TypeLowering`) - rust-lang#122249 (interpret: do not call machine read hooks during validation) - rust-lang#122299 (Store backtrace for `must_produce_diag`) - rust-lang#122318 (Revision-related tweaks for next-solver tests) - rust-lang#122320 (Use ptradd for vtable indexing) - rust-lang#122328 (unix_sigpipe: Replace `inherit` with `sig_dfl` in syntax tests) - rust-lang#122330 (bootstrap readme: fix, improve, update) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#119385 - fmease:assoc-const-eq-fixes-2, r=oli-obk,cjgillot Fix type resolution of associated const equality bounds (take 2) Instead of trying to re-resolve the type of assoc const bindings inside the `type_of` query impl in an incomplete manner, transfer the already (correctly) resolved type from `add_predicates_for_ast_type_binding` to `type_of`/`anon_type_of` through query feeding. --- Together with rust-lang#118668 (merged) and rust-lang#121258, this supersedes rust-lang#118360. Fixes rust-lang#118040. r? ``@ghost``

…y-generic-tys, r=compiler-errors Reject overly generic assoc const binding types Split off from rust-lang#119385 to make rust-lang#119385 easier to review. --- In the *instantiated* type of assoc const bindings 1. reject **early-bound generic params** * Provide a rich error message instead of ICE'ing ([rust-lang#108271](rust-lang#108271)). * This is a temporary and semi-artificial restriction until the arrival of *generic const generics*. * It's quite possible that rustc could already perfectly support this subset of generic const generics if we just removed some checks (some `.no_bound_vars().expect(…)`) but even if that was the case, I'd rather gate it behind a new feature flag. Reporting an error instead of ICE'ing is a good first step towards an eventual feature gate error. 2. reject **escaping late-bound generic params** * They lead to ICEs before & I'm pretty sure that they remain incorrect even in a world with *generic const generics* --- Together with rust-lang#118668 & rust-lang#119385, this supersedes rust-lang#118360. Fixes rust-lang#108271.

Rollup merge of rust-lang#121258 - fmease:assoc-const-eq-reject-overly-generic-tys, r=compiler-errors Reject overly generic assoc const binding types Split off from rust-lang#119385 to make rust-lang#119385 easier to review. --- In the *instantiated* type of assoc const bindings 1. reject **early-bound generic params** * Provide a rich error message instead of ICE'ing ([rust-lang#108271](rust-lang#108271)). * This is a temporary and semi-artificial restriction until the arrival of *generic const generics*. * It's quite possible that rustc could already perfectly support this subset of generic const generics if we just removed some checks (some `.no_bound_vars().expect(…)`) but even if that was the case, I'd rather gate it behind a new feature flag. Reporting an error instead of ICE'ing is a good first step towards an eventual feature gate error. 2. reject **escaping late-bound generic params** * They lead to ICEs before & I'm pretty sure that they remain incorrect even in a world with *generic const generics* --- Together with rust-lang#118668 & rust-lang#119385, this supersedes rust-lang#118360. Fixes rust-lang#108271.

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 Dec 28, 2023

This comment has been minimized.

Sign in to view

fmease force-pushed the assoc-const-eq-fixes-2 branch from 299d62f to 8ce12cd Compare December 31, 2023 04:36

fmease force-pushed the assoc-const-eq-fixes-2 branch from 8ce12cd to cca22f2 Compare January 8, 2024 12:22

fmease mentioned this pull request Jan 9, 2024

ICE with assoc const equality & closure: ty::Const::eval called on a non-valtree-compatible type #119783

Open

fmease force-pushed the assoc-const-eq-fixes-2 branch from cca22f2 to 8bf6e86 Compare January 10, 2024 02:10

This comment has been minimized.

Sign in to view

fmease force-pushed the assoc-const-eq-fixes-2 branch from 8bf6e86 to 809f68c Compare January 10, 2024 14:04

fmease commented Jan 10, 2024

View reviewed changes

fmease force-pushed the assoc-const-eq-fixes-2 branch 3 times, most recently from 7938112 to ecf2aba Compare January 10, 2024 15:35

fmease commented Jan 10, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/collect/type_of.rs Outdated Show resolved Hide resolved

fmease changed the title ~~[WIP] Fix type resolution of associated const equality bounds (take 2)~~ Fix type resolution of associated const equality bounds (take 2) Jan 10, 2024

fmease marked this pull request as ready for review January 10, 2024 15:58

rust-lang deleted a comment from rustbot Jan 10, 2024

rustbot assigned compiler-errors Jan 10, 2024

fmease 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 Jan 10, 2024

This comment was marked as outdated.

Sign in to view

fmease force-pushed the assoc-const-eq-fixes-2 branch 3 times, most recently from cfc3f0f to ca6ac84 Compare January 14, 2024 11:33

fmease commented Jan 16, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/collect/type_of.rs Outdated Show resolved Hide resolved

fmease commented Feb 23, 2024

View reviewed changes

compiler/rustc_hir_analysis/src/astconv/bounds.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

Slightly simplify feeding of assoc const eq bounds

858d336

fmease force-pushed the assoc-const-eq-fixes-2 branch from 2b166bd to 858d336 Compare February 27, 2024 08:47

fmease assigned cjgillot Feb 27, 2024

compiler-errors removed their assignment Mar 4, 2024

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 Mar 10, 2024

matthiaskrgr mentioned this pull request Mar 11, 2024

Rollup of 11 pull requests #122337

Closed

workingjubilee mentioned this pull request Mar 11, 2024

Rollup of 15 pull requests #122338

Merged

bors merged commit a450339 into rust-lang:master Mar 11, 2024
11 checks passed

rustbot added this to the 1.78.0 milestone Mar 11, 2024

fmease deleted the assoc-const-eq-fixes-2 branch March 11, 2024 19:41

fmease mentioned this pull request Mar 13, 2024

Tracking Issue for associated const equality #92827

Open

8 tasks

fmease mentioned this pull request Jul 26, 2024

Tracking issue for generic const items #113521

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type resolution of associated const equality bounds (take 2) #119385

Fix type resolution of associated const equality bounds (take 2) #119385

fmease commented Dec 28, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

fmease Jan 10, 2024 •

edited

Loading

fmease commented Jan 10, 2024

This comment was marked as outdated.

oli-obk commented Feb 21, 2024

fmease commented Feb 23, 2024 •

edited

Loading

This comment has been minimized.

This comment was marked as resolved.

oli-obk commented Feb 23, 2024

oli-obk commented Feb 23, 2024

fmease commented Feb 27, 2024

cjgillot commented Mar 10, 2024

bors commented Mar 10, 2024

Fix type resolution of associated const equality bounds (take 2) #119385

Fix type resolution of associated const equality bounds (take 2) #119385

Conversation

fmease commented Dec 28, 2023 • edited Loading

This comment has been minimized.

This comment has been minimized.

fmease Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

fmease commented Jan 10, 2024

This comment was marked as outdated.

oli-obk commented Feb 21, 2024

fmease commented Feb 23, 2024 • edited Loading

This comment has been minimized.

This comment was marked as resolved.

oli-obk commented Feb 23, 2024

oli-obk commented Feb 23, 2024

fmease commented Feb 27, 2024

cjgillot commented Mar 10, 2024

bors commented Mar 10, 2024

fmease commented Dec 28, 2023 •

edited

Loading

fmease Jan 10, 2024 •

edited

Loading

fmease commented Feb 23, 2024 •

edited

Loading