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

Don't probe InferConst in fold_const if self.infcx is None, deeply_normalize tys in check_tys_might_be_eq #124526

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ShE3py
Copy link
Contributor

@ShE3py ShE3py commented Apr 29, 2024

Fixes #119381, fixes #114456.

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Apr 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2024

changes to the core type system

cc @compiler-errors, @lcnr

@compiler-errors
Copy link
Member

r? lcnr

@rustbot rustbot assigned lcnr and unassigned TaKO8Ki Apr 29, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Apr 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Apr 29, 2024
@ShE3py
Copy link
Contributor Author

ShE3py commented Apr 29, 2024

@rustbot label -A-testsuite -T-bootstrap
(typo fix)

@rustbot rustbot removed A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 29, 2024
@BoxyUwU
Copy link
Member

BoxyUwU commented May 2, 2024

Im not sure this fix is correct. Whether the paramenv has already been canonicalized or not has nothing to do with whether we can encounter consts with aliases as their type.

I would expect the correct fix to be to normalize both types in check_tys_might_be_eq . We probably have to be careful there with errors during normalization because if we're in a probe and have an non wf alias which doesn't cause an error then we don't want to ICE in this check.

Sooo we probably want that fn to call deeply_nornalize then select_all_or_error, but return Ok(()) in case of error. Though at this point this check is becoming a bit of a pain lol...

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 3, 2024
@ShE3py ShE3py changed the title Don't canonicalize ty::Const if its ParamEnv isn't canonicalized yet Don't probe InferConst in fold_const if self.infcx is None, deeply_normalize tys in check_tys_might_be_eq May 3, 2024
@ShE3py
Copy link
Contributor Author

ShE3py commented May 3, 2024

Ty for the pointers, though non-wf aliases ICEs even without deeply_nornalizeing the tys;

struct Sized<T>(T);
type NonWf = Sized<dyn ToString>;

struct Walk<const W: NonWf> {}
impl Walk<{ Sized("") }> {}

https://play.rust-lang.org/?gist=66eb4c5f0a126c73ba8734ef399b639d

@rustbot label -A-testsuite -T-bootstrap

@rustbot rustbot removed A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 3, 2024
@BoxyUwU
Copy link
Member

BoxyUwU commented May 3, 2024

Why the first commit? You didn't update the function that handles tys and also it shouldn't be required for this as in super_combine_tys we have an infcx available for canonicalization

@ShE3py
Copy link
Contributor Author

ShE3py commented May 3, 2024

It's for #119381, whose backtrack is as follow:

  1. Canonicalizer::fold_const where self.infcx.unwrap() fails
  2. InferCtxt::canonicalize_query::{closure#0} where infcx := None
  3. CanonicalParamEnvCache::get_or_insert whose docstring says that it shouldn't resolve inference variables as to not invalidates its cache (PR cache param env canonicalization #117749)
  4. InferCtxt::canonicalize_query
  5. InferCtxt::super_combine_consts
  6. InferCtxt::super_combine_tys

Though it may be preferable to (re-)canonicalize ParamEnv (bypassing the cache) in super_combine_consts as to have an infcx?

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