Add lint for never type regressions #68350
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
Add lint for never type regressions Fixes #67225 Tl;DR: This PR introduces a lint that detects the 'bad' never-type fallback in `objc` (which results in U.B.), while allowing safe fallback to occur without a warning. See https://hackmd.io/@FohtAO04T92wF-8_TVATYQ/SJ0vcjyWL for some background on never-type fallback. ### The problem We want to reject "bad" code like this: ```rust 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. }; } ``` in which enabling never-type fallback can cause undefined behavior. However, we want to allow "good" code like this: ```rust struct E; impl From<!> for E { fn from(x: !) -> E { x } } fn foo(never: !) { <E as From<_>>::from(never); } fn main() {} ``` which relies on never-type fallback being enabled, but is perfectly safe. ### The solution The key difference between these two examples lies in how the result of never-type fallback is used. In the first example, we end up inferring the generic parameter of `unconstrained_return` to be `!`. In the second example, we still infer a generic parameter to be `!` (`Box::<!>::new(!)`), but we also pass an uninhabited parameter to the function. Another way of looking at this is that the call to `unconstrained_return` is **potentially live* - none of its arguments are uninhabited, so we might (and in fact, do) end up actually executing the call at runtime. In the second example, `Box::new()` has an uninhabited argument (the `!` type). This means that this call is **definitely dead** - since the `!` type can never be instantiated, it's impossible for the call to every be executed. This forms the basis for the check. For each method call, we check the following: 1. Did the generic arguments have unconstrained type variables prior to fallback? 2. Did any of the generic arguments become uninhabited after fallback? 3. Are all of the method arguments inhabited? If the answer to all of these is *yes*, we emit an error. I've left extensive comments in the code describing how this is accomplished. These conditions ensure that we don't error on the `Box` and `From<!>` examples, while still erroring on the bad `objc` code. ### Further notes You can test out this branch with the original bad `objc` code as follows: 1. Clone `https://github.com/Aaron1011/rust-objc` 2. Checkout the `bad-fallback` branch. 3. With a local rustc toolchain built from this branch, run `cargo build --example example` 4. Note that you get an error due to an unconstrained return type ### Unresolved questions 1. This lint only checks method calls. I believe this is sufficient to catch any undefined behavior caused by fallback changes. Since the introduced undefined behavior relies on actually 'producing' a `!` type instance, the user must be doing something 'weird' (calling `transmute` or some other intrinsic). I don't think it's possible to trigger this without *some* kind of intrinsic call - however, I'm not 100% certain. 2. This lint requires us to perform extra work during the type-checking of every single method. This is not ideal - however, changing this would have required significant refactoring to method type-checking. It would be a good idea to due to a perf run to see what kind of impact this has, and it another approach will be required. 3. This 'lint' is currently a hard error. I believe it should always be possible to fix this error by adding explicit type annotations *somewhere* (though in the `obj` case, this may be in the caller of a macro). Unfortunately, I think actually emitting any kind of suggestion to the user will be extremely difficult. Hopefully, this error is so rare that the lack of suggestion isn't a problem. If users are running into this with any frequency, I think we'll need a different approach. 4. If this PR is accepted, I see two ways of rolling this out: 1. If the bad `objc` crate is the only crate known to be affected, we could potentially go from no warning/lint to a hard error in a single release (coupled enabling never-type fallback0. 2. If we're worried that this could break a lot of crates, we could make this into a future compatibility lint. At some point in the future, we could enable never-type fallback while simultaneously making this a hard error. What we should **not** do is make the never-type fallback changes without making this lint (or whatever lint ends up getting accepted) into a hard error. A lint, even a deny-by-default one, would be insufficient, as we would run a serious risk introducing undefined behavior without any kind of explicit acknowledgment from the user.
| /// Stores additional data about a generic path | ||
| /// containing inference variables (e.g. `my_method::<_, u8>(bar)`). | ||
| /// This is used by `NeverCompatHandler` to inspect | ||
| /// all method calls that contain inference variables. | ||
| /// | ||
| /// This struct is a little strange, in that its data | ||
| /// is filled in from two different places in typecheck. | ||
| /// Thw two `Option` fields are written by `check_argument_types` | ||
| /// and `instantiate_value_path`, since neither method | ||
| /// has all of the information it needs. | ||
| #[derive(Clone, Debug)] | ||
| struct InferredPath<'tcx> { | ||
| /// The span of the corresponding expression. | ||
| span: Span, | ||
| /// The type of this path. For method calls, | ||
| /// this is a `ty::FnDef` | ||
| ty: Option<Ty<'tcx>>, | ||
| /// The types of the arguments (*not* generic substs) | ||
| /// provided to this path, if it represents a method | ||
| /// call. For example, `foo(true, 25)` would have | ||
| /// types `[bool, i32]`. If this path does not | ||
| /// correspond to a method, then this will be `None` | ||
| /// | ||
| /// This is a `Cow` rather than a `Vec` or slice | ||
| /// to accommodate `check_argument_types`, which may | ||
| /// be called with either an interned slice or a Vec. | ||
| /// A `Cow` lets us avoid unecessary interning | ||
| /// and Vec construction, since we just need to iterate | ||
| /// over this | ||
| args: Option<Cow<'tcx, [Ty<'tcx>]>>, | ||
| /// The unresolved inference variables for each | ||
| /// generic substs. Each entry in the outer vec | ||
| /// corresponds to a generic substs in the function. | ||
| /// | ||
| /// For example, suppose we have the function | ||
| /// `fn foo<T, V> (){ ... }`. | ||
| /// | ||
| /// The method call `foo::<MyStruct<_#0t, #1t>, true>>()` | ||
| /// will have an `unresolved_vars` of `[[_#0t, _#1t], []]` | ||
| unresolved_vars: Vec<Vec<Ty<'tcx>>>, | ||
| } |
There was a problem hiding this comment.
Would be good to move this into never_compat to keep all the logic required for that more self-contained. This file is also already huge and I would prefer not to add anything more miscellaneous to it.
| let fn_inputs = fn_inputs.into(); | ||
| debug!("check_argument_types: storing arguments for expr {:?}", expr); | ||
| // We now have the arguments types available for this msthod call, | ||
| // so store them in the `inferred_paths` entry for this method call. | ||
| // We set `ty` as `None` if we are the first to access the entry | ||
| // for this method, and leave it untouched otherwise. | ||
| match self.inferred_paths.borrow_mut().entry(expr.hir_id) { | ||
| Entry::Vacant(e) => { | ||
| debug!("check_argument_types: making new entry for types {:?}", fn_inputs); | ||
| e.insert(InferredPath { | ||
| span: sp, | ||
| ty: None, | ||
| args: Some(fn_inputs.clone()), | ||
| unresolved_vars: vec![], | ||
| }); | ||
| } | ||
| Entry::Occupied(mut e) => { | ||
| debug!( | ||
| "check_argument_types: modifying existing {:?} with types {:?}", | ||
| e.get(), | ||
| fn_inputs | ||
| ); | ||
| e.get_mut().args = Some(fn_inputs.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ditto with this bit. Let's move the logic into a new function inside never_compat.
| if ty_substituted.has_infer_types() { | ||
| debug!( | ||
| "instantiate_value_path: saving path with infer: ({:?}, {:?})", | ||
| span, ty_substituted | ||
| ); | ||
| let parent_id = tcx.hir().get_parent_node(hir_id); | ||
| let parent = tcx.hir().get(parent_id); | ||
| match parent { | ||
| Node::Expr(hir::Expr { span: p_span, kind: ExprKind::Call(..), .. }) | ||
| | Node::Expr(hir::Expr { span: p_span, kind: ExprKind::MethodCall(..), .. }) => { | ||
| // Fill in the type for our parent expression. This might not be | ||
| // a method call - if it is, the argumetns will be filled in by | ||
| // `check_argument_types` | ||
| match self.inferred_paths.borrow_mut().entry(parent_id) { | ||
| Entry::Vacant(e) => { | ||
| debug!("instantiate_value_path: inserting new path"); | ||
| e.insert(InferredPath { | ||
| span: *p_span, | ||
| ty: Some(ty_substituted), | ||
| args: None, | ||
| unresolved_vars: vec![], | ||
| }); | ||
| } | ||
| Entry::Occupied(mut e) => { | ||
| debug!("instantiate_value_path: updating existing path {:?}", e.get()); | ||
| e.get_mut().ty = Some(ty_substituted); | ||
| } | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
..and let's move this part as well into a new function.
| use rustc_data_structures::fx::FxHashMap; | ||
| use rustc_hir::HirId; | ||
|
|
||
| /// Code to detect cases where using `!` (never-type) fallback instead of `()` fallback |
There was a problem hiding this comment.
Shouldn't this be a module doc comment (//!)?
|
|
||
| let mut err = tcx | ||
| .sess | ||
| .struct_span_err(span, "Fallback to `!` may introduce undefined behavior"); |
There was a problem hiding this comment.
| .struct_span_err(span, "Fallback to `!` may introduce undefined behavior"); | |
| .struct_span_warn(span, "fallback to `!` may introduce undefined behavior"); |
There was a problem hiding this comment.
I'm happy with keeping this an error for the purposes of a crater run, but this needs to become a warning (or a lint, depending on your opinions re. --cap-lints) before the PR can land.
Centril
added
I-nominated
T-lang
|
I'll schedule a crater run as well when the try builder is done. Please avoid pushing to the branch meanwhile. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
We discussed this in the T-Lang meeting today but did not reach a conclusion re. rollout. We're awaiting crater data for further evaluation next week. |
|
@Centril: Are the meeting minutes publically available? |
|
I think @nikomatsakis will publish them soon in the lang team repo and we also should have a recording up soon on YouTube. |
|
I am particularly curious to see what sorts of false errors we see. One alternative I wanted to note that might be worth considering: If we wanted to detect only cases where the "type variables which fell back to This has the advantage of of naturally avoiding, I think, all of the false errors that we've seen? I'm not sure if it has its own set of false errors, though. |
|
@Aaron1011 the minutes are here, they include a link to the recording |
I thought about doing something like that. However, the current "unreachable code" is fairly coarse-grained - it uses a single Additionally, the The dead-code detection needs to detect dead code with no false positives (so that we don't lint code that's actually live). False negatives (failing to detect dead code) are fine, since this lint is best effort. A never-type fallback lint needs to detect live code with as few false positives as possible (so we avoid linting code that never actually executes). It might be possible to achieve both of these goals with the same infrastructure, but I think it would be difficult.
That's wouldn't avoid the |
I suppose. I was intended I think to look for live expressions that yielded the type variable Ah, this reminds me -- in your current code, are you looking for cases where the function returns "some type that mentions the type variable"? Is there a reason not to look for "some function that returns the type variable itself"? Well, I guess you could have a case like: let x: Foo<_> = make_foo();
x.method();where struct Foo<T> {
the_function: fn() -> T,
}
impl Foo {
...
fn method(&self) { (self.the_function)() }
}i.e., the code that produces the In general, the idea of special-casing type constructions is ok, but could be generalized I think to be a test on the return type. Basically if you have a type Hmm, I guess you could also have some kind of function that takes a type parameter fn foo<T>(...) {
let x: T = ...;
}and the caller could somehow determine It'd be good to extend my write-up with all of these cases, I think. I'm not sure which fraction make sure to lint. |
Yeah, that was my concern as well. I wanted to err on the side of linting more code, and seeing if the number of false positives was too high. If the Crater results come back with more than a few errors that would be solved by checking the return type, I think we should definitely reconsider the current implementation. However, if Crater shows few to no cases where this happens, then I think a small risk of "questionable false positives" (in code not covered by Crater) is worth the increased coverage of potential UB. |
Do you mean returning it directly (e.g. My concern is that an struct FFIReturn<T> {
val: T,
timestamp: usize
}Given that a caller-chosen type parameter is already being returned, it seems perfectly reasonable for such a function to add 'extra' information in some way. |
My rationale for the special-casing was less to do with the return type, and more to do with such functions being guaranteed to be "sane". In general, we can't know whether the function being called does something "reasonable" or not - except for when it's compiler generated. |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
The lint is triggering on calls to this #[inline]
fn raise_e(m: impl Into<String>) -> PErr {
PErr::Etc { msg: m.into() }
}
#[inline]
fn raise<T>(m: impl Into<String>) -> Result<T, PErr> {
Err(raise_e(m))
}
...
raise("no sprites")?;Unfortunately, |
|
Report from https://github.com/Mark-Simulacrum/crater-generate-report/: root: bson - 2 (2 gh, 0 crates.io) detected crates which regressed due to this root: cis_client - 5 (5 gh, 0 crates.io) detected crates which regressed due to this
root: jni - 11 (7 gh, 4 crates.io) detected crates which regressed due to this
root: libimagentrytag - 4 (0 gh, 4 crates.io) detected crates which regressed due to this root: oauth2 - 3 (2 gh, 1 crates.io) detected crates which regressed due to this root: proptest-derive - 31 (1 gh, 30 crates.io) detected crates which regressed due to this
root: redis - 32 (17 gh, 15 crates.io) detected crates which regressed due to this
root: rlua - 25 (14 gh, 11 crates.io) detected crates which regressed due to this
root: warp - 73 (53 gh, 20 crates.io) detected crates which regressed due to this
root: unknown causes - 70 (46 gh, 24 crates.io) detected crates which regressed due to this
|
|
There seem to be four types of regressions here:
pub fn bad<T>(something: SomeType) -> Wrapper<T> { ... }which, while completely safe (there's no unsafe calls going on), triggers the lint. |
|
To summarize: True positives: False positives: All of the false positives seem to have the same underlying cause: A pub fn bad<T>(something: SomeType) -> Wrapper<T> { ... }Only one variant of the enum is ever constructed (the one not involving the type parameter), though there may be several method calls between the call to |
|
I think it might be possible to refine the check as follows:
I believe that this check will catch any cases where an uninhabited type is 'produced' (e.g. a live expression actually gets an uninhaibted type) due to never-type fallback. It should exclude all of the current false-positives, since the uninhabited type parameter never winds up being produced in any way. However, it has some significant downsides:
I think we have a few options:
|
|
@Aaron1011 thanks for the detailed examination! Super useful. I am definitely nervous about the idea of having the lint be non-modular (i.e., examining things across functions). (Honestly, this is making me wonder if the idea of changing fallback from There is of course another alternative. Rather than give up on this approach entirely, we could refine the lint to just look for return types that are uninhabited (and which involve a "fallback" variable) and thus permit |
|
Well, what I just wrote might not be quite true -- I guess that the case of wouldn't be caught by the lint as I described, but would require us to be testing the type of "live expressions". |
Unfortunately, that would fail to catch the original |
|
I'm starting to think that the core premise of this lint is flawed. My original idea was that we could use a 'top-down' approach: we start from the function where fallback occurred, and see if control flow reaches a potentially 'questionable' function. However, the Crater run shows that the following pattern is used by many different crates: fn make_it<T>(param: Something) -> Wrapper<T> { ... }
I would have hoped that the combination of the From the 'outside' (e.g. without inspecting function bodies), there is fundamentally no difference between a 'good' function like I'm starting to think that a 'bottom-up' approach might be more effective. The immediate cause of the problem is a call to My idea is to use a greatly simplified version of the MIR-based check I previously proposed. There are three steps:
This approach still has all of the drawbacks of a MIR-based lint (non-modularity, interaction with
However, this additional complexity might not be worth the cost. If we don't anticipate any affected crates other than |
|
Regarding the objc case of returning Regarding the transmute proposal, a few brief thoughts:
|
The only non- |
|
☔ The latest upstream changes (presumably #70536) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to go ahead and close this PR. I think at this point we've decided that this is not the approach we want to take. Let's continue the discussion on #66173, I just posted a follow-up there yesterday. |
Fixes #67225
Fixes #66173
Tl;DR: This PR introduces a lint that detects the 'bad' never-type fallback in
objc(which results in U.B.), while allowing safe fallback to occur without a warning.See https://hackmd.io/@FohtAO04T92wF-8_TVATYQ/SJ0vcjyWL for some background on never-type fallback.
The problem
We want to reject "bad" code like this:
in which enabling never-type fallback can cause undefined behavior.
However, we want to allow "good" code like this:
which relies on never-type fallback being enabled, but is perfectly safe.
The solution
The key difference between these two examples lies in how the result of never-type fallback is used. In the first example, we end up inferring the generic parameter of
unconstrained_returnto be!. In the second example, we still infer a generic parameter to be!(Box::<!>::new(!)), but we also pass an uninhabited parameter to the function.Another way of looking at this is that the call to
unconstrained_returnis *potentially live - none of its arguments are uninhabited, so we might (and in fact, do) end up actually executing the call at runtime.In the second example,
Box::new()has an uninhabited argument (the!type). This means that this call is definitely dead - since the!type can never be instantiated, it's impossible for the call to every be executed.This forms the basis for the check. For each method call, we check the following:
If the answer to all of these is yes, we emit an error. I've left extensive comments in the code describing how this is accomplished.
These conditions ensure that we don't error on the
BoxandFrom<!>examples, while still erroring on the badobjccode.Further notes
You can test out this branch with the original bad
objccode as follows:https://github.com/Aaron1011/rust-objcbad-fallbackbranch.cargo build --example exampleUnresolved questions
This lint only checks method calls. I believe this is sufficient to catch any undefined behavior caused by fallback changes. Since the introduced undefined behavior relies on actually 'producing' a
!type instance, the user must be doing something 'weird' (callingtransmuteor some other intrinsic). I don't think it's possible to trigger this without some kind of intrinsic call - however, I'm not 100% certain.This lint requires us to perform extra work during the type-checking of every single method. This is not ideal - however, changing this would have required significant refactoring to method type-checking. It would be a good idea to due to a perf run to see what kind of impact this has, and it another approach will be required.
This 'lint' is currently a hard error. I believe it should always be possible to fix this error by adding explicit type annotations somewhere (though in the
objcase, this may be in the caller of a macro). Unfortunately, I think actually emitting any kind of suggestion to the user will be extremely difficult. Hopefully, this error is so rare that the lack of suggestion isn't a problem. If users are running into this with any frequency, I think we'll need a different approach.If this PR is accepted, I see two ways of rolling this out:
If the bad
objccrate is the only crate known to be affected, we could potentially go from no warning/lint to a hard error in a single release (coupled enabling never-type fallback0.If we're worried that this could break a lot of crates, we could make this into a future compatibility lint. At some point in the future, we could enable never-type fallback while simultaneously making this a hard error.
What we should not do is make the never-type fallback changes without making this lint (or whatever lint ends up getting accepted) into a hard error. A lint, even a deny-by-default one, would be insufficient, as we would run a serious risk introducing undefined behavior without any kind of explicit acknowledgment from the user.