create a lint for tracking "tainted" fallback around abrupt expressions and ! type

[nikomatsakis]

As part of #65992, we want to create a lint that warns about cases where changing the fallback for type variables from () to ! will create problems. This issue is tracking that implementation work.

Edit 2020-10-05: stream for work on this, https://rust-lang.zulipchat.com/#narrow/stream/259160-t-lang.2Fproject-never-type

Current status and next steps

• prototype lint and verify that it works on the example test cases
• turn the lint to deny by default and make sure libstd builds
• create a helper function for adding to dead_nodes that explains what is going on, document that this function is invoked on the "way up" the tree, so that nodes are added to dead_nodes set if they diverge before completing (or they never start)
• test against the objc crate version that had problems
• run a "crater check" run and see the results
• improve the error message

[nikomatsakis]

We are discussing this on Zulip.

[Centril]

Removing C-future-compatibility as this is not ever meant to become a hard error.

[nikomatsakis]

Collecting examples.

This is the canonical example where we ran into trouble:

fn unconstrained_return<T>() -> Result<T, String> {
    let ffi: fn() -> T = transmute(some_pointer);
    Ok(ffi())
}
fn foo() {
    match unconstrained_return::<_>() {
        Ok(x) => x,  // `x` has type `_`, which is unconstrained
        Err(s) => panic!(s),  // … except for unifying with the type of `panic!()`
        // so that both `match` arms have the same type.
        // Therefore `_` resolves to `!` and we "return" an `Ok(!)` value.
    };
}

[nikomatsakis]

Another example (playground), this one uses if/else and not a local variable:

#![feature(never_type_fallback)]

fn make_unit() {
}

fn unconstrained_return<T>() ->T {
    unsafe {
        let make_unit_fn: fn() = make_unit;
        let ffi: fn() -> T = std::mem::transmute(make_unit_fn);
        ffi()
    }
}

fn main() {
    let _ = if true {
      unconstrained_return()
    } else {
      panic!()
    };
}

[nikomatsakis]

We have a branch on my repository that identifies type variables that got their value as a result of diverging fallback (and hence will change from () to ! when the feature gate switches).

We are currently issuing way too many warnings.

What we need to do is to narrow down the warnings to problematic cases.

The ideal is probably "if the type of any live expression E is some variable V where V got its value from diverging fallback, issue a warning" -- this should never happen, as it would either be compilation error, UB, or an unexpected panic.

[nikomatsakis]

So, picking this up again, and echoing a comment from Zulip:

In my previous comment, I linked to a branch. The key idea there was to (a) identify type variables that that got their value as a result of diverging fallback and then (b) warn when we find those type variables in "certain contexts". The challenge is narrowing down that notion of "certain contexts".

The current branch was just warning whenever the type of any expression is changed as a result of "diverging fallback". That's clearly not correct. I think what we conceptually want is to warn whenever the type of a live expression is changed as the result of diverging fallback. That raises, of course, the question of how to determine where the "live" expressions are.

What I had in mind was to build on the way that the type checker is currently detecting live vs dead expressions. In particular, we have this flag on the FnCtxt stored in the diverges field.

As the comments note:

In general, when entering an expression or other node in the tree, the initial value indicates whether prior parts of the containing expression may have diverged.

So, what I was thinking is that we can store the hir-ids for expressions where the flag was set to WarnedAlways on entry as "dead nodes". We can then issue a warning if (a) the type of some hir-id H contains a type variable whose value was set by diverging fallback and (b) that expression is not in the set of dead-nodes (i.e., it is maybe live). This set of dead-nodes, it seems to me, is likely to be much smaller than the set of live nodes, so it makes sense to track that.

This is perhaps too coarse, though, we may need to do something tighter, like look for live nodes whose type not only contains a diverging-fallback variable, but actually is a diverging-fallback variable. This would still cover the known bad examples I gave previously, since in those cases the type of the value returned by unconstrained_return() would be live in all cases and has the type !. Still, it's clear that one could construct a case that would not be covered, though it takes more effort and I don't know that we've seen such a case in the wild.

@Aaron1011 it'd be useful to log in this issue some condensed "false warning" patterns that we wish to avoid!

Some examples that come to mind.

Example Result-Return. This should not lint:

fn unconstrained_err<T>() -> Result<u32, T> { Ok(22) }

fn main() {
    let _ = if true {
      unconstrained_err()
    } else {
      Err(panic!())
    };
}

However, in this case I believe that the type of T will be a "diverging fallback" to ! -- because the result of Err(panic!) will be Result<_, ?X> where ?X is a diverging fallback variable, and that will unify with the type of the true branch, which is Result<u32, ?Y>.

Note that you could make this behave badly if unconstrained_err should somehow assume (via unsafe code) that T is inhabited.

This example shows why we don't want to warn when a live expression has a result that merely includes a fallback variable (here, the result of unconstrained_err is live and ultimately includes a fallback variable, and that's ok).

[nikomatsakis]

This is an example of something where we would NOT lint but it still produces UB:

