Skip to content

Implement Deref<Target = [T]> for [T; N], and fix const infer variable canonicalization (fix Deref<Target=[T; N]> ICEs)#92652

Closed
compiler-errors wants to merge 6 commits intorust-lang:masterfrom
compiler-errors:array-deref-on-newtype2
Closed

Implement Deref<Target = [T]> for [T; N], and fix const infer variable canonicalization (fix Deref<Target=[T; N]> ICEs)#92652
compiler-errors wants to merge 6 commits intorust-lang:masterfrom
compiler-errors:array-deref-on-newtype2

Conversation

@compiler-errors
Copy link
Contributor

@compiler-errors compiler-errors commented Jan 7, 2022

  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. Implement Deref for [T; N] which allows us to remove some custom array unsizing logic that was leaking an inference variable during method selection.
    Fixes Calling slice method on newtype wrapper that implements Deref<Target=[_; 1]> causes ICE #92637

r? @lcnr

@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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2022
@compiler-errors compiler-errors mentioned this pull request Jan 7, 2022
Merged
compiler-errors
compiler-errors commented Jan 7, 2022
View reviewed changes
compiler/rustc_mir_build/src/thir/cx/expr.rs Outdated
Comment on lines 143 to 144
Copy link
Contributor Author

@compiler-errors compiler-errors Jan 7, 2022

Choose a reason for hiding this comment

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

This is the only part of the PR I am somewhat unhappy with, but it works.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors compiler-errors changed the title Fix ICEs related to Deref<Target=[T; N]> on newtypes (attempt 2) Implement Deref<Target = [T]> for [T; N], and fix const infer variable canonicalization (fix Deref<Target=[T; N]> ICEs) Jan 7, 2022
@compiler-errors compiler-errors force-pushed the array-deref-on-newtype2 branch from 5205639 to 595657d Compare January 7, 2022 20:03
@eddyb
Copy link
Member

eddyb commented Jan 7, 2022

Trait impls are insta-stable and I strongly disagree with a [T; N] -> [T] deref impl, regardless.

(click to expand my reasons - they're not really relevant outside of a FCP/RFC discussion)

The reason deref impls like Vec<T> -> [T] exist is because Vec<T> is like Box<[T]>, i.e. a pointer to [T].

Array unsizing (to slice) is in a different coercion category than deref coercions, and more flexible at that (e.g. Rc<[T; N]> -> Rc<[T]>, where a deref coercion couldn't give back an Rc) - I also don't see any reason to mix up the two too much (though long-term I would like to see generalized coercions).

Something this significant requires at least an FCP by @rust-lang/lang, if not an RFC.

@rust-log-analyzer
Copy link
Collaborator

rust-log-analyzer commented Jan 7, 2022

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot) 
Set({"src/doc/edition-guide"}) not skipped for "bootstrap::doc::EditionGuide" -- not in ["src/tools/tidy"]
Rustbook (x86_64-unknown-linux-gnu) - edition-guide
Building stage0 tool linkchecker (x86_64-unknown-linux-gnu)
    Finished release [optimized] target(s) in 0.16s
core/primitive.array.html:336: broken link fragment `#method.to_ascii_uppercase` pointing to `core/primitive.array.html`
core/primitive.array.html:341: broken link fragment `#method.to_ascii_lowercase` pointing to `core/primitive.array.html`
core/primitive.array.html:1434: broken link - `core/slice::sort_by_key`
core/primitive.array.html:1519: broken link fragment `#method.sort_by_cached_key` pointing to `core/primitive.array.html`
number of HTML files scanned: 31972
number of HTML redirects found: 9914
number of links checked: 2162137
number of links ignored due to external: 89734

@compiler-errors
Copy link
Contributor Author

compiler-errors commented Jan 7, 2022

Thanks for the feedback, @eddyb.

For now, I will reopen #92640 which implements a less dramatic set of changes to fix these ICEs? Would you or @lcnr be willing to review that PR instead?

@lcnr
Copy link
Contributor

lcnr commented Jan 12, 2022 •
edited
Loading

I misunderstood what exactly is happening here and after @eddyb's comment i am now also not in favor of [T; N] implementing Deref to [T] anymore, though I am not really opposed to it either 😆

will review #92640 and am in favor of only going forward with Deref for arrays in case an unrelated legitimate use case comes up

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 12, 2022

I agree with @eddyb that a Deref impl for [T; N] -> [T] is not obviously correct =)

@compiler-errors
Copy link
Contributor Author

compiler-errors commented Jan 12, 2022

Alright, I'll close this out then

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 19, 2022
@lcnr lcnr mentioned this pull request Mar 31, 2022
Closed
@compiler-errors compiler-errors deleted the array-deref-on-newtype2 branch April 7, 2022 04:35
@compiler-errors compiler-errors mentioned this pull request Mar 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@lcnr lcnr

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. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

8 participants

@compiler-errors @rust-log-analyzer @eddyb @lcnr @nikomatsakis @dtolnay @rust-highfive @rustbot