Distinguish fn item types to allow reification from nothing to fn pointers.

[eddyb]

Type-checking and translation of zero-sized function item types (fixes #19925).

The first commit is a rebase of #26284, except for files that have moved since.

This is a [breaking-change], due to:

• each FFI function has a distinct type, like all other functions currently do
• all generic parameters on functions are recorded in their item types, e.g.:
  size_of::<u8> & size_of::<i8>'s types differ despite their identical signature.
• function items are zero-sized, which will stop transmutes from working on them

The first two cases are handled in most cases with the new coerce-unify logic,
which will combine incompatible function item types into function pointers,
at the outer-most level of if-else chains, match arms and array literals.

The last case is specially handled during type-checking such that transmutes
from a function item type to a pointer or integer type will continue to work for
another release cycle, but are being linted against. To get rid of warnings and
ensure your code will continue to compile, cast to a pointer before transmuting.

[rust-highfive]

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

[nikomatsakis]

r? @nikomatsakis

[nikomatsakis]

Triggering a crater run.

[nikomatsakis]

Crater run results: https://gist.github.com/nikomatsakis/0902ea970301a841231e

One regression (fann).

[bors]

☔️ The latest upstream changes (presumably #31394) made this pull request unmergeable. Please resolve the merge conflicts.

[eddyb]

The regression should be handled by the unify-coerce logic, however I wish I could enable the full extent of it without triggering LLVM asserts 😞.

[eddyb]

Turns out that using fcx.resolve_type_vars_if_possible(...) on both types in coercion::try_unify solves the problem.
That is worrying, because demand::coerce does this and it appears to be the reason normalizations happen at all.

The problem in question starts with &[1, 2, 3][..] being lowered to Index::index(&[1, 2, 3] as &[i32], ..: $Idx): &$Output with an obligation of <&[i32] as Index<$Idx>>::Output == $Output.
Immediately afterwards, $Idx is unified to RangeFull. This is unchanged by my PR.
This specific sequence of events means $Output is an inference variable and not a projection or [i32].

My code was mistakenly assigning Vec<i32> to $Output.
That should error. Instead, the obligation gets dropped or otherwise ignored.

What usually happens is demand::coerce (called from anywhere in typeck) calls resolve_type_vars_if_possible which resolves $Output and nothing has time to assign $Output something incorrect.

I've come up with this testcase, which should trigger the same sequence of events, but does not:

fn main() {
    let mut x = unsafe {std::mem::zeroed()};
    let mut y = &[1, 2, 3][x];
    y = &vec![];
    x = ..;
}

It errors with "type mismatch resolving <[_] as Index<RangeFull>>::Output == Vec<_>" before and after this PR.

cc @rust-lang/compiler Even though my PR appears to work now, I dread leaving an obligation-dropping bug around.

[eddyb]

Found the culprit: it's this resolve_type_vars_if_possible call, which ironically I added a long time ago (although I'm not sure how the fulfillment context worked at that point, if it even existed).

        let resolved_t = match unresolved_type_action {
            UnresolvedTypeAction::Error => {
                structurally_resolved_type(fcx, sp, t)
            }
            UnresolvedTypeAction::Ignore => {
                // We can continue even when the type cannot be resolved
                // (i.e. it is an inference variable) because `Ty::builtin_deref`
                // and `try_overloaded_deref` both simply return `None`
                // in such a case without producing spurious errors.
                fcx.resolve_type_vars_if_possible(t)
            }
        };

UnresolvedTypeAction::Ignore is only used by the coercion code, which is incidentally (AFAICT) the sole caller of autoderef from within a transaction (commit_if_ok).

fcx.resolve_type_vars_if_possible, unlike its infcx counterpart, also tries to select obligations.
Obligations selected inside a transaction have their type bindings lost if the transaction is rolled back.

Usually that's not an issue because a failed coercion is fatal, so at worst you get an ICE after a legitimate error.

But I had code which tried coercions and continued in a different manner, resulting in projection obligations successfully selected but then their effects completely rolled back and thus &[1, 2, 3][..]: &Vec<i32> managed to sneak in undetected.

The fix is very simple: replace that line with fcx.infcx().resolve_type_vars_if_possible(&t).

However, I would like a permanent solution, that doesn't allow access to fulfillment_cx unless there is no transaction going on. Sticking fulfillment_cx in infcx might make this cleaner, but atm there's no easy/efficient way of determining whether you're already in a transaction.

[eddyb]

@nikomatsakis Tests appear to pass, can I get another crater run? If we see any regressions caused by the scheme I chose, we'll have to tone it down, perhaps limit it to two function item types (although I really like this scheme, especially now that I've plucked the bugs out and it seems to "just work").

[nikomatsakis]

OK, I've reviewed as far as ea2510c (though I'm still reading through d619ebe, which seems to be the "meaty" commit) -- it all seems quite nice thus far.

[nikomatsakis]

@eddyb the crater run seems to have results now, maybe it just wasn't done before:

https://gist.github.com/nikomatsakis/54fbfe9a0d09b8f51cc6

• 4148 crates tested: 2469 working / 1389 broken / 235 regressed / 1 fixed / 54 unknown.
• There are 27 root regressions
• There are 235 regressions

[nikomatsakis]

@eddyb so we were talking about this in the @rust-lang/lang meeting -- if we want to abide by our usual policies, we may want to think about a compatibility mode for transmute where we permit transmutes from a (zero-sized) fn to a ptr but issue a "forward compatibility" lint warning about phasing this change out.

I wish there was something more ergonomic than transmute(foo as *mut _) in any case, which seems rather awkward.

[eddyb]

@bors r=nikomatsakis

[bors]

📌 Commit 3855fa9 has been approved by nikomatsakis

[bors]

⌛️ Testing commit 3855fa9 with merge 3dc9398...

[bors]

⛄️ The build was interrupted to prioritize another pull request.

[bors]

⌛️ Testing commit 3855fa9 with merge bcda58f...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

[SSheldon]

FYI, special casing the transmute from functions did not prevent all regressions. I had some code that started segfaulting in 1.9.0, fixed by SSheldon/rust-objc-foundation@f918819. Basically, it boils down to looking like this:

#[link(name = "objc", kind = "dylib")]
extern {
    fn objc_msgSend();
}

unsafe fn msg_send<T>(t: T) {
    let f: unsafe extern fn() = objc_msgSend;
    let f: unsafe extern fn(T) = ::std::mem::transmute(f);
    f(t)
}

fn hello() {
    println!("hello");
}

fn main() {
    unsafe {
        msg_send(hello);
    }
}

That's a case where what used to be a function pointer silently becomes a 0-sized type.

Anyways, fixed now, hopefully no one else is writing code like this! But figured I'd make it known in case anyone else runs into something similar.

[eddyb]

@SSheldon Interesting example. While there is transmute in there, it has to do with passing generic arguments to C functions. A similar situation arose with C variadics in FFI, see #32201.
You could also notice this problem if you were doing (*(&rust_function as *const fn()))().