```rust
// the bet is that an unconstrained return value like `T` here signals data
// that is never used, e.g. this fn could return `Ok(())`, but in this case
// that is not true:
fn unconstrained_return<T>() -> Result<(), T> {
    let ffi: fn() -> T = transmute(some_pointer);
    ffi(); // produces a value of type T
}

fn foo() {
    match unconstrained_return::<_>() { // _ is ?X
        Ok(_) => Ok(()), // type of `Ok` is `Result<(), ?A>`
        Err(s) => Err(panic!(s)), // type of `Err(_)` is `Result<?B, ?V>` where `?V` is diverging
    }; // type of expression is `Result<(), ?V>`
}

However, as far as I know we've never observed a pattern like this in the wild, but we have observed many examples where the warning here would be a false warning because the fn just never returned an Err variant (most of these were the variant constructors themselves).

[nikomatsakis]

So @blitzerr and I spent some time explaining the problem and my proposed solution.

Recording available on YouTube

Hackmd

[crlf0710]

I have another idea, which is a larger change itself but i think it is simpler and easier to implement and explain itself:
Introduce a opt-out Inhabited marker trait language item in the same way as Sized, so there is Inhabited, !Inhabited and ?Inhabited. For type variables in generics, they'll be Inhabited by default and only opted-out explicitly.

For the type fallback change part, if the type is constrained to be Inhabited, it should fallback to (), if it's not, it should fallback to !.

---

In this original example:

fn unconstrained_return<T>() -> Result<T, String> {
    let ffi: fn() -> T = transmute(some_pointer);
    Ok(ffi())
}
fn foo() {
    match unconstrained_return::<_>() {
        Ok(x) => x,  // `x` has type `_`, which is constrained to be `Inhabited`
        Err(s) => panic!(s),  // the type of `panic!()` is never type
        // `x` cannot be never type since it is `Inhabited`, so it will fallback to `()`.
        // Therefore `_` resolves to `()` and we "return" an `Ok(())` value.
    };
}

[scottmcm]

Zulip stream: https://rust-lang.zulipchat.com/#narrow/stream/259160-t-lang.2Fproject-never-type

[nikomatsakis]

Hey @SSheldon and @SimonSapin -- so @blitzerr has a lint on a branch that is detecting and issuing a warning for my "standalone test case" that I think represented the harmful pattern in the objc crate. I thought it would be nice to test it on "the real thing" to make sure the test case is representative. Can you point us at a particular revision of the repo or something like that where we could test the lint and see if you would indeed have gotten a warning/error?

[SSheldon]

@nikomatsakis version 0.2.6 and below of objc are the problematic versions! You can reproduce the behavior with a sample like this:

#[macro_use]
extern crate objc;

use std::ptr;
use objc::runtime::Object;

fn main() {
    let obj: *mut Object = ptr::null_mut();
    unsafe {
        msg_send![obj, retain];
    };
}

[nikomatsakis]

Update from last Thursday:

@blitzerr and I did a bit of investigation. We got the lint more-or-less working as I intended. This led us to some interesting "false warnings" and other cases that are worth highlighting.

Example 1:

We found some code in the parser that generated a warning:

rust/compiler/rustc_parse/src/lib.rs

Line 230 in 4051473

parser.unexpected()?;

Specifically what happens here is that unexpected always returns an Err, but rustc doesn't know that:

rust/compiler/rustc_parse/src/parser/mod.rs

Lines 368 to 375 in 4051473

	pub fn unexpected<T>(&mut self) -> PResult<'a, T> {
	match self.expect_one_of(&[], &[]) {
	Err(e) => Err(e),
	// We can get `Ok(true)` from `recover_closing_delimiter`
	// which is called in `expected_one_of_not_found`.
	Ok(_) => FatalError.raise(),
	}
	}

Because of some quirks of how ? is desugared, the result of unexpected()? winds up being unified with a "fallback-to-never" variable, and so the type of unexpected()?; falls back to the never type but the code is not known to be dead. Therefore, the lint issues a warning. In reality, the code is dead, so the warning is a false warning.

This was a known pattern that we encountered in the past but weren't sure how to fix. Note that if you didn't have the ? operator, then unexpected(); on its own would not compile, because the result type T is unconstrained. So I tend to think a lint here is not a problem. To silence it, one could write unexpected::<()>? or something like that.

Example 2:

This case is a bit stranger:

rust/compiler/rustc_middle/src/util/bug.rs

Lines 29 to 36 in 4051473

	tls::with_opt(move |tcx| {
	let msg = format!("{}: {}", location, args);
	match (tcx, span) {
	(Some(tcx), Some(span)) => tcx.sess.diagnostic().span_bug(span, &msg),
	(Some(tcx), None) => tcx.sess.diagnostic().bug(&msg),
	(None, _) => panic!(msg),
	}
	});

What happens here is that the closure gets the return type -> ! and the with_opt function returns that same T (which falls back to never), but we again don't realize that since the with_opt function returns never it must be dead.

Summary

Both of these false warnings seem unfortunate and potentially widespread. Next step might be to do a crater run. I am also debating if there is a way to silence these warnings while still warning about the problematic cases but I'm not entirely sure. Their unifying characteristic is perhaps that the functions are returning ! directly, rather than synthesizing (e.g.) a fn() -> !.

[pnkfelix]

Visiting during T-compiler backlog bonanza.

Much like #35121, this seems like it has design-concerns

#65992 was closed as something we are not sure we want to do. I'm pretty sure that with #65992 closed, it makes sense to close this as well. We can always reopen it if we realize that it is something we actually want.

@rustbot label: S-tracking-design-concerns