function pointers as match patterns have optimization-dependent behavior

[pnkfelix]

I tried this code, supplied by @eddyb play:

#![feature(test)]
use std::hint::black_box;

fn foo() {}
const FOO: fn() = foo;

fn bar() {}
const BAR: fn() = bar;

#[inline(never)]
fn print(s: &str) {
    println!("{}", s);
}

fn main() {
    match black_box(FOO) {
        FOO => print("FOO"),
        BAR => print("BAR"),
        _ => unreachable!(),
    }
    
    match black_box(FOO) {
        BAR => print("BAR"),
        FOO => print("FOO"),
        _ => unreachable!(),
    }
    
    match black_box(BAR) {
        FOO => print("FOO"),
        BAR => print("BAR"),
        _ => unreachable!(),
    }
    
    match black_box(BAR) {
        BAR => print("BAR"),
        FOO => print("FOO"),
        _ => unreachable!(),
    }
}

I expected to see this happen: print out of

FOO
FOO
BAR
BAR

(and in debug builds, that is what happens).

Instead, this happened: in release builds, it prints:

FOO
BAR
FOO
BAR

Meta

This was on a playpen running 1.44.0-nightly, 2020-04-05.

This all arose from a conversation with @eddyb regarding structural match; you can read it on the zulip archive

cc #31434

[pnkfelix]

It is not clear to me what the best thing to do here is. @eddyb would like to disallow matching against function pointer constants entirely. I am thinking it might be better to issue a lint, since I know from experience some people simply are doing this, for better or for worse, and I suspect that its not a soundness issue, just a potential gotcha.

Also, for you archeaologists out there, here is a classic rrrs-authors thread on this subject.

Update: and yes, in case you were wondering, the scheme specifiers are still debating this matter nearly 30 years later: http://scheme-reports.org/mail/scheme-reports/msg03616.html

[eddyb]

The scary thing to me is this same thing happening with const generics, because they use the same "structural match" requirement.
(but there's no "runtime value" there, so it's more nebulous in a different way I suppose)

[eddyb]

Looks like this is where the whitelisting is missing:

rust/src/librustc_mir_build/hair/pattern/const_to_pat.rs

Line 260 in 6dee5f1

_ => PatKind::Constant { value: cv },

[eddyb]

Very disconcertingly, it doesn't look like we turn &T constants into reference patterns, which means we can't fully correctly check leaves behind them (and I'm scared to look at how anything works with them, are they lazily expanded?).

Fixing that should probably be a priority, I wonder if @oli-obk already has plans for this.

[oli-obk]

Not just that, I have an implementation of it! #70743

[eddyb]

Ah, thanks, I vaguely remembered some discussion, I should've checked your PRs first, sorry!

[hanna-kruppe]

I believe the non-deterministic comparison of function pointers is already tracked by #54685, though the question of what to do about it w.r.t. patterns seems novel and worth tracking independently.

[eddyb]

Yesterday I wrote (#70861 (comment)):

The scary thing to me is this same thing happening with const generics, because they use the same "structural match" requirement.
(but there's no "runtime value" there, so it's more nebulous in a different way I suppose)

But today I filed #70889, due to which we might need to do a completely separate check for const generics, so we don't have to be as strict on pattern match-ing anyway.

[RalfJung]

I think this is basically a duplicate of #54685, caused by unnamed_addr.

though the question of what to do about it w.r.t. patterns seems novel and worth tracking independently.

match with a constant and == are just two different ways to write a comparison. This definitely should be mentioned in the other issue but I feel separate tracking is confusing.

[pnkfelix]

I think this is basically a duplicate of #54685, caused by unnamed_addr.

though the question of what to do about it w.r.t. patterns seems novel and worth tracking independently.

match with a constant and == are just two different ways to write a comparison. This definitely should be mentioned in the other issue but I feel separate tracking is confusing.

Separate tracking may indeed be confusing, but I just want to note that I do not consider match with a constant to be equivalent to using ==.

In particular, the point of rust-lang/rfcs#1445 is that the compiler has not historically treated such matches as being equivalent to == (namely in cases where PartialEq has not been implemented via #[derive(..)]), and we did not want to commit at that time to choosing between so-called "structural" vs "semantic" equality.

Having said that, certainly this is related to #54685, and it may indeed be a good idea to merge these two tickets. But I want to make sure we do it for the right reason.