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.

Sign up for GitHub

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

Jump to bottom

Fix ICEs related to Deref<Target=[T; N]> on newtypes #92640

Merged
merged 3 commits into from
Jan 18, 2022
Merged

Fix ICEs related to Deref<Target=[T; N]> on newtypes #92640

merged 3 commits into from
Jan 18, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jan 7, 2022 •
edited
Loading

  1. Stash a const infer's type into the canonical var during canonicalization, so we can recreate the fresh const infer with that same type.
    For example, given [T; _] we know _ is a usize. If we go from infer => canonical => infer, we shouldn't forget that variable is a usize.
    Fixes Inferring const parameters to wrapper type causes rustc to panic. #92626
    Fixes ICE on Rust 1.51 with min const generics and Deref<Target=[T; N]> #83704

  2. Don't stash the autoderef'd slice type that we get from method lookup, but instead recreate it during method confirmation. We need to do this because the type we receive back after picking the method references a type variable that does not exist after probing is done.
    Fixes Calling slice method on newtype wrapper that implements Deref<Target=[_; 1]> causes ICE #92637

... A better solution for the second issue would be to actually properly implement Deref for [T; N] instead of fixing this autoderef hack to stop leaking inference variables. But I actually looked into this, and there are many complications with const impls.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 7, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2022
@compiler-errors
Copy link
Member Author

r? @eddyb
Just because I saw you in the git-blame history near both of these changes, and you were also reviewer for other commits touching code around here. Feel free to reassign to someone else if you care to do so.

@rust-highfive rust-highfive assigned eddyb and unassigned estebank Jan 7, 2022
@lcnr lcnr self-assigned this Jan 7, 2022
@lcnr
Copy link
Contributor

lcnr commented Jan 7, 2022

r? @lcnr

alright, so 1. looks correct to me, have to take some time to fully grep the second issue but it seems preferable to implement Deref and DerefMut if that doesn't break anything

@compiler-errors
Copy link
Member Author

@lcnr, is it worthwhile to land the solution #2 as-is while I work on properly impl'ing Deref for [T; N]?

@lcnr
Copy link
Contributor

lcnr commented Jan 7, 2022

i personally would prefer you trying to implement the better fix first, we could merge the first change of this PR by itself.
If implementing Deref ends up being more difficult than expected, i can look at the second change in more detail and then merge both.

@compiler-errors
Copy link
Member Author

Closing in favor of the better solution, #92652

@compiler-errors compiler-errors mentioned this pull request Jan 7, 2022
Closed
@compiler-errors
Copy link
Member Author

Reviving this PR :-)

@lcnr
Copy link
Contributor

lcnr commented Jan 12, 2022

1 nit, after that r=me, thanks 👍

lcnr
lcnr reviewed Jan 12, 2022
View reviewed changes
compiler/rustc_traits/src/chalk/mod.rs Show resolved Hide resolved
compiler/rustc_typeck/src/check/method/confirm.rs Outdated
@@ -182,10 +182,15 @@ impl<'a, 'tcx> ConfirmContext<'a, 'tcx> {
target,
});

if let Some(unsize_target) = unsize {
if *unsize {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you change l168 from match &pick.autoref_or_ptr_adjustment { to match pick.autoref_or_ptr_adjustment {?

compiler-errors
compiler-errors commented Jan 12, 2022
View reviewed changes
compiler/rustc_typeck/src/check/method/confirm.rs

let mut target =
self.structurally_resolved_type(autoderef.span(), autoderef.final_ty(false));
Copy link
Member Author

Choose a reason for hiding this comment

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

afaict autoderef.final_ty is identical to the type returned by autoderef.nth, so this was a bit redundant

@lcnr
Copy link
Contributor

lcnr commented Jan 17, 2022

@bors r+ rollup

@lnicola
Copy link
Member

lnicola commented Jan 17, 2022

@bors r=lcnr rollup

@bors
Copy link
Contributor

bors commented Jan 17, 2022

📌 Commit 7bf0cb7 has been approved by lcnr

@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 Jan 17, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 18, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2022
@bors
Auto merge of rust-lang#93021 - matthiaskrgr:rollup-o7z8zoe, r=matthi…
7531d2f 
…askrgr

Rollup of 14 pull requests

Successful merges:

 - rust-lang#92629 (Pick themes on settings page, not every page)
 - rust-lang#92640 (Fix ICEs related to `Deref<Target=[T; N]>` on newtypes)
 - rust-lang#92701 (Add some more attribute validation)
 - rust-lang#92803 (Hide mobile sidebar on some clicks)
 - rust-lang#92830 (Rustdoc style cleanups)
 - rust-lang#92866 ("Does exists" typos fix)
 - rust-lang#92870 (add `rustc_diagnostic_item` attribute to `AtomicBool` type)
 - rust-lang#92914 (htmldocck: Add support for `/text()` in ``@snapshot`)`
 - rust-lang#92923 (Abstract the pretty printer's ringbuffer to be infinitely sized)
 - rust-lang#92946 (Exclude llvm-libunwind from the self-contained set on s390x-musl targets)
 - rust-lang#92947 (rustdoc: Use `intersperse` in a `visit_path` function)
 - rust-lang#92997 (Add `~const` bound test for negative impls)
 - rust-lang#93004 (update codegen test for LLVM 14)
 - rust-lang#93016 (Stabilize vec_spare_capacity)

Failed merges:

 - rust-lang#92924 (Delete pretty printer tracing)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit cb5ecff into rust-lang:master Jan 18, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 18, 2022
@compiler-errors compiler-errors deleted the array-deref-on-newtype branch April 7, 2022 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 left review comments

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
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
Milestone
1.60.0
9 participants
@compiler-errors @rust-highfive @lcnr @lnicola @bors @jackh726 @eddyb @estebank @rustbot