Stream: t-compiler

Topic: Structural match marker traits


ecstatic-morse (Apr 04 2020 at 21:22, on Zulip):

@oli I see you are working on to refactor the support for consts in patterns. I left a comment on your PR that adds some StructuralEq impls. The weirdness around bounds is explicitly documented in the trait docs. Basically we have to use a type visitor to recurse through all the members of a type in question to determine whether it is "always" structurally matchable (e.g., usize, (usize, usize), enum X { A(uszie), B(usize) }) or only "sometimes" structurally matchable (e.g. Option<T>). For the sometimes types, we will do structural qualification like we do for NeedsDrop and HasMutInterior.

oli (Apr 06 2020 at 09:10, on Zulip):

we could also add a method to StructuralEq and make miri interpret it for a specific constant

oli (Apr 06 2020 at 09:11, on Zulip):

that way we need no visitor :D

oli (Apr 06 2020 at 09:11, on Zulip):

(but we need a hack that allows us to define associated const functions)

oli (Apr 06 2020 at 09:11, on Zulip):

oh... maybe we can do it with an assoc const?

oli (Apr 06 2020 at 09:11, on Zulip):

ah no

oli (Apr 06 2020 at 09:11, on Zulip):

we need the value as input

oli (Apr 06 2020 at 09:17, on Zulip):

also that example in the docs is weird. I don't get how function pointers can be StructuralEq at all

oli (Apr 06 2020 at 09:17, on Zulip):

function pointers to the same function can be different if they were created in different crates

eddyb (Apr 06 2020 at 12:19, on Zulip):

@oli wasn't there a discussion with... you and @varkor, maybe? where we decided const generics + structural eq has really terrible edge cases around pointer equality?

eddyb (Apr 06 2020 at 12:20, on Zulip):

