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

wart: non-Eq [T; 0] can be matched as if T were #[structural_match] #62336

Closed
pnkfelix opened this issue Jul 3, 2019 · 10 comments
Closed

wart: non-Eq [T; 0] can be matched as if T were #[structural_match] #62336

pnkfelix opened this issue Jul 3, 2019 · 10 comments
Labels
A-array Area: [T; N] A-patterns Relating to patterns and pattern matching A-zst Area: Zero-sized types (ZST). T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Jul 3, 2019

Spawned off of investigation of issue #62307 and PR #55837

We currently allow constants that are empty arrays of any type to be used as patterns in match. We allow this regardless of whether they are an array of an ADT that does not derive PartialEq/Eq

This may be a feature or a bug depending on one's perspective.

Here is an example of the behavior in question (play):

struct B(i32);

const B0: [B; 0] = [];

//#[derive(PartialEq, Eq)] // can't uncomment b/c B doesn't impl PartialEq+Eq.
struct UhOh([B; 0]);

const _UH_OH: UhOh = UhOh(B0);

fn main() {
    match [] {
        B0 => { println!("B0 matched []"); }
    }

    match UhOh([]) {
        UhOh(B0) => { println!("UhOh(B0) matched UhOh([])"); }
    }

    #[cfg(this_wont_compile_without_derive_of_partial_eq_and_eq)]
    match UhOh([]) {
        _UH_OH => { println!("_UH_OH matched UhOh([]])"); }
    }
}

To be clear: This behavior might be fine.

It is just a little weird, because on other uses of consts in patterns, we do tend to require that their types derive PartialEq and Eq

But we can treat an empty array as an exceptional case here, if that's what people want.

@pnkfelix pnkfelix added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels Jul 3, 2019
@pnkfelix
Copy link
Member Author

pnkfelix commented Jul 3, 2019

nominating for discussion at T-lang team meeting, since this is a question about the design of the language itself.

(It may be worthwhile to try to make a PR that errors on such consts, e.g. rejecting the match [] { B0 => ... } in the example above, and then doing a crater run against that PR, to gather data on whether this is likely to occur in the wild.)

@pnkfelix pnkfelix changed the title wart: non-Eq [T; 0] can be matched as if #[structural_match] wart: non-Eq [T; 0] can be matched as if T were #[structural_match] Jul 3, 2019
@pnkfelix
Copy link
Member Author

pnkfelix commented Jul 3, 2019

A possibly interesting note: I think the reason this case wasn't caught was because of how this is written:

ty::Array(_, n) => {
PatternKind::Array {
prefix: (0..n.unwrap_usize(self.tcx))
.map(|i| adt_subpattern(i as usize, None))
.collect(),
slice: None,
suffix: Vec::new(),
}
}

because we have layered the type-checking atop the recursion (encoded in the adt_subpattern closure), and there is no substructure to recur on when n=0.

I mainly note this because I'm curious if there's other checks on the contents' type in [T; n] that are skipped because of similar folding over 0..n.

@varkor
Copy link
Member

varkor commented Jul 3, 2019

This is consistent with special handling for zero-length arrays throughout the language. For instance, Default is implemented for zero-length arrays, even for types that themselves don't implement Default. (I think this is the right decision, but it does make some behaviour more complex.)

@Centril
Copy link
Contributor

Centril commented Jul 3, 2019

Further to @varkor's point, another case where LEN = 0 is "special" (or rather <= 1 is special for expected move-checking reasons) is the case of array repeat expressions, e.g.

struct NoCopy;
let x = [NoCopy; 0]; // OK.
let x = [NoCopy; 1]; // OK.
let x = [NoCopy; 2]; // ERROR.

@pnkfelix
Copy link
Member Author

pnkfelix commented Jul 5, 2019

And yet the type [NoCopy; 0] is itself non-copyable... right? (play).

seems like something that's been decided in an ad hoc manner in each case, rather than having a consistent policy, no?

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

@pnkfelix A very good point! It seems pretty inconsistent indeed.

Might be good to do a survey if time allows. Ostensibly making [NoCopy; 0] Copy is the non-breaking change in "what direction should we move consistency in" discussion.

One further case in which [T; 0] is special is if is_uninhabited(T) holds and the corollary to this: [T; 0] is always a singleton type (if we assume totality under a sort of fast and loose reasoning).

bors added a commit that referenced this issue Jul 10, 2019
…l-match-check, r=nikomatsakis

use visitor for #[structural_match] check

This changes the code so that we recur down the structure of a type of a const (rather than just inspecting at a shallow one or two levels) when we are looking to see if it has an ADT that did not derive `PartialEq` and `Eq`.

Fix #61188

Fix #62307

Cc #62336
@nikomatsakis
Copy link
Contributor

Discussed briefly in the @rust-lang/lang meeting. We need to schedule some time to get into structural match. But, in the meantime, a useful step would be to do a crater run that tries to remove the [Foo; 0] case and see whether it has any effect.

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Jul 11, 2019
@nikomatsakis
Copy link
Contributor

I don't know @pnkfelix whether you'd have time to do that before you leave, but for anyone else who might have an interest, you could take a look at #62339 which adjusted this code and try to build a test PR from that.

@pnkfelix pnkfelix added the A-patterns Relating to patterns and pattern matching label Apr 28, 2020
@workingjubilee workingjubilee added A-zst Area: Zero-sized types (ZST). A-array Area: [T; N] labels Oct 6, 2021
@lcnr
Copy link
Contributor

lcnr commented Jul 15, 2024

cc @RalfJung @oli-obk I think this issue can now be closed?

Afaict we now always need the type to implement PartialEq/Eq, even when inside of a zero-sized array, but it does not have to be derived. This is as any wrapper types for which we have a value need to derive PartialEq, meaning that its zero-length array field has to implement PartialEq.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b5239f44a96ae232228b5c9677390dbf

@RalfJung
Copy link
Member

Indeed this should have been closed together with #120362. Empty arrays with element type T behave exactly like None::<T>: the type T must implement PartialEq, but it must not be derived, it can be a custom impl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: [T; N] A-patterns Relating to patterns and pattern matching A-zst Area: Zero-sized types (ZST). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants