Repository navigation
function pointers as match patterns have optimization-dependent behavior #70861
Description
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
Reacted by scottmcm
Reacted by Aaron Hill and Stanislav Tkach
Activity
pnkfelix commentedon Apr 6, 2020
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 commentedon Apr 6, 2020
The scary thing to me is this same thing happening with
constgenerics, 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 commentedon Apr 6, 2020
Looks like this is where the whitelisting is missing:
rust/src/librustc_mir_build/hair/pattern/const_to_pat.rs
Line 260 in 6dee5f1
eddyb commentedon Apr 7, 2020
Very disconcertingly, it doesn't look like we turn
&Tconstants 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 commentedon Apr 7, 2020
Not just that, I have an implementation of it! #70743
eddyb commentedon Apr 7, 2020
Ah, thanks, I vaguely remembered some discussion, I should've checked your PRs first, sorry!
hanna-kruppe commentedon Apr 7, 2020
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 commentedon Apr 7, 2020
Yesterday I wrote (#70861 (comment)):
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 commentedon Apr 8, 2020
I think this is basically a duplicate of #54685, caused by
unnamed_addr.matchwith 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 commentedon Apr 28, 2020
Separate tracking may indeed be confusing, but I just want to note that I do not consider
matchwith 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 wherePartialEqhas 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.
RalfJung commentedon Jan 9, 2024
This code causes forward-compat lints now, so once pointer_structural_match is a hard error this can be closed. (Cc #62411)
Auto merge of rust-lang#124661 - RalfJung:only-structural-consts-in-p…
Auto merge of rust-lang#124661 - RalfJung:only-structural-consts-in-p…
Auto merge of #124661 - RalfJung:only-structural-consts-in-patterns, …
Auto merge of #124661 - RalfJung:only-structural-consts-in-patterns, …
Auto merge of #124661 - RalfJung:only-structural-consts-in-patterns, …
Auto merge of #124661 - RalfJung:only-structural-consts-in-patterns, …