all builtin types have implicit StructuralEq instead of whitelisting the ones that make sense :(

eddyb (Apr 06 2020 at 12:22, on Zulip):

e.g. ==/!= on the ConstValue should be sound to rely on in pattern-matching

eddyb (Apr 06 2020 at 12:22, on Zulip):

for leaves, references being the only indirection traversed automatically before that

eddyb (Apr 06 2020 at 12:22, on Zulip):

seems unsound to have any kind of address in this system

eddyb (Apr 06 2020 at 12:23, on Zulip):

the only non-ZST leaves should be pure integers

eddyb (Apr 06 2020 at 12:23, on Zulip):

like &str would compare the actual UTF-8 bytes which are integers

eddyb (Apr 06 2020 at 12:24, on Zulip):

so I guess both pattern-matching and const generics need us to ban code that compiles on stable :/

eddyb (Apr 06 2020 at 12:24, on Zulip):

is there an issue about this?

eddyb (Apr 06 2020 at 12:24, on Zulip):

cc @nikomatsakis @pnkfelix who might not have seen previous discussions we had (elsewhere? Discord maybe? I forget)

eddyb (Apr 06 2020 at 12:25, on Zulip):

but also those previous discussions didn't focus on how pattern-matching may interact with non-integers

oli (Apr 06 2020 at 12:26, on Zulip):

you can't traverse references automatically, since you can manually implement PartialEq for &T where T: !StructPartialEq

eddyb (Apr 06 2020 at 12:26, on Zulip):

I'm talking about StructuralEq references

eddyb (Apr 06 2020 at 12:26, on Zulip):

any others are just banned

oli (Apr 06 2020 at 12:27, on Zulip):

i thought others just default to ==?

oli (Apr 06 2020 at 12:27, on Zulip):

for backcompat

oli (Apr 06 2020 at 12:27, on Zulip):

and don't get any exhaustiveness or other fancy checks

eddyb (Apr 06 2020 at 12:27, on Zulip):

sure but that's not relevant to soundness and const generics

eddyb (Apr 06 2020 at 12:27, on Zulip):

because they don't factor into exhaustiveness and whatnot

eddyb (Apr 06 2020 at 12:28, on Zulip):

and const generics would just ban them

oli (Apr 06 2020 at 12:28, on Zulip):

oh, you mean the breaking change is that they are relevant for exhaustivness right now?

eddyb (Apr 06 2020 at 12:28, on Zulip):

the problem is StructuralEq raw/fn pointers

eddyb (Apr 06 2020 at 12:28, on Zulip):

compared by abstract address

oli (Apr 06 2020 at 12:28, on Zulip):

yea

eddyb (Apr 06 2020 at 12:29, on Zulip):

references are fine because they're like a (T,) to the structural eq property

eddyb (Apr 06 2020 at 12:29, on Zulip):

but any other kind of indirection is not

oli (Apr 06 2020 at 12:29, on Zulip):

boxes?

eddyb (Apr 06 2020 at 12:29, on Zulip):

not a thing AFAIK, you have to use box patterns

oli (Apr 06 2020 at 12:30, on Zulip):

yes

oli (Apr 06 2020 at 12:30, on Zulip):

so unstable -> let's ignore

eddyb (Apr 06 2020 at 12:30, on Zulip):

@oli if there isn't an issue already about non-integral leaves, can you open one? I'm already doing several things at once

eddyb (Apr 06 2020 at 12:30, on Zulip):

or maybe I should, it's not clear I got my point across Q_Q

oli (Apr 06 2020 at 12:30, on Zulip):

you didn't

eddyb (Apr 06 2020 at 12:30, on Zulip):

and I don't have the time to invent examples

eddyb (Apr 06 2020 at 12:30, on Zulip):

@oli so, !StructuralEq types are sound by definition since we don't trust them

eddyb (Apr 06 2020 at 12:31, on Zulip):

but there are untrustworthy primitives that nevertheless we implemented StructuralEq for

eddyb (Apr 06 2020 at 12:31, on Zulip):

because we assumed that only custom impls could be untrustworthy

eddyb (Apr 06 2020 at 12:31, on Zulip):

but that's just wrong

oli (Apr 06 2020 at 12:32, on Zulip):

but what if we just stopped doing that? would we break anything? you can't exhaustively match function pointers or even raw pointers for that matter

eddyb (Apr 06 2020 at 12:32, on Zulip):

okay yeah I'm suggesting to try breaking them

oli (Apr 06 2020 at 12:32, on Zulip):

we'd stop detecting unreachable arms, but :shrug:

eddyb (Apr 06 2020 at 12:32, on Zulip):

and the hard part would be finding an example showing they're unsound

oli (Apr 06 2020 at 12:32, on Zulip):

ok, with that info I can work, I'll play a bit and open an issue

eddyb (Apr 06 2020 at 12:33, on Zulip):

maybe we can use const generics to craft an example? but idk if it even enforces StructuralEq yet

oli (Apr 06 2020 at 12:34, on Zulip):

it doesn't

eddyb (Apr 06 2020 at 12:34, on Zulip):

I guess we can't because the value is never computed at runtime

eddyb (Apr 06 2020 at 12:35, on Zulip):

so it's just weird right now

oli (Apr 06 2020 at 12:36, on Zulip):

ah... now I get what @ecstatic-morse is talking about. We can use a constant in pattern matching if its value does not contain any !StructuralEq values

eddyb (Apr 06 2020 at 12:36, on Zulip):

yeah

eddyb (Apr 06 2020 at 12:41, on Zulip):

@oli https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=bd3fca6a9c204a1579448e465461f28b

eddyb (Apr 06 2020 at 12:41, on Zulip):

this is a fun example

eddyb (Apr 06 2020 at 12:42, on Zulip):

compare output from debug vs release

eddyb (Apr 06 2020 at 12:43, on Zulip):

it doesn't show anything egregious

eddyb (Apr 06 2020 at 12:45, on Zulip):

@oli and this is what happens if you make fn pointers non-StructuralEq https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=801477c9fcfad4074c0daa3683ff00e3

eddyb (Apr 06 2020 at 12:45, on Zulip):

doesn't fall back to ==

oli (Apr 06 2020 at 12:54, on Zulip):

but we could avoid any breaking changes by falling back to ==

eddyb (Apr 06 2020 at 12:54, on Zulip):

eeeeeeeh

eddyb (Apr 06 2020 at 12:55, on Zulip):

you'd first have to figure out why we broke things in the past (e.g. floats) without doing that

eddyb (Apr 06 2020 at 12:55, on Zulip):

I think it's hard to keep things sound when you have nested patterns

eddyb (Apr 06 2020 at 12:55, on Zulip):

probably have to treat it like x if x == CONST

eddyb (Apr 06 2020 at 12:55, on Zulip):

but we don't have that today

eddyb (Apr 06 2020 at 12:56, on Zulip):

so it'd have to be like ref x that can still fail to match

varkor (Apr 06 2020 at 13:05, on Zulip):

but idk if it even enforces StructuralEq yet

const generics do enforce structural_match now

varkor (Apr 06 2020 at 13:07, on Zulip):

but what if we just stopped doing that? would we break anything?

we would break using constants of those types in patterns

varkor (Apr 06 2020 at 13:09, on Zulip):

ah, I see that was pointed out above

varkor (Apr 06 2020 at 13:10, on Zulip):

I wonder how much fallout we would get if we just banned pointers without adding a == fallback

varkor (Apr 06 2020 at 13:10, on Zulip):

how many people can really be relying on explicit pointer matching? :/

eddyb (Apr 06 2020 at 13:13, on Zulip):

note that I'm not 100% sure how type-based it can be, I'm talking about pointer values, which could potentially be hidden in integers (I hope not?)

eddyb (Apr 06 2020 at 13:13, on Zulip):

and *mut T with an integer value would be a perfectly fine pattern :P

oli (Apr 06 2020 at 13:16, on Zulip):

we are forbidding usize constants from having relocation value

eddyb (Apr 06 2020 at 13:17, on Zulip):

okay so at least that shouldn't cause issues

varkor (Apr 06 2020 at 13:17, on Zulip):

we surely don't want to make the structural match property depend on values?

oli (Apr 06 2020 at 13:17, on Zulip):

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d60d7ca1fd1a804026813a88b3e5fca9

oli (Apr 06 2020 at 13:18, on Zulip):

we may have to, but I'd prefer not to either

oli (Apr 06 2020 at 13:18, on Zulip):

my brain breaks enough with the value based things for UnsafeCell

eddyb (Apr 06 2020 at 13:18, on Zulip):

if we can avoid it I'll be happy

varkor (Apr 06 2020 at 13:19, on Zulip):

what's the issue with trying to disable structural match for pointer types and then if users store addresses in other types, that's on them?

eddyb (Apr 06 2020 at 13:20, on Zulip):

no it's fine, as long as we ban addresses in other types early

oli (Apr 06 2020 at 13:20, on Zulip):

we do

ecstatic-morse (Apr 06 2020 at 17:07, on Zulip):

@oli I also don't understand why StructuralEq needs to exist after reading its documentation, perhaps @pnkfelix could inform us?

oli (Apr 06 2020 at 17:08, on Zulip):

I think it's for floats basically. Floats are !StructuralEq but they are StructuralPartialEq because of NaNs

ecstatic-morse (Apr 06 2020 at 17:10, on Zulip):

The documentation explicitly calls out function pointers not implementing PartialEq and Eq, but I thought they did?

ecstatic-morse (Apr 06 2020 at 17:11, on Zulip):

oli said:

I think it's for floats basically. Floats are !StructuralEq but they are StructuralPartialEq because of NaNs

Hm, but then we could just check for Eq. We wouldn't need a separate structural version.

oli (Apr 06 2020 at 17:12, on Zulip):

Can't you impl Eq manually for a type that is StructuralPartialEq?

oli (Apr 06 2020 at 17:13, on Zulip):

or or is an Eq derive also required for StructuralPartialEq?

ecstatic-morse (Apr 06 2020 at 17:13, on Zulip):

In any case, I'll be working on using the Qualif machinery for determining structural match. One of the difficulties I had was that pattern lowering was a bit messy, so I'm very happy to see it get refactored.

oli (Apr 06 2020 at 17:13, on Zulip):

Do we really need such a value based analysis?

ecstatic-morse (Apr 06 2020 at 17:14, on Zulip):

The goal would be to fix some longstanding bugs where we accidentally allow some non structural match constants in patterns.

eddyb (Apr 06 2020 at 17:14, on Zulip):

StructuralPartialEq+StructuralEq is the replacement for #[structural_match]

oli (Apr 06 2020 at 17:14, on Zulip):

right, but the alternative solution is to just allow them and treat them as opaque

ecstatic-morse (Apr 06 2020 at 17:14, on Zulip):

It's not to allow new things

eddyb (Apr 06 2020 at 17:14, on Zulip):

which was too fragile

eddyb (Apr 06 2020 at 17:15, on Zulip):

(and also problematic to have, since it detected two derives at once)

ecstatic-morse (Apr 06 2020 at 17:15, on Zulip):

/me goes to look up some issue numbers

eddyb (Apr 06 2020 at 17:15, on Zulip):

the traits are just there to detect a property

eddyb (Apr 06 2020 at 17:15, on Zulip):

i.e. that #[derive(PartialEq, Eq)] was used

eddyb (Apr 06 2020 at 17:16, on Zulip):

each trait is meaningless on its own

ecstatic-morse (Apr 06 2020 at 17:17, on Zulip):

@eddyb I'm still unclear why we need both a StructuralPartialEq and a StructuralEq. Here's the docs for StructuralEq:

In a more ideal world, we could check that requirement by just checking that the given type implements both (1.) the StructuralPartialEq trait and (2.) the Eq trait. However, you can have ADTs that do derive(PartialEq, Eq), and be a case that we want the compiler to accept, and yet the constant's type fails to implement Eq.

Namely, a case like this:

#[derive(PartialEq, Eq)]
struct Wrap<X>(X);
fn higher_order(_: &()) { }
const CFN: Wrap<fn(&())> = Wrap(higher_order);
fn main() {
    match CFN {
        CFN => {}
        _ => {}
    }
}

(The problem in the above code is that Wrap<fn(&())> does not implement PartialEq, nor Eq, because for<'a> fn(&'a _) does not implement those traits.)

Therefore, we cannot rely on naive check for StructuralPartialEq and mere Eq.

eddyb (Apr 06 2020 at 17:18, on Zulip):

what

eddyb (Apr 06 2020 at 17:18, on Zulip):

the reason for two traits is because that's the only way to do this

eddyb (Apr 06 2020 at 17:18, on Zulip):

am I missing something?

pnkfelix (Apr 06 2020 at 17:19, on Zulip):

ecstatic-morse said:

The documentation explicitly calls out function pointers not implementing PartialEq and Eq, but I thought they did?

for <'a> fn(&'a _) does not, I think...

eddyb (Apr 06 2020 at 17:19, on Zulip):

#[derive(PartialEq)] + manual Eq impl is the thing we want to guard against

oli (Apr 06 2020 at 17:19, on Zulip):

pnkfelix said:

ecstatic-morse said:

The documentation explicitly calls out function pointers not implementing PartialEq and Eq, but I thought they did?

for <'a> fn(&'a _) does not, I think...

can't we "just" fix that? More compiler magic or sth

eddyb (Apr 06 2020 at 17:19, on Zulip):

also I thought all builtin impls were just automatically StructuralEq which is the reason we have the bug where pointers are considered StructuralEq

eddyb (Apr 06 2020 at 17:20, on Zulip):

or was that only the case with the old #[structural_match]?

oli (Apr 06 2020 at 17:20, on Zulip):

it's all very confusing :D

ecstatic-morse (Apr 06 2020 at 17:20, on Zulip):

#62614 is the main one. #65466 is related, but I think a value-based strategy is not necessary to resolve it.

oli (Apr 06 2020 at 17:20, on Zulip):

I haven't found any impls for primitives in libcore

eddyb (Apr 06 2020 at 17:20, on Zulip):

my opinion was that we're missing a whitelist in the compiler: not all primitives should be considered structurally matchable, IMO

pnkfelix (Apr 06 2020 at 17:21, on Zulip):

eddyb said:

#[derive(PartialEq)] + manual Eq impl is the thing we want to guard against

(or a manual PartialEq and a #[derive(Eq)], right?)

eddyb (Apr 06 2020 at 17:21, on Zulip):

sure yeah

eddyb (Apr 06 2020 at 17:21, on Zulip):

each trait corresponds to one #[derive(...)]

eddyb (Apr 06 2020 at 17:22, on Zulip):

there's two because there's two #[derive(...)]s we need to check the conjunction of

pnkfelix (Apr 06 2020 at 17:22, on Zulip):

right, because that was the simplest way to handle the injection via derive

pnkfelix (Apr 06 2020 at 17:22, on Zulip):

(s/simplest/only/, if you like)

eddyb (Apr 06 2020 at 17:22, on Zulip):

this used to be done in the implementation of those derives, by injecting#[structural_match] when both were present

eddyb (Apr 06 2020 at 17:22, on Zulip):

but that's not something the proc macro system is meant to support, hence the traits

eddyb (Apr 06 2020 at 17:22, on Zulip):

(or can fully support)

oli (Apr 06 2020 at 17:23, on Zulip):

I'm not really sure primites implement StructuralEq

ecstatic-morse (Apr 06 2020 at 17:23, on Zulip):

@oli most primitives are currently handled by a TypeVisitor, that's why those impls aren't needed for things to work.

eddyb (Apr 06 2020 at 17:23, on Zulip):

maybe the traits are poorly named

oli (Apr 06 2020 at 17:23, on Zulip):

nothing in the type_marked_structural implementation mentions primitives

eddyb (Apr 06 2020 at 17:24, on Zulip):

AutoDerivedPartialEq and AutoDerivedEq may have been better

eddyb (Apr 06 2020 at 17:24, on Zulip):

because that's all they are

pnkfelix (Apr 06 2020 at 17:24, on Zulip):

does anything else manually implement it them?

ecstatic-morse (Apr 06 2020 at 17:24, on Zulip):

Err, well I guess just pointers

ecstatic-morse (Apr 06 2020 at 17:24, on Zulip):

Hmm

oli (Apr 06 2020 at 17:25, on Zulip):

ahh. yea, typed_marked_structural does not recurse

eddyb (Apr 06 2020 at 17:25, on Zulip):

I think maybe the names confused people and led to incorrect changes?

eddyb (Apr 06 2020 at 17:25, on Zulip):

@oli because "marked" just means the old #[structural_match] attribute check

oli (Apr 06 2020 at 17:25, on Zulip):

yea :confused:

eddyb (Apr 06 2020 at 17:25, on Zulip):

it means it's "transparent"

pnkfelix (Apr 06 2020 at 17:26, on Zulip):

pnkfelix said:

does anything else manually implement it them?

ah PhantomData does, I think

oli (Apr 06 2020 at 17:26, on Zulip):

if the derives also put the requirement on its bounds that would work

oli (Apr 06 2020 at 17:26, on Zulip):

that explains why https://github.com/rust-lang/rust/pull/70759 doesn't change anything

pnkfelix (Apr 06 2020 at 17:27, on Zulip):

oli said:

if the derives also put the requirement on its bounds that would work

which requirement? StructuralPartialEq and StructuralEq? I think there was a specific reason we didn't do that

eddyb (Apr 06 2020 at 17:28, on Zulip):

the traits are badly named

oli (Apr 06 2020 at 17:28, on Zulip):

so if we made the derives put bounds on generic parameters and fixed for<'a> fn(&'a T) so it'd implement StructuralPartialEq, then we'd have a recursive type_marked_structural

eddyb (Apr 06 2020 at 17:28, on Zulip):

there's no reason to have bounds on them because they're just an attribute

eddyb (Apr 06 2020 at 17:28, on Zulip):

they can't be an attribute because derives can't modify the original item

eddyb (Apr 06 2020 at 17:28, on Zulip):

(AFAIK)

oli (Apr 06 2020 at 17:28, on Zulip):

they can't

eddyb (Apr 06 2020 at 17:30, on Zulip):

a hypothetical StructuralMatch would have to require AutoDerivedPartialEq + AutoDerivedEq at every level

oli (Apr 06 2020 at 17:30, on Zulip):

But what I'm saying the whole time is that it would be nice to have them be a real trait, because then we could a) move a lot of the logic to libcore instead of doing weird things in the compiler and b) we could add a const method that does the logic that @ecstatic-morse is trying to implement as a Qualif

eddyb (Apr 06 2020 at 17:30, on Zulip):

why add a const method?

oli (Apr 06 2020 at 17:31, on Zulip):

if you have a const fn value_is_structural_match(&self) -> bool then you could make sure that an Option::None is always structural_match

eddyb (Apr 06 2020 at 17:31, on Zulip):

also IIUC the entire thing is value based, but on the HIR?

oli (Apr 06 2020 at 17:31, on Zulip):

for the matching logic

ecstatic-morse (Apr 06 2020 at 17:31, on Zulip):

@oli If you bound on generic parameters, how does const NONE: Option<NonStructuralEqTy> = None get handled.

eddyb (Apr 06 2020 at 17:31, on Zulip):

and the whole point with the qualif stuff is that we move the HIR analysis to MIR

eddyb (Apr 06 2020 at 17:31, on Zulip):

@oli ah I highly disapprove of letting the library configure that

oli (Apr 06 2020 at 17:31, on Zulip):

yes and I want to move it to const eval and having it defined in source instead of in the compiler

oli (Apr 06 2020 at 17:31, on Zulip):

well... it's auto-derived

oli (Apr 06 2020 at 17:31, on Zulip):

you don't write this as a user

oli (Apr 06 2020 at 17:31, on Zulip):

never, that makes no sense

oli (Apr 06 2020 at 17:32, on Zulip):

just like you don't write StructuralEq impls

oli (Apr 06 2020 at 17:32, on Zulip):

ok, as a first step we can change StructuralEq to AutoDerivedEq

eddyb (Apr 06 2020 at 17:32, on Zulip):

it's trivial to make that correct in the compiler - since we already have this analysis for other purposes

eddyb (Apr 06 2020 at 17:32, on Zulip):

but if you're doing CTFE to find out an answer... that's really scary

oli (Apr 06 2020 at 17:33, on Zulip):

I love it

eddyb (Apr 06 2020 at 17:33, on Zulip):

I hate it

oli (Apr 06 2020 at 17:33, on Zulip):

:frown:

eddyb (Apr 06 2020 at 17:33, on Zulip):

especially since a sound analysis is what const generics need

eddyb (Apr 06 2020 at 17:34, on Zulip):

introducing CTFE as an extra step seems wholly unnecessary

eddyb (Apr 06 2020 at 17:34, on Zulip):

the important thing to me is that we start whitelisting primitives and try to remove the pointers from the whitelist

eddyb (Apr 06 2020 at 17:34, on Zulip):

instead of allowing any built-in types by default

oli (Apr 06 2020 at 17:34, on Zulip):

ok

oli (Apr 06 2020 at 17:36, on Zulip):

still, https://github.com/rust-lang/rust/pull/70759 makes sense even if we assume that StructuralEq is called AutoDerivedEq, because we can't autoderive Eq for slice types

eddyb (Apr 06 2020 at 17:36, on Zulip):

slices are built-in

eddyb (Apr 06 2020 at 17:36, on Zulip):

