8000 Fully destructure slice and array constants into patterns by oli-obk · Pull Request #77390 · rust-lang/rust · GitHub
[go: up one dir, main page]
Skip to content

Fully destructure slice and array constants into patterns #77390

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

Closed
wants to merge 7 commits into from
Closed

Fully destructure slice and array constants into patterns #77390

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
002a1c2
Destructure byte array constants to array patterns instead of keeping…
oli-obk Sep 29, 2020
14a0b10
Destructure byte slices and remove all the workarounds
oli-obk Oct 1, 2020
e0e48ac
Treat booleans as integers with valid range 0..=1
oli-obk Oct 2, 2020
523b752
Stop passing opaque constants around in the exhaustiveness checks and…
oli-obk Oct 2, 2020
6553d38
Document new (or new uses) of enum variants
Oct 12, 2020
d0bb77d
Update comment
Oct 15, 2020
0915063
WIP: Add opaque constructor
Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Stop passing opaque constants around in the exhaustiveness checks and… 
… just treat them as nonexhaustive
  • Loading branch information
@oli-obk
oli-obk committed Oct 2, 2020
commit 523b752e55797c718f52432daaf34b7f2ba247a1
36 changes: 16 additions & 20 deletions compiler/rustc_mir_build/src/thir/pattern/_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,8 +820,8 @@ enum Constructor<'tcx> {
Single,
/// Enum variants.
Variant(DefId),
/// Literal values.
ConstantValue(&'tcx ty::Const<'tcx>),
/// String literals
Str(&'tcx ty::Const<'tcx>),
Copy link
Member

Choose a reason for hiding this comment

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

Okay so this type is indeed only used for exhaustiveness checking, that's why we do not need consts in here? Maybe add that to the type doccomment.

Copy link
Member

Choose a reason for hiding this comment

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

Why are strings special here?

/// Ranges of integer literal values (`2`, `2..=5` or `2..5`).
IntRange(IntRange<'tcx>),
/// Ranges of floating-point literal values (`2.0..=5.2`).
Expand All @@ -840,22 +840,13 @@ impl<'tcx> Constructor<'tcx> {
}
}

