Skip to content

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

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
Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

compiler-errors
Copy link
Member

  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
Comment on lines +143 to +144
// FIXME: this is a hack to allow us to evaluate `<[T; N]>::deref` as const, since
// it's really just a pointer unsize from `&[T; N]` -> `&[T]`
Copy link
Member Author

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

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
Member Author

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

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

@compiler-errors
Copy link
Member Author

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