Try to evaluate in try unify and postpone resolution of constants that contain inference variables by b-naber · Pull Request #95179 · rust-lang/rust

b-naber · 2022-03-21T17:53:34Z

We want code like that in ui/const-generics/generic_const_exprs/eval-try-unify.rs to compile. To do that we need to try to evaluate constants in try_unify_abstract_consts, this requires us to be more careful about what constants we try to resolve, specifically we cannot try to resolve constants that still contain inference variables.

r? @lcnr

rust-highfive · 2022-03-21T17:53:36Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

lcnr

a few general nits, apart from that r=me

b-naber · 2022-03-22T11:06:17Z

Well ok this seems to behave badly with incremental. Only tested ui locally.

lcnr · 2022-03-22T11:27:16Z


+/// Tries to unify two abstract constants using structural equality.
+#[instrument(skip(tcx), level = "debug")]
+pub(super) fn try_unify<'tcx>(


why do we need this to be pub(super) here? additionally, why the try_unify_inner split, can't we use ContextUnifyCtxt::new(tcx, param_env).try_unify(a, b) everywhere instead?

I thought this was the better abstraction, since always creating a ConstUnifyCtxt prior to that call didn't seem ideal at first. But we're calling try_unify only twice (besides the recursive calls) anyway and only in this file, so I got rid of the inner method.

lcnr

yeah, might have to guard against the infer vars in the param env as well.

Instead of canonicalization you should probably erase_regions which 1) improves caching 2) gets rid of region infer variables

lcnr · 2022-03-22T11:31:36Z

also generally: please mark review comments as resolved after they are no longer relevant

Github sadly removes the context from review comments after a force push (I think), so I can't easily tell whether specific comments are still relevant. While you're still working on it, its also preferable to only push new commits if possible.

lcnr

some last nits 😅 after that r=me

lcnr · 2022-03-24T11:31:08Z

@bors r+

bors · 2022-03-24T11:31:09Z

📌 Commit 11a70db has been approved by lcnr

bors · 2022-03-24T15:22:06Z

⌛ Testing commit 11a70db with merge d3d54c547b90b9e2fbd58a0f4c803d94749d9e38...

rust-log-analyzer · 2022-03-24T18:10:00Z

A job failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)

Try to evaluate in try unify and postpone resolution of constants that contain inference variables We want code like that in [`ui/const-generics/generic_const_exprs/eval-try-unify.rs`](https://github.com/rust-lang/rust/compare/master...b-naber:eval-in-try-unify?expand=1#diff-8027038201cf07a6c96abf3cbf0b0f4fdd8a64ce6292435f01c8ed995b87fe9b) to compile. To do that we need to try to evaluate constants in `try_unify_abstract_consts`, this requires us to be more careful about what constants we try to resolve, specifically we cannot try to resolve constants that still contain inference variables. r? `@lcnr`

Try to evaluate in try unify and postpone resolution of constants that contain inference variables We want code like that in [`ui/const-generics/generic_const_exprs/eval-try-unify.rs`](https://github.com/rust-lang/rust/compare/master...b-naber:eval-in-try-unify?expand=1#diff-8027038201cf07a6c96abf3cbf0b0f4fdd8a64ce6292435f01c8ed995b87fe9b) to compile. To do that we need to try to evaluate constants in `try_unify_abstract_consts`, this requires us to be more careful about what constants we try to resolve, specifically we cannot try to resolve constants that still contain inference variables. r? ``@lcnr``

Try to evaluate in try unify and postpone resolution of constants that contain inference variables We want code like that in [`ui/const-generics/generic_const_exprs/eval-try-unify.rs`](https://github.com/rust-lang/rust/compare/master...b-naber:eval-in-try-unify?expand=1#diff-8027038201cf07a6c96abf3cbf0b0f4fdd8a64ce6292435f01c8ed995b87fe9b) to compile. To do that we need to try to evaluate constants in `try_unify_abstract_consts`, this requires us to be more careful about what constants we try to resolve, specifically we cannot try to resolve constants that still contain inference variables. r? ```@lcnr```

Dylan-DPC · 2022-03-24T23:35:17Z

@bors r-

Dylan-DPC · 2022-03-24T23:35:55Z

uhm seems spurious

@bors r=lcnr

bors · 2022-03-24T23:35:56Z

📌 Commit 11a70db has been approved by lcnr

Rollup of 5 pull requests Successful merges: - rust-lang#94391 (Fix ice when error reporting recursion errors) - rust-lang#94655 (Clarify which kinds of MIR are allowed during which phases.) - rust-lang#95179 (Try to evaluate in try unify and postpone resolution of constants that contain inference variables) - rust-lang#95270 (debuginfo: Fix debuginfo for Box<T> where T is unsized.) - rust-lang#95276 (add diagnostic items for clippy's `trim_split_whitespace`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

b-naber added 4 commits March 21, 2022 18:47

try to evaluate in try_unify
47f78a2

stall on on constants that contain infer vars in const_eval_resolve
ac0458a

dont try to unify unevaluated constants that contain infer vars
3b9de6b

add/update tests
6cf3409

rust-highfive assigned lcnr Mar 21, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2022

lcnr reviewed Mar 21, 2022
View reviewed changes

Comment threadcompiler/rustc_infer/src/infer/mod.rs Outdated

lcnr reviewed Mar 21, 2022
View reviewed changes

Comment threadcompiler/rustc_infer/src/infer/mod.rs Outdated

lcnr reviewed Mar 21, 2022
View reviewed changes

This comment has been minimized.
Sign in to view

b-naber force-pushed the eval-in-try-unify branch 2 times, most recently from eb3a01f to 7d708fbCompare March 22, 2022 09:58

This comment has been minimized.
Sign in to view

fix previous failures and address review
8ff1edb

b-naber force-pushed the eval-in-try-unify branch from 7d708fb to 8ff1edbCompare March 22, 2022 10:36

This comment has been minimized.
Sign in to view

lcnr reviewed Mar 22, 2022
View reviewed changes

Comment threadcompiler/rustc_infer/src/infer/mod.rs Outdated

lcnr reviewed Mar 22, 2022
View reviewed changes

dont canonicalize in try_unify_abstract_consts and erase regions instead
fe69a5c

lcnr reviewed Mar 22, 2022
View reviewed changes

Comment threadcompiler/rustc_infer/src/infer/mod.rs Outdated
Comment threadcompiler/rustc_middle/src/mir/interpret/queries.rs Outdated
Comment threadcompiler/rustc_trait_selection/src/traits/const_evaluatable.rs Outdated

erase region in ParamEnvAnd and make ConstUnifyCtxt private
11a70db

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 24, 2022

Dylan-DPC mentioned this pull request Mar 24, 2022
Rollup of 5 pull requests #95279
Closed

Dylan-DPC mentioned this pull request Mar 24, 2022
Rollup of 6 pull requests #95287
Closed

Dylan-DPC mentioned this pull request Mar 24, 2022
Rollup of 5 pull requests #95288
Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 24, 2022

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 Mar 24, 2022

Dylan-DPC mentioned this pull request Mar 25, 2022
Rollup of 5 pull requests #95291
Merged

bors merged commit 1fcb8fc into rust-lang:masterMar 25, 2022

rustbot added this to the 1.61.0 milestone Mar 25, 2022

Uh oh!

Uh oh!

Conversation

b-naber commented Mar 21, 2022

Uh oh!

rust-highfive commented Mar 21, 2022

Uh oh!

Uh oh!

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

b-naber commented Mar 22, 2022

Uh oh!

Uh oh!

lcnrMar 22, 2022

Choose a reason for hiding this comment

Uh oh!

b-naberMar 22, 2022

Choose a reason for hiding this comment

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

lcnr commented Mar 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lcnr commented Mar 24, 2022

Uh oh!

bors commented Mar 24, 2022

Uh oh!

bors commented Mar 24, 2022

Uh oh!

rust-log-analyzer commented Mar 24, 2022

Uh oh!

Dylan-DPC commented Mar 24, 2022

Uh oh!

Dylan-DPC commented Mar 24, 2022

Uh oh!

bors commented Mar 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lcnr commented Mar 22, 2022•
edited
Loading