fn variant_index_for_adt<'a>(
&self,
cx: &MatchCheckCtxt<'a, 'tcx>,
adt: &'tcx ty::AdtDef,
) -> VariantIdx {
fn variant_index_for_adt(&self, adt: &'tcx ty::AdtDef) -> VariantIdx {
match *self {
Variant(id) => adt.variant_index_with_id(id),
Single => {
assert!(!adt.is_enum());
VariantIdx::new(0)
}
ConstantValue(c) => cx
.tcx
.destructure_const(cx.param_env.and(c))
.variant
.expect("destructed const of adt without variant id"),
_ => bug!("bad constructor {:?} for adt {:?}", self, adt),
}
}
Expand All @@ -869,7 +860,7 @@ impl<'tcx> Constructor<'tcx> {

match self {
// Those constructors can only match themselves.
Single | Variant(_) | ConstantValue(..) | FloatRange(..) => {
Single | Variant(_) | Str(_) | FloatRange(..) => {
if other_ctors.iter().any(|c| c == self) { vec![] } else { vec![self.clone()] }
}
&Slice(slice) => {
Expand Down Expand Up @@ -979,7 +970,7 @@ impl<'tcx> Constructor<'tcx> {
PatKind::Variant {
adt_def: adt,
substs,
variant_index: self.variant_index_for_adt(cx, adt),
variant_index: self.variant_index_for_adt(adt),
subpatterns,
}
} else {
Expand Down Expand Up @@ -1018,7 +1009,7 @@ impl<'tcx> Constructor<'tcx> {
PatKind::Slice { prefix, slice: Some(wild), suffix }
}
},
&ConstantValue(value) => PatKind::Constant { value },
&Str(value) => PatKind::Constant { value },
&FloatRange(lo, hi, end) => PatKind::Range(PatRange { lo, hi, end }),
IntRange(range) => return range.to_pat(cx.tcx),
NonExhaustive => PatKind::Wild,
Expand Down Expand Up @@ -1125,7 +1116,7 @@ impl<'p, 'tcx> Fields<'p, 'tcx> {
// Use T as the sub pattern type of Box<T>.
Fields::from_single_pattern(wildcard_from_ty(substs.type_at(0)))
} else {
let variant = &adt.variants[constructor.variant_index_for_adt(cx, adt)];
let variant = &adt.variants[constructor.variant_index_for_adt(adt)];
// Whether we must not match the fields of this variant exhaustively.
let is_non_exhaustive =
variant.is_field_list_non_exhaustive() && !adt.did.is_local();
Expand Down Expand Up @@ -1173,7 +1164,7 @@ impl<'p, 'tcx> Fields<'p, 'tcx> {
}
_ => bug!("bad slice pattern {:?} {:?}", constructor, ty),
},
ConstantValue(..) | FloatRange(..) | IntRange(..) | NonExhaustive => Fields::empty(),
Str(_) | FloatRange(..) | IntRange(..) | NonExhaustive => Fields::empty(),
};
debug!("Fields::wildcards({:?}, {:?}) = {:#?}", constructor, ty, ret);
ret
Expand Down Expand Up @@ -2110,7 +2101,12 @@ fn pat_constructor<'tcx>(
if let Some(int_range) = IntRange::from_const(tcx, param_env, value, pat.span) {
Some(IntRange(int_range))
} else {
Some(ConstantValue(value))
match value.ty.kind() {
ty::Ref(_, t, _) if t.is_str() => Some(Str(value)),
ty::Float(_) => Some(FloatRange(value, value, RangeEnd::Included)),
// Non structural-match values are opaque.
_ => None,
Copy link
Member

Choose a reason for hiding this comment

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

None here explicitely means a wildcard, so I'm not sure this is correct. Does this behave correctly? I would expect this implies we'd consider a branch that uses an opaque pattern to exhaustively match anything, which is the opposite of how it should behave.
What's an example of such an opaque constant? I'd like to construct a test.

I would instead add an Opaque constructor for this case to force us to treat it correctly. (I've also been meaning to add Wildcard as a constuctor instead of returning an Option here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember trying something like this, and could not figure out all the details. If you want to try to add an Opaque constructor, it's easy to construct an opaque constant by using a type that doesn't derive PartialEq but manually implements it:

#[derive(PartialEq)]
struct Opaque(i32);

impl Eq for Opaque {}

const FOO: &&Opaque = &&Opaque(42);

match FOO {
    FOO => {},
    // The following should not lint about unreachable
    Opaque(42) => {}
}

amusingly the above ICEs on the nightly playground, not sure if that has my PR yet.

Copy link
Member

Choose a reason for hiding this comment

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

a type that doesn't derive PartialEq but manually implements it:

But this derives it, no manual impl? Or did you mean Eq?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think either will do, we require both for structural equality. I was just lazy and didn't want to write the impl when the Eq impl is trivial to write

Copy link
Member
@Nadrieril Nadrieril Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Your example ICEs because of the refs, that's been an open issue for a while.

#[derive(PartialEq)]
struct Opaque(i32);

impl Eq for Opaque {}

const FOO: Opaque = Opaque(42);
match FOO {
    FOO => {},
    // The following should not lint about unreachable
    _ => {}
}

Without the refs it works, but does report the second branch as unreachable on both stable and nightly. I don't understand why, and I haven't seen it reported as a bug yet.

Copy link
Member

Choose a reason for hiding this comment

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

The good thing is that your incorrect usage of None above won't be a regression x)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did an experiment by returning Some(NonExhaustive), which made patterns like FOO above lint that they are unreachable, so that's definitely not it. I'm ot sure what Constructor::Opaque would do differently from NonExhaustive, but I'm going through all the uses of NonExhaustive now to analyze that

Copy link
Member

Choose a reason for hiding this comment

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

A simpler way to know what the Opaque constructor should do is look at all the places where ConstantValue was used but the constant wasn't inspected

Copy link
Member
@Nadrieril Nadrieril Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, an easy way to avoid a regression would be that instead of removing ConstantValue altogether like you did, you would rename it to Opaque and see what's left once you remove any code that inspects the constant.

Copy link
Member
@Nadrieril Nadrieril Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

All in all, given that matching with opaque constants is an error anyways, this should not allow any unsound code. So I'm not too worried if we let that slip. We can file the above as a bug and I'll introduce Opaque myself whenever I get some time and a powerful enough computer to play with rustc

}
}
}
PatKind::Range(PatRange { lo, hi, end }) => {
Expand Down Expand Up @@ -2458,7 +2454,7 @@ fn constructor_covered_by_range<'tcx>(
_ => bug!("`constructor_covered_by_range` called with {:?}", pat),
};
let (ctor_from, ctor_to, ctor_end) = match *ctor {
ConstantValue(value) => (value, value, RangeEnd::Included),
Str(value) => (value, value, RangeEnd::Included),
FloatRange(from, to, ctor_end) => (from, to, ctor_end),
_ => bug!("`constructor_covered_by_range` called with {:?}", ctor),
};
Expand Down Expand Up @@ -2558,7 +2554,7 @@ fn specialize_one_pattern<'p, 'tcx>(
let suffix = suffix.iter().enumerate().map(|(i, p)| (arity - suffix.len() + i, p));
Some(ctor_wild_subpatterns.replace_fields_indexed(prefix.chain(suffix)))
}
ConstantValue(_) => None,
Str(_) => None,
_ => span_bug!(pat.span, "unexpected ctor {:?} for slice pat", constructor),
},

Expand Down
0