the traits are for literally #[derive(PartialEq, Eq)] written by users

oli (Apr 06 2020 at 17:37, on Zulip):

I was just thinking that it would make more sense to define these things in libcore instead of in rustc

oli (Apr 06 2020 at 17:40, on Zulip):

so you prefer a whitelist in rustc over writing things like impl StructuralEq for u8 (assuming a new trait StructuralEq in contrast to the soon renamed one that is AutoDerivedEq)

eddyb (Apr 06 2020 at 17:48, on Zulip):

specifically I prefer the value check to be in the compiler instead of a const fn method in a trait

eddyb (Apr 06 2020 at 18:00, on Zulip):

so IMO priorities:

  1. rename the traits
  2. add a whitelist for builtin/primitive types instead of "everything goes"
  3. finish the move of the value-oriented analysis to MIR const-qualif
eddyb (Apr 06 2020 at 18:10, on Zulip):

^ @pnkfelix I put my opinion on how to make progress on this in here

eddyb (Apr 06 2020 at 18:11, on Zulip):

/me needs to grab a snack, low blood sugar after getting dragged into all of this

eddyb (Apr 06 2020 at 18:11, on Zulip):

@pnkfelix also, in case you missed it, the scary bit for const generics (and kind of pattern-matching too) is that we can't trust pointers

eddyb (Apr 06 2020 at 18:12, on Zulip):

my example was https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=bd3fca6a9c204a1579448e465461f28b and how it changes between debug and release mode

pnkfelix (Apr 06 2020 at 21:41, on Zulip):

hmm oh dear

pnkfelix (Apr 06 2020 at 21:42, on Zulip):

does this imply we should simply issue a breaking change and disallow matching on fn pointers (and other pointer types in general)? Or is this a compiler bug we might expect to be able to fix?

pnkfelix (Apr 06 2020 at 21:43, on Zulip):

/me is reminded of the debates in the Scheme community about the behavior of eq?/eqv?/equal? on procedure values.

eddyb (Apr 06 2020 at 21:43, on Zulip):

@pnkfelix the former but we should measure the impact before we think too much about it. I guess I should try to induce this breaking change and we can do a crater check-only run?

eddyb (Apr 06 2020 at 21:43, on Zulip):

not 100% sure I can cleanly do it but I can try

pnkfelix (Apr 06 2020 at 21:43, on Zulip):

Certainly there are test cases in the compiler itself that were relying on being able to match fn pointers

pnkfelix (Apr 06 2020 at 21:44, on Zulip):

that were based on bugs filed when we broke it

pnkfelix (Apr 06 2020 at 21:44, on Zulip):

I wasn't too surprised to see @oli note that this could break in the cross-crate case, but I am a little surprised to see it break here

eddyb (Apr 06 2020 at 21:45, on Zulip):

it's due to deduplication instead of duplication

pnkfelix (Apr 06 2020 at 21:45, on Zulip):

though I shouldn't be, given my experience with compilers doing stuff with duplicating and de-duplicating function definitions

eddyb (Apr 06 2020 at 21:45, on Zulip):

duplication is hard to test because you have to somehow get the two constants into the same match

pnkfelix (Apr 06 2020 at 21:45, on Zulip):

yeah I figured it was something like that

eddyb (Apr 06 2020 at 21:45, on Zulip):

but we have mergefunc now

eddyb (Apr 06 2020 at 21:45, on Zulip):

so deduplication is trivial to trigger :P

eddyb (Apr 06 2020 at 21:45, on Zulip):

(in fact it makes using godbolt a pain at times because identical functions are collapsed and godbolt doesn't seem to show aliases by default...)

pnkfelix (Apr 06 2020 at 21:45, on Zulip):

on the other hand, maybe we can just go with "its an underspecified corner of the language"

pnkfelix (Apr 06 2020 at 21:46, on Zulip):

i.e. presumably two procedures with different behavior would have to have different pointer values

eddyb (Apr 06 2020 at 21:46, on Zulip):

the match behavior may be sound, assuming it doesn't get optimized weirdly, but I'd rather not const generics end up allowing these

pnkfelix (Apr 06 2020 at 21:46, on Zulip):

and therefore the deduplication case is only going to fire for procedures with identical behavior

pnkfelix (Apr 06 2020 at 21:47, on Zulip):

but yeah, I tend to agree with you: Better to force people to change their code

eddyb (Apr 06 2020 at 21:47, on Zulip):

probably the best thing to do is to force const generics types to be stricter while still using the "structural match" information for user ADTs

eddyb (Apr 06 2020 at 21:47, on Zulip):

if we can't change patterns themselves

pnkfelix (Apr 06 2020 at 21:47, on Zulip):

is there already a bug filed regarding this example you just shared with me?

pnkfelix (Apr 06 2020 at 21:48, on Zulip):

if not I'll make one; I want to make sure we keep track of this detail

eddyb (Apr 06 2020 at 21:48, on Zulip):

not sure, I should've done some filing myself, idk what @oli has done

pnkfelix (Apr 06 2020 at 21:48, on Zulip):

its okay, I'll look into it. Thanks for exploring this.

eddyb (Apr 06 2020 at 21:48, on Zulip):

(const generics args that aren't equivalent to a tree of integer values are impossible to represent in symbol mangling AFAIK. and also unification probably becomes a nightmare)

eddyb (Apr 06 2020 at 21:50, on Zulip):

ironically raw pointers to statics are probably the easiest to handle since they can be described nominally without being wrong (due to them having unique address)

eddyb (Apr 06 2020 at 21:50, on Zulip):

but anything else is a minefield

eddyb (Apr 06 2020 at 21:51, on Zulip):

this reminds me, I need to ping the Rust v0 demangling GCC patch again, argh, nobody seems to be replying to it

eddyb (Apr 06 2020 at 21:52, on Zulip):

and at some point I should go ahead and implement full hierarchical const value mangling support (abusing the const_field query is probably the best way to do this heh)

eddyb (Apr 06 2020 at 23:43, on Zulip):

@pnkfelix @oli wait what, fn recur in const_to_pat.rs doesn't handle reference types at all?!

eddyb (Apr 06 2020 at 23:44, on Zulip):

they're not banned, so... what happens?!

eddyb (Apr 06 2020 at 23:44, on Zulip):

a PatKind::Constant of reference type, does it get "lazily unpacked" or something?

eddyb (Apr 06 2020 at 23:46, on Zulip):

anyway I'm happy to find https://github.com/rust-lang/rust/issues/70861#issuecomment-610093666

pnkfelix (Apr 07 2020 at 01:43, on Zulip):

don't we end up calling PartialEq::eq for those cases?

eddyb (Apr 07 2020 at 01:44, on Zulip):

@pnkfelix that seems weird to me, references should be structural...

eddyb (Apr 07 2020 at 01:44, on Zulip):

also we don't seem to call PartialEq::eq for non-structural ADTs, we just error

pnkfelix (Apr 07 2020 at 01:44, on Zulip):

its certainly cases like that that end up calling out to the method. Maybe I'm thinking of &[T] ...

eddyb (Apr 07 2020 at 01:44, on Zulip):

it's all very obtuse to navigate

eddyb (Apr 07 2020 at 01:45, on Zulip):

anyway I've opened https://github.com/rust-lang/rust/pull/70872

eddyb (Apr 07 2020 at 01:45, on Zulip):

now I can stop thinking about this for at least two-three days :P

eddyb (Apr 07 2020 at 01:45, on Zulip):

(because of crater being slow)

pnkfelix (Apr 07 2020 at 01:46, on Zulip):

Like, this is what motivated #62411

eddyb (Apr 07 2020 at 01:47, on Zulip):

@pnkfelix but there's nothing that does unpacking for something like &(1, 2, 3)

eddyb (Apr 07 2020 at 01:47, on Zulip):

it just stops

eddyb (Apr 07 2020 at 01:47, on Zulip):

and something later sees a leaf pattern with a &... constant in it

eddyb (Apr 07 2020 at 01:48, on Zulip):

I'm not even sure how good the lint is if you add enough &s

eddyb (Apr 07 2020 at 01:48, on Zulip):

only the type check can catch anything not the const value check

pnkfelix (Apr 07 2020 at 01:48, on Zulip):

maybe I'm misunderstanding your questions

pnkfelix (Apr 07 2020 at 01:48, on Zulip):

some of the code in const_to_pat is in charge of the code-gen (in order to implement the desired dynamic semantics)

eddyb (Apr 07 2020 at 01:49, on Zulip):

there is a hole in the Const -> Pat conversion code

pnkfelix (Apr 07 2020 at 01:49, on Zulip):

but, (unfortunately?) some of the code also in const_to_pat is trying to handle the static semantics (rejecting non-structural-match)

eddyb (Apr 07 2020 at 01:49, on Zulip):

of the size of a large & truck

eddyb (Apr 07 2020 at 01:50, on Zulip):

I wonder if I can show the deficiencies at play here with exhaustiveness checks hmm

pnkfelix (Apr 07 2020 at 01:51, on Zulip):

so we've been trying to add the necessary static checks. And I'm trying to understand if you are describing known deficiencies in the static check

eddyb (Apr 07 2020 at 01:51, on Zulip):

(unrelatedly) wow, https://crater.rust-lang.org/ex/pr-70452 has slowed down a lot, I wonder what the rate of jobs per hour looks like (as a graph. but maybe I've seen enough graph to last me the whole year...)

pnkfelix (Apr 07 2020 at 01:51, on Zulip):

or if you are describing deficiencies in the dynamic semantics

eddyb (Apr 07 2020 at 01:51, on Zulip):

@pnkfelix not sure, because the code is just not handling references at all except in the very special case of a reference to an ADT (which isn't turned into a pattern even, it just triggers an error)

eddyb (Apr 07 2020 at 01:51, on Zulip):

all sorts of things can happen as a result

pnkfelix (Apr 07 2020 at 01:52, on Zulip):

well in principle dispatching to PartialEq::eq is "fine" if everyone is structural-match

eddyb (Apr 07 2020 at 01:52, on Zulip):

but how do we check the patterns?

pnkfelix (Apr 07 2020 at 01:52, on Zulip):

this is the problem

eddyb (Apr 07 2020 at 01:53, on Zulip):

like, fully structural patterns. there's no code in the expected place to handle references

eddyb (Apr 07 2020 at 01:53, on Zulip):

so if they work they are either buggy or very limited

eddyb (Apr 07 2020 at 01:53, on Zulip):

or very hacky :P

pnkfelix (Apr 07 2020 at 01:53, on Zulip):

unfortunately I do not think I have time to have this conversation right now

pnkfelix (Apr 07 2020 at 01:53, on Zulip):

I really do wish you could at least use words like "dynamic" and/or "static"

pnkfelix (Apr 07 2020 at 01:53, on Zulip):

so that I could have a better idea about what you are saying is buggy

eddyb (Apr 07 2020 at 01:54, on Zulip):

I don't know which of those are affected

eddyb (Apr 07 2020 at 01:54, on Zulip):

because the code isn't handling references. so it could be both or either

eddyb (Apr 07 2020 at 01:54, on Zulip):

downstream code has to somehow make up for it if anything is to work

pnkfelix (Apr 07 2020 at 01:54, on Zulip):

my belief is that these are known bugs in the desired static semantics

pnkfelix (Apr 07 2020 at 01:55, on Zulip):

that occasional leads to ICE's that arise due to attempts to emit invocations of PartialEq::eq on non-existant impl's.

eddyb (Apr 07 2020 at 01:55, on Zulip):

it's just broken https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=21e1b92935c02e6c6a7ed8f55620eddb

pnkfelix (Apr 07 2020 at 01:55, on Zulip):

(lead to ICE's when trying to encode the dynamic semantics, that is)

eddyb (Apr 07 2020 at 01:55, on Zulip):

ugh

pnkfelix (Apr 07 2020 at 01:55, on Zulip):

the exhaustiveness checking is another kettle of fish entirelyh

eddyb (Apr 07 2020 at 01:56, on Zulip):

maybe I should point out that non-structural examples don't matter, they're just blanket-caught by the type-based check

pnkfelix (Apr 07 2020 at 01:56, on Zulip):

which I guess you did reference up above, but I did not think that was your focus here.

eddyb (Apr 07 2020 at 01:56, on Zulip):

which you might be meaning by "static" checks

eddyb (Apr 07 2020 at 01:56, on Zulip):

I mean, it matters maybe in terms of precision of diagnostics, but they'd be caught anyway

eddyb (Apr 07 2020 at 01:56, on Zulip):

the problem is the code isn't transparent wrt references. it's like they're raw pointers

pnkfelix (Apr 07 2020 at 01:57, on Zulip):

had you read the examples on #62411 ?

pnkfelix (Apr 07 2020 at 01:57, on Zulip):

such as https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=b9aeae13dc490474e1e45f294ac31e5e ?

eddyb (Apr 07 2020 at 01:57, on Zulip):

I'm talking about structural things only

eddyb (Apr 07 2020 at 01:58, on Zulip):

I'm tired too, I should've been clearer from the start

pnkfelix (Apr 07 2020 at 01:58, on Zulip):

so you're talking about cases where everything does implement StructuralPartialEq ?

eddyb (Apr 07 2020 at 01:58, on Zulip):

yes

pnkfelix (Apr 07 2020 at 01:58, on Zulip):

that was not clear to me. I was not aware of any bugs there

eddyb (Apr 07 2020 at 01:58, on Zulip):

for those cases, references aren't handled the same way tuples or arrays are

pnkfelix (Apr 07 2020 at 01:58, on Zulip):

right, but the whole point is that they don't need to be?

eddyb (Apr 07 2020 at 01:59, on Zulip):

but why do tuples and arrays?

pnkfelix (Apr 07 2020 at 01:59, on Zulip):

since dispatching to PartialEq::eq will work for them?

eddyb (Apr 07 2020 at 01:59, on Zulip):

like, what's the difference?

pnkfelix (Apr 07 2020 at 01:59, on Zulip):

heh, are you asking why the compiler has ended up in this state?

eddyb (Apr 07 2020 at 01:59, on Zulip):

& is no more than a constructor in patterns

eddyb (Apr 07 2020 at 01:59, on Zulip):

the unequal treatment looks very suspicious

pnkfelix (Apr 07 2020 at 02:00, on Zulip):

I'm just trying to say that the observable dynamic semantics should be the same

pnkfelix (Apr 07 2020 at 02:00, on Zulip):

for cases where the whole recursive structure is structural-match

eddyb (Apr 07 2020 at 02:00, on Zulip):

so @oli was suggesting earlier today that we should use == for non-structural cases

eddyb (Apr 07 2020 at 02:00, on Zulip):

and for some reason I expected we're doing that already because I thought I had seen something about ==

eddyb (Apr 07 2020 at 02:01, on Zulip):

somehow I blanked out on the fact that we use == for structural cases and treat them as opaque

pnkfelix (Apr 07 2020 at 02:01, on Zulip):

yeah years ago niko and I talked about just throwing in the towel and doing that

pnkfelix (Apr 07 2020 at 02:01, on Zulip):

(just using ==, that is)

eddyb (Apr 07 2020 at 02:01, on Zulip):

which begs the natural question: why do this ever for structural cases, where you can always decompose a known value?

pnkfelix (Apr 07 2020 at 02:02, on Zulip):

but there was a fear that this would be confusing, since manually inlining the RHS of the const-item into a pattern could then have a different semantics.

pnkfelix (Apr 07 2020 at 02:02, on Zulip):

are you asking "why ever dispatch to PartialEq::eq ?

eddyb (Apr 07 2020 at 02:02, on Zulip):

pretty much, I guess

pnkfelix (Apr 07 2020 at 02:02, on Zulip):

well, you'd probably have to do some archealogy to answer that question

eddyb (Apr 07 2020 at 02:02, on Zulip):

it seems unnecessary unless you can't evaluate the constant

pnkfelix (Apr 07 2020 at 02:03, on Zulip):

maybe someone was worried about code size blow up

pnkfelix (Apr 07 2020 at 02:03, on Zulip):

i.e. cheaper to store one copy of the const value in static storage and dispatch to PartialEq::eq rather than inline the pattern matching code for it at each pattern?

pnkfelix (Apr 07 2020 at 02:03, on Zulip):

but now I'm just guessing

eddyb (Apr 07 2020 at 02:03, on Zulip):

that feels like it should be a mode or something

pnkfelix (Apr 07 2020 at 02:04, on Zulip):

anyway I really do have to go. I'm tired too and its not even 2am where I am.

pnkfelix (Apr 07 2020 at 02:04, on Zulip):

or wait, 4 am? Yikes

eddyb (Apr 07 2020 at 02:04, on Zulip):

where MIR matching building keeps consts opaque

eddyb (Apr 07 2020 at 02:04, on Zulip):

(it's 5am here)

eddyb (Apr 07 2020 at 02:04, on Zulip):

while match checking fully expands consts

eddyb (Apr 07 2020 at 02:04, on Zulip):

the way it's setup right now everyone gets the worst of both worlds

eddyb (Apr 07 2020 at 02:06, on Zulip):

@pnkfelix I've found so many bugs recently just by looking for places that handled Ty but not ty::Const (showing up in a type/predicate/etc.), so I guess that also makes me more suspicious than usual of asymmetries

eddyb (Apr 07 2020 at 02:06, on Zulip):

but also I vaguely recall @oli asking a question about eagerly expanding const patterns, that might be related to what I'm staring at

eddyb (Apr 07 2020 at 02:07, on Zulip):

whatever, we can sort it out after the crater run. I have enough hacks in it that should gracefully handle most legitimate cases and is only slightly conservative

eddyb (Apr 07 2020 at 02:08, on Zulip):

because exhaustiveness checking doesn't seem to be able to see behind references in general, I suspect most people don't use patterns like that anyway

eddyb (Apr 07 2020 at 02:16, on Zulip):

hopefully we'll need to care less if we start using a value-based analysis on the MIR - even unevaluated consts could be considered fully structural

eddyb (Apr 07 2020 at 02:16, on Zulip):

(like an associated const NONE: Option<T> = None;)

ecstatic-morse (Apr 07 2020 at 02:52, on Zulip):

#65466 seems relevant to this discussion. In it, we eagerly emit a call to <[O<B>] as PartialEq>::eq despite the fact that PartialEq is not satisfied when building the MIR for main. This issue is somewhat orthogonal to using dataflow to check for structural match violations, since O::None should always be valid for structural matching.

#[derive(PartialEq, Eq)]
enum O<T> {
    Some(*const T), // Can also use PhantomData<T>
    None,
}

struct B;

const C: &[O<B>] = &[O::None];

fn main() {
    let x = O::None;
    match &[x][..] {
        C => (),
        _ => (),
    }
}
ecstatic-morse (Apr 07 2020 at 02:57, on Zulip):

#67088, the expr walking structural match PR, fixed this by rejecting the code entirely. This is fine as a stopgap measure, but is that what we want in the long-term? I suppose rejecting this is necessary to preserve the ability to choose between semantic and structural equality.

eddyb (Apr 07 2020 at 09:52, on Zulip):

@ecstatic-morse fascinating, so even for MIR match lowering (not sure what to call that part of MIR building tbh), we must descend through constants of types that do not implement PartialEq

eddyb (Apr 07 2020 at 09:53, on Zulip):

and we can't just allow unevaluated generic consts on the sole basis on "const-qualif vetted them"

eddyb (Apr 07 2020 at 09:53, on Zulip):

e.g. None::<String> is fine, None::<Uncomparable> is not

eddyb (Apr 07 2020 at 09:56, on Zulip):

which is kind of sad but impossible to reconcidence without us building patterns from promotable-like MIR or something, which still limitates you compared to non-generic constants

eddyb (Apr 07 2020 at 15:05, on Zulip):

@pnkfelix good news for people using raw/fn pointers in patterns: due to #70889, we might need to do a completely separate check for const generics anyway

eddyb (Apr 07 2020 at 15:05, on Zulip):

so while we could lint it or whatever, it might be as harmless as using == shrug

eddyb (Apr 07 2020 at 15:08, on Zulip):

@pnkfelix I am not happy to keep finding aspects of Rust or parts of the compiler that break const generics in one way or another, but it's probably for the better to bite the bullet and do in-depth "constant in type" WF checks

eddyb (Apr 07 2020 at 15:09, on Zulip):

ofc the usual WF-deficient culprits (type aliases and default types, AFAIK) might cause problems here, idk what to do about them

eddyb (Apr 07 2020 at 15:09, on Zulip):

other than "hopefully it gets caught at use site

pnkfelix (Apr 07 2020 at 15:09, on Zulip):

eddyb said:

so while we could lint it or whatever, it might be as harmless as using == shrug

I am not sure what "harmless" is meant to mean here.

eddyb (Apr 07 2020 at 15:09, on Zulip):

it's not like the raw/fn pointers are enum discriminants

eddyb (Apr 07 2020 at 15:10, on Zulip):

I don't think they can break the soundness of pattern-matching

eddyb (Apr 07 2020 at 15:10, on Zulip):

they might just be surprising, that's all

pnkfelix (Apr 07 2020 at 15:10, on Zulip):

oh, okay. Yes, to my knowledge this has never been a soundness concern

eddyb (Apr 07 2020 at 15:10, on Zulip):

(same as with using ==)

pnkfelix (Apr 07 2020 at 15:10, on Zulip):

more a question of, as you said, violating a principle of least surprise

eddyb (Apr 07 2020 at 15:11, on Zulip):

my motivation and why I was so scared was const generics, but even there I think it would just ICE or cause link errors

pnkfelix (Apr 07 2020 at 15:11, on Zulip):

and also a dash of "we don't want to commit to structural nor semantic match by accident"

eddyb (Apr 07 2020 at 15:12, on Zulip):

or like #70889 shows, cause two equal things to appear unequal

pnkfelix (Apr 07 2020 at 15:12, on Zulip):

yes okay. Now I think I am grokking why you were getting so worked up about this last night.

eddyb (Apr 07 2020 at 15:12, on Zulip):

I was hoping I could break pattern-matching in a way which would show why letting non-integral values in was scary but I wasn't able to

eddyb (Apr 07 2020 at 15:15, on Zulip):

and I guess from there on I was a walking talking ongoing failure to communicate

pnkfelix (Apr 07 2020 at 15:17, on Zulip):

welcome to my life

eddyb (Apr 07 2020 at 15:18, on Zulip):

I'm really sorry, I'm getting flashbacks of the associated type soundness bug period which lasted a year or two IIRC, and seeing similar things happening around const generics, and getting emotional about it. I really shouldn't

eddyb (Apr 07 2020 at 15:20, on Zulip):

at least const generics won't be stable any time soon so I should relax

ecstatic-morse (Apr 07 2020 at 19:30, on Zulip):

#67343 is ready to review. It still needs a test for cross-crate constants as match patterns, however.

eddyb (Apr 07 2020 at 19:42, on Zulip):

"auxiliary" is the keyword for multi-crate tests

eddyb (Apr 07 2020 at 19:46, on Zulip):

I love how simple the const-qualif impl of that is <3

Last update: May 29 2020 at 18:05UTC