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

Deal with unnormalized projections when structurally resolving types with new solver #110204

Merged
merged 2 commits into from
May 23, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Apr 11, 2023

  1. Normalize types in structurally_resolved_type when the new solver is enabled
  2. Normalize built-in autoderef targets in Autoderef when the new solver is enabled
  3. Normalize-erasing-regions in resolve_type in writeback

This is motivated by the UI test provided, which currently fails with:

error[E0609]: no field `x` on type `<usize as SliceIndex<[Foo]>>::Output`
 --> <source>:9:11
  |
9 |     xs[0].x = 1;
  |           ^

I'm pretty happy with the approach in (1.) and (2.) and think we'll inevitably need something like this in the long-term, but (3.) seems like a hack to me. It's a lot of work to add tons of new calls to every user of these typeck results though (mir build, late lints, etc). Happy to discuss further.

r? @lcnr

@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 11, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the new-solver-hir-typeck-hacks branch from 1d91b94 to 2e3766d Compare April 23, 2023 20:11
@apiraino
Copy link
Contributor

apiraino commented May 3, 2023

I'll re-roll the review dice 🎲 (hope it's fine since the autoassigned reviewer didnt yet comment)

r? compiler

@rustbot rustbot assigned TaKO8Ki and unassigned lcnr May 3, 2023
@compiler-errors
Copy link
Member Author

@apiraino, this basically needs to be reviewed by lcnr

r? lcnr

@rustbot rustbot assigned lcnr and unassigned TaKO8Ki May 3, 2023
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

is the structurally_normalize vs structurally_resolve split meaningful, i.e. is there a case where we would want to have different behavior?

I could imagine having infcx.structurally_resolve(&mut fulfill_cx, span, ty) which returns Err if the ty cannot be resolved to a rigid ty.

compiler/rustc_hir_analysis/src/autoderef.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/autoderef.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/autoderef.rs Outdated Show resolved Hide resolved
@lcnr lcnr 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-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2023
@bors
Copy link
Contributor

bors commented May 4, 2023

☔ The latest upstream changes (presumably #110806) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member Author

compiler-errors commented May 8, 2023

@lcnr: Not sure if I understand the changes you're asking for.

The distinction between structually_normalize and structurally_resolve is made because we don't need to do any normalization from the calls that originate from FnCtxt. If we just had all calls to structually_resolve go through InferCtxt::structurally_normalize, then we'd end up trying to normalize a bunch of rigid projections and doing unnecessary work with -Ztrait-solver=classic. However, the calls that originate from Autoderef do need to go through a normalization step.

The problem here is we are trying to unify two somewhat different codepaths -- one coming from FnCtxt that's expected to have called infer::At::normalize and one coming from Autoderef that has not. The former has already also had structurally_resolve called on it (well, FnCtxt::structurally_resolved_type) whereas the latter has not, but we never expect to have an infer var coming from it that needs to be structurally resolved first.

@compiler-errors compiler-errors 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 May 8, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the new-solver-hir-typeck-hacks branch from 2e3766d to 9c65181 Compare May 8, 2023 04:14
@bors
Copy link
Contributor

bors commented May 15, 2023

☔ The latest upstream changes (presumably #111570) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

r=me after dealing with comments. opened rust-lang/trait-system-refactor-initiative#14 and rust-lang/trait-system-refactor-initiative#15 so i don't forget about this

Comment on lines +175 to +177
// This shouldn't happen, except for evaluate/fulfill mismatches,
// but that's not a reason for an ICE (`predicate_may_hold` is conservative
// by design).
Copy link
Contributor

@lcnr lcnr May 19, 2023

Choose a reason for hiding this comment

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

when do we actually hit that path? do we have an existing test for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

No :( doesn't seem like anything hits it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally would prefer to ICE here to get a test 😁 but ah well, r=me with or without bug! :p

@rust-cloud-vms rust-cloud-vms bot force-pushed the new-solver-hir-typeck-hacks branch from 9c65181 to 4cfafb2 Compare May 22, 2023 21:25
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented May 23, 2023

📌 Commit 4cfafb2 has been approved by lcnr

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 May 23, 2023
@bors
Copy link
Contributor

bors commented May 23, 2023

⌛ Testing commit 4cfafb2 with merge 4400d8f...

@bors
Copy link
Contributor

bors commented May 23, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 4400d8f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 23, 2023
@bors bors merged commit 4400d8f into rust-lang:master May 23, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 23, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (4400d8f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.7% [1.7%, 1.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-2.8%, -2.4%] 2
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.072s -> 644.363s (0.05%)

@compiler-errors compiler-errors deleted the new-solver-hir-typeck-hacks branch August 11, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants