Infer type in irrefutable slice patterns with fixed length as array

[b-naber]

Fixes #76342

In irrefutable slice patterns with a fixed length, we can infer the type as an array type. We now choose to prefer some implementations over others, e.g. in:

struct Zeroes;

const ARR: [usize; 2] = [0; 2];
const ARR2: [usize; 2] = [2; 2];

impl Into<&'static [usize; 2]> for Zeroes {
    fn into(self) -> &'static [usize; 2] {
        &ARR
    }
}

impl Into<&'static [usize]> for Zeroes {
    fn into(self) -> &'static [usize] {
        &ARR2
    }
}

fn main() {
    let &[a, b] = Zeroes.into();
}

We now prefer the impl candidate impl Into<&'static [usize; 2]> for Zeroes, it's not entirely clear to me that this is correct, but given that the slice impl would require a type annotation anyway, this doesn't seem unreasonable.

r? @lcnr

[bors]

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

[lcnr]

feels like els should be part of Origin instead of being a separate field

[lcnr]

happy with the changes to allow arrays, the changes with PatInfo feel like they're only halfway '^^

[lcnr]

Suggested change

	pub(super) fn try_get_els(&self) -> Option<&'a hir::Block<'a>> {
	pub(super) fn try_get_else(&self) -> Option<&'a hir::Block<'a>> {

els is only used because else is a keyword and can't be the name of a local :>

[lcnr]

please don't destructure pat_info if you then rebuild it for nested calls

[b-naber]

Note that we create a new def_bm in this method later.

[lcnr]

can you in this case instead shaddow pat_info after we have the new def_bm?

[lcnr]

same here

[lcnr]

and here

[bors]

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

[lcnr]

can you in this case instead shaddow pat_info after we have the new def_bm?

[lcnr]

Suggested change

	decl_origin.try_get_else().is_none()
	&& matches!(decl_origin, DeclOrigin::LocalDecl { .. })
	match decl_origin {
	DeclOrigin::LocalDecl { els: None } => true,
	DeclOrigin::LOcalDecl { else: Some(_) } | DeclOrigin::LetExpr => false,
	}

[lcnr]

not sure whether this test tests anything specific 🤔 might make snese to only have this as a check-pass test for irrefutable let?

please add the test separately where we incompletely a pattern as an array even though it should be a slice #76342 (comment)

[b-naber]

please add the test separately where we incompletely a pattern as an array even though it should be a slice #76342 (comment)

Sorry, I don't understand what you're trying to say here. The quote does not seem to relate to the comment you link to.

[lcnr]

this test:

struct Foo(u32);
struct Bar;
impl Into<&'static Foo> for Bar {
    fn into(self) -> &'static Foo { &Foo(1) }
}
impl Into<Foo> for Bar {
    fn into(self) -> Foo { Foo(1) }
}

fn ty<T>(_: T) {
    println!("{}", std::any::type_name::<T>());
}

fn main() {
    let Foo(i) = Bar.into();
    ty(i); // u32
    let Foo(j) = <Bar as Into<&'static Foo>>::into(Bar);
    ty(j); // &u32
    let Foo(k): Foo = Bar.into();
    ty(k); // u32
    let Foo(l): &Foo = Bar.into();
    ty(l); // &u32
}

[lcnr]

woops, replacing Foo with an array 😓

#![allow(unused_variables)]
struct Zeroes;
impl Into<&'static [usize; 3]> for Zeroes {
    fn into(self) -> &'static [usize; 3] { &[0; 3] }
}
impl Into<[usize; 3]> for Zeroes {
    fn into(self) -> [usize; 3] { [0; 3] }
}
fn main() {
    let [a, b, c] = Zeroes.into();
    let [d, e, f] = <Zeroes as Into<&'static [usize; 3]>>::into(Zeroes);
    let &[g, h, i] = Zeroes.into();
    let [j, k, l]: [usize; _] = Zeroes.into();
    let [m, n, o]: &[usize; _] = Zeroes.into();

    // check the binding mode of these patterns:
    let _: &[usize] = &[a, b, c, g, h, i, j, k, l];
    let _: &[&usize] = &[d, e, f, m, n, o];

}

[b-naber]

let [d, e, f] = <Zeroes as Into<&'static [usize; 3]>::into();

Again not sure what you're trying to do here. This doesn't look like valid rust code to me. into takes a receiver.

[lcnr]

it can be valid by adding Zeroes as a receiver

    let [d, e, f] = <Zeroes as Into<&'static [usize; 3]>>::into(Zeroes);

[lcnr]

please move this if until after the structurally_resolve_type as it can otherwise miss info about the type of expected.

[b-naber]

The new tests previously failed because structurally_resolve_type returned an inference error.

[lcnr]

ah, I guess we need try_structurally_resolve_type here 😅

[b-naber]

I'm not convinced this is necessary. We only get into the demand_eqtype call when the pattern is irrefutable, how exactly would trying to structurally resolve the type help here? Afaict either there's a type annotation and we ignore all the logic introduced in this PR or there is none, in which case expected is just an unresolvable type variable afaict. Since we have to resolve the type after the demand_eqtype call anyway (since we still have to resolve the inner type after hinting that we're looking for [_; n]) I don't see why we want try_structurally_resolve_type first. Can you explain why that would be helpful/necessary?

[lcnr]

we get into the demand_eqtype call when the pattern is irrefutable and the expected type is an inference variable. Whether the expected type is an inference variable can depend on structrally resolving it.

I can't imagine how you'd get to a point where it matters, but generally looking at the kind() of a type and then using structurally_resolve or shallow_resolve or whatever is always sus. Either the resolve is a noop, in which case it should be removed, or it can change the kind() of the type, in which case looking at the type before the resovle call is buggy.

Ah, nearly forgot: can you add tests for destructing assignments, i.e. what happens with the following

fn main() {
    let a: i32;
    let b;
    [a, b] = Default::default();
}

we should treat these as also being irrefutable and this example should compile i think

[lcnr]

ah, two more things:

• can you rename this file to slice-patterns-irrefutable.rs?
• can you add the following function to this test

use std::array::TryFromSliceError;
fn array_try_from(x: &[usize]) -> Result<usize, TryFromSliceError> {
    let [a, b] = x.try_into()?;
    Ok(a + b)
}

[lcnr]

This PR infers the type of slice pattern in irrefutable positions, i.e. let bindings, to an array with the length of the pattern. This inference incompletely assumes that no match ergonomics are taking place and the type of the pattern is an array, not a reference to an array. This matches the behavior of other patterns: #76342 (comment). We do not make inference progress if the slice pattern contains a .. subpattern.

This behavior is generally desirable when using TryFrom, i.e. let [a, b] = slice.try_into()?;.

#![allow(unused_variables)]
struct Zeroes;
impl Into<&'static [usize; 3]> for Zeroes {
    fn into(self) -> &'static [usize; 3] { &[0; 3] }
}
impl Into<[usize; 3]> for Zeroes {
    fn into(self) -> [usize; 3] { [0; 3] }
}
fn main() {
    let [a, b, c] = Zeroes.into();
    let [d, e, f] = <Zeroes as Into<&'static [usize; 3]>>::into(Zeroes);
    let &[g, h, i] = Zeroes.into();
    let [j, k, l]: [usize; _] = Zeroes.into();
    let [m, n, o]: &[usize; _] = Zeroes.into();

    // check the binding mode of these patterns:
    let _: &[usize] = &[a, b, c, g, h, i, j, k, l];
    let _: &[&usize] = &[d, e, f, m, n, o];

}

This also works for nested slice pattern, see https://github.com/rust-lang/rust/blob/046d751e4f5351e40e1a7f96dc3ee73ab0fe5dc7/tests/ui/pattern/slice-patterns-nested.rs#L4-L15

Given that this is a detail of type inference, I think this is full t-types, cc @rust-lang/lang

@rfcbot fcp merge

[rfcbot]

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @aliemjay
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[bors]

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

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[lcnr]

final nits, then r=me

[lcnr]

@bors r+ rollup=never

[bors]

📌 Commit 5a9af37 has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 5a9af37 with merge fcf3006...

[pietroalbini]

@bors retry

Yield priority to #114410

[bors]

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

[rust-timer]

Finished benchmarking commit (fcf3006): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.3%	[1.3%, 1.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-	-	0

Cycles

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)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 649.925s -> 649.462s (-0.07%)