Fix type resolution of associated const equality bounds (take 2) by fmease · Pull Request #119385 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation21 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

fmease

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

@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

Dec 28, 2023

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

fmease

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()

Choose a reason for hiding this comment

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

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?

fmease

@fmease 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 fmease marked this pull request as ready for review

January 10, 2024 15:58

@fmease

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

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

@bors

This comment was marked as outdated.

fmease

@oli-obk

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.

fmease

@fmease

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?

@rust-log-analyzer

This comment has been minimized.

@fmease

This comment was marked as resolved.

@oli-obk

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

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

@fmease

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

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

@bors

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

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

Mar 10, 2024

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 11, 2024

@matthiaskrgr

…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

bors added a commit to rust-lang-ci/rust that referenced this pull request

Mar 11, 2024

@bors

…kingjubilee

Rollup of 15 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 11, 2024

@rust-timer

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

@fmease fmease deleted the assoc-const-eq-fixes-2 branch

March 11, 2024 19:41

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Mar 18, 2024

@workingjubilee

…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).
    • 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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 18, 2024

@matthiaskrgr

…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).
    • 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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Mar 18, 2024

@matthiaskrgr

…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).
    • 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.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Mar 18, 2024

@rust-timer

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).
    • 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.

Labels

F-associated_const_equality

`#![feature(associated_const_equality)]`

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-types

Relevant to the types team, which will review and decide on the PR/issue.