Better typeck divergence #50262
Better typeck divergence #50262
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
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 |
|
I would prefer at least a lang team FCP for these sorts of changes. cc @rust-lang/lang |
|
So above the actual lang-team FCP, the main thing I want is documentation for what is happening, so that we can be sure we're not doing anything stupid here. Also, as implemented, this makes some code legal and insta-stable, so this probably requires some stability gating. |
|
I'll explain what's going on in a comment on
Do you think it'd be better to make the changes to the warnings, but feature gate them instead then? I'll do that for now! Also, I can't test the debuginfo failure at the moment, but I suspect it's caused by the changes from |
|
The test is testing something that is definitely UB: rust/src/test/debuginfo/nil-enum.rs Lines 30 to 43 in 3c43aa5 It should be changed to not be UB. |
This comment has been minimized.
This comment has been minimized.
varkor
force-pushed
the
better-divergence
branch
from
874d8f5
to
9c46fe6
Compare
|
I couldn't see any reason to keep that test around, as its entire purpose was UB as far as I can tell. I've added more of a description to |
|
I do want to have a test that checks that the debug-info is sane (e.g., does not kill gdb). You could test without invoking UB by using a raw pointer, and printing the dereference of it in gdb ( enum Empty {}
fn zzz() {}
fn main() {
let first: *const Empty = 1 as *const _;
zzz();
} |
|
#50304 has some overlap, I didn't care about type checking at all but I used the proper layout infra for the uninhabitedness check in trans. |
varkor
force-pushed
the
better-divergence
branch
from
9c46fe6
to
a6c28bd
Compare
This comment has been minimized.
This comment has been minimized.
varkor
force-pushed
the
better-divergence
branch
from
bf69fba
to
2fecb9f
Compare
This comment has been minimized.
This comment has been minimized.
varkor
force-pushed
the
better-divergence
branch
from
2fecb9f
to
726cc6c
Compare
This comment has been minimized.
This comment has been minimized.
| @@ -15,31 +15,21 @@ | |||
| // compile-flags:-g | |||
| // gdb-command:run | |||
|
|
|||
| // gdb-command:print first | |||
There was a problem hiding this comment.
Why did you remove the gdbr-check? Please leave it here
varkor
force-pushed
the
better-divergence
branch
2 times, most recently
from
8775da4
to
ed3b2e8
Compare
src/libsyntax/feature_gate.rs
Outdated
| @@ -466,6 +466,9 @@ declare_features! ( | |||
|
|
|||
| // #[doc(alias = "...")] | |||
| (active, doc_alias, "1.27.0", Some(50146), None), | |||
|
|
|||
| // Enables improved divegence checking in functions/match arms. | |||
There was a problem hiding this comment.
So one of the things that was a bit tricky with this PR for me was that I felt like it's goals were not clearly articulated. Here are the things I think it does, let me know if I am missing anything:
- Extend the "conservative" definition of uninhabited to include not only
!and empty enums but also tuples and non-empty arrays of said types.&!is still not included- Neither is
struct Foo(!)
- Expands the check for functions that return
!to include any uninhabited type- this also affects liveness, debuginfo, and a few other random places
- Marks function bodies as dead code whenever function parameters are uninhabited
- maybe we should special case an empty body here for the purposes of warnings?
- I believe an empty body would always be legal, because of the rule that a block with an implied tail expression can be coerced to
!-- or perhaps we could make that true anyway?
I think I'm in favor of all 3 of those goals, though the first one seems somewhat separable to me from the rest. That part makes me the most nervous, though I think I cannot imagine any unsafe code guidelines that would permit (!,) (and indeed I think this is already UB in existing code).
| @@ -213,8 +213,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
| exit_block.unit() | |||
| } | |||
| ExprKind::Call { ty, fun, args } => { | |||
| // FIXME(canndrew): This is_never should probably be an is_uninhabited | |||
| let diverges = expr.ty.is_never(); | |||
| let diverges = expr.ty.conservative_is_uninhabited(); | |||
| @@ -415,8 +415,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> { | |||
| args: I) -> CFGIndex { | |||
| let func_or_rcvr_exit = self.expr(func_or_rcvr, pred); | |||
| let ret = self.straightline(call_expr, func_or_rcvr_exit, args); | |||
| // FIXME(canndrew): This is_never should probably be an is_uninhabited. | |||
| if self.tables.expr_ty(call_expr).is_never() { | |||
| if self.tables.expr_ty(call_expr).conservative_is_uninhabited() { | |||
src/librustc/ty/sty.rs
Outdated
| // (for enums, unions, etc.). | ||
| ty::TyAdt(def, _) => def.variants.is_empty(), | ||
| ty::TyTuple(tys) => tys.iter().any(|ty| ty.conservative_is_uninhabited()), | ||
| ty::TyArray(ty, len) => { |
There was a problem hiding this comment.
Any particular reason to expand the set of cases here? Not that I I necessarily object. It seems though that including (!,) but not struct Foo(!) is sort of surprising to me -- in other words, I'm tempted to stick to ! and Void, and separate out this aspect of the PR.
| @@ -986,8 +986,7 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> { | |||
| } | |||
| } | |||
| None => { | |||
| // FIXME(canndrew): This is_never should probably be an is_uninhabited | |||
| if !sig.output().is_never() { | |||
| if !sig.output().conservative_is_uninhabited() { | |||
| @@ -225,7 +225,15 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> { | |||
| let scrutinee_is_uninhabited = if self.tcx.features().exhaustive_patterns { | |||
| self.tcx.is_ty_uninhabited_from(module, pat_ty) | |||
| } else { | |||
| self.conservative_is_uninhabited(pat_ty) | |||
| if self.tcx.features().better_divergence_checking { | |||
There was a problem hiding this comment.
Maybe we should lump this feature gate in with exhaustive_patterns? I'm not sure yet that I see the difference here
There was a problem hiding this comment.
Well, the one reason it might be worth separating is because the change to uninhabited function parameter warning may not really be well suited to exhaustive_patterns. But it does seem like overkill to have another feature for this, so I'll add it there for now.
src/librustc_trans/debuginfo/mod.rs
Outdated
| @@ -271,7 +271,7 @@ pub fn create_function_debug_context<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, | |||
| } | |||
| None => {} | |||
| }; | |||
| if sig.output().is_never() { | |||
| if sig.output().conservative_is_uninhabited() { | |||
src/librustc_trans/declare.rs
Outdated
| @@ -133,8 +133,7 @@ pub fn declare_fn<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, name: &str, | |||
| let fty = FnType::new(cx, sig, &[]); | |||
| let llfn = declare_raw_fn(cx, name, fty.llvm_cconv(), fty.llvm_type(cx)); | |||
|
|
|||
| // FIXME(canndrew): This is_never should really be an is_uninhabited | |||
| if sig.output().is_never() { | |||
| if sig.output().conservative_is_uninhabited() { | |||
src/librustc_typeck/check/mod.rs
Outdated
| @@ -433,6 +433,14 @@ pub enum PlaceOp { | |||
| /// wake). Tracked semi-automatically (through type variables marked | |||
| /// as diverging), with some manual adjustments for control-flow | |||
| /// primitives (approximating a CFG). | |||
| /// We know a node diverges in the following (conservative) situations: | |||
There was a problem hiding this comment.
Nit: new paragraph please
src/librustc_typeck/check/mod.rs
Outdated
| if arg_ty.conservative_is_uninhabited() { | ||
| fcx.diverges.set(fcx.diverges.get() | Diverges::Always); | ||
| } | ||
| } |
There was a problem hiding this comment.
it's definitely an interesting "judgement call" whether we want to issue a warning in this case
src/librustc_typeck/check/mod.rs
Outdated
| self.diverges.set(self.diverges.get() | Diverges::Always); | ||
| // Any expression that produces a value of an uninhabited type must have diverged. | ||
| if ty.conservative_is_uninhabited() { | ||
| if ty.is_never() || self.tcx.features().better_divergence_checking { |
There was a problem hiding this comment.
seems good 👍 -- in particular, seems like a natural extension of the existing check, and it's not clear that ! should be special here
Yeah, I did amalgamate some separate but related issues here. I'll separate the first one out into a follow-up.
Yes, those are right.
Perhaps — this would require some tweaks, as (despite [1]) empty blocks don't seem to be coerced even when diverging (granted, this possibility is new as of this PR, so that's probably not unexpected). I'll look into doing that. |
This comment has been minimized.
This comment has been minimized.
varkor
force-pushed
the
better-divergence
branch
from
2116316
to
f0c6599
Compare
varkor
force-pushed
the
better-divergence
branch
from
57bc68d
to
168d2a3
Compare
|
I've addressed all the current comments. |
|
Ping from triage @nikomatsakis! This PR needs your review. |
|
Ping from triage @nikomatsakis / @rust-lang/compiler. This PR requires your review. |
pietroalbini
added
S-waiting-on-team
|
Ping from triage! This PR needs a review, can @nikomatsakis or someone else from @rust-lang/compiler review this? |
|
We decided on the lang team meeting to revisit this next week. |
There was a problem hiding this comment.
OK, I did another pass. I think my feeling on this PR is that I am vaguely 👍 on the direction it is going but not sure about the point that it reaches. I'm a bit nervous, for example, about the new warnings — and I'd like to know what our overall plan is.
I am wondering, @varkor, if you'd be interested in starting work on an RFC, perhaps incorporating the ideas around a ! pattern, to kind of "set the plan" here?
I'd be interested in seeing such an RFC and helping to discuss it, but I don't think I can take lead on it.
| @@ -270,7 +270,7 @@ pub fn create_function_debug_context<'a, 'tcx>(cx: &CodegenCx<'a, 'tcx>, | |||
| } | |||
| None => {} | |||
| }; | |||
| if cx.layout_of(sig.output()).abi == ty::layout::Abi::Uninhabited { | |||
| if sig.output().conservative_is_uninhabited() { | |||
There was a problem hiding this comment.
Tell LLVM that functions that return uninhabited types can never return
| ty::TyNever => true, | ||
| ty::TyAdt(def, _) => def.variants.is_empty(), | ||
| _ => false | ||
| } |
There was a problem hiding this comment.
Why this change here?
There was a problem hiding this comment.
In other words, why not invoke the conservative_is_uninhabited fn we are invoking elsewhere
There was a problem hiding this comment.
I think it would be identical?
| if let hir::ExprKind::Block(ref block, _) = body.value.node { | ||
| // If the function is completely empty, or has a single trailing | ||
| // expression, then we do not issue a warning (as it was likely | ||
| // mandated by a trait, rather than being an oversight). |
There was a problem hiding this comment.
Hmm, these rules seem reasonable, but not perfect. For example, by these rules, the following would not warn:
fn foo(x: !) {
{
println!("Hi"):
}
}but this would, right?
fn foo(x: !) {
{
println!("Hi"):
}
}(What happens if you use the unimplemented! macro? I guess it expands to a block or something.)
There was a problem hiding this comment.
hmm... did you mean to write something else in the second snippet? They are identical...
| /// converted to `WarnedAlways`. Used when | ||
| /// unreachable code is expected (e.g. in | ||
| /// function parameters as part of trait impls). | ||
| UnwarnedAlways, |
There was a problem hiding this comment.
If I understand the role of this value, it is basically used to signal when the fn has "uninhabited" arguments -- but only if the body of the function looks "sufficiently trivial". Did we anticipate this having a "flow sensitive" role at some point? If not, I feel like it might be clearer to add a field like suppress_dead_code_warnings: bool into the FnCtxt.
(Otherwise, it kind of messes with the lattice)
| @@ -310,7 +310,7 @@ impl Error for string::FromUtf16Error { | |||
| #[stable(feature = "str_parse_error2", since = "1.8.0")] | |||
| impl Error for string::ParseError { | |||
| fn description(&self) -> &str { | |||
| match *self {} | |||
| match self {} | |||
There was a problem hiding this comment.
Why this change?
Also, would this code compile on stable...?
There was a problem hiding this comment.
Well, I see that libstd/lib.rs has the exhaustive_patterns feature, so perhaps that accounts for it.
| @@ -18,6 +18,7 @@ enum Void {} | |||
| impl From<Void> for i32 { | |||
| fn from(v: Void) -> i32 { | |||
| match v {} | |||
| //~^ WARN unreachable expression | |||
There was a problem hiding this comment.
This is a bit surprising to me — I feel like this code should compile without warning. Matching with no arms feels like a great way (to me) to declare to the compiler "I am well aware this code is unreachable".
There was a problem hiding this comment.
If not this code, what would you expect the user to write?
| @@ -39,6 +40,7 @@ fn qux(x: Result<u32, Void>) -> Result<u32, i32> { | |||
| fn vom(x: Result<u32, Void>) -> Result<u32, i32> { | |||
| let y = (match x { Ok(n) => Ok(n), Err(e) => Err(e) })?; | |||
| //~^ WARN unreachable pattern | |||
| //~| WARN unreachable expression | |||
There was a problem hiding this comment.
Similarly here, it feels unfortunate to start warning unless we have a nicer way to write this code (but, at present, we don't...?).
If we had the ! pattern concept, we could perhaps do
match x {
Ok(n) => Ok(n),
Err(!)
}| // is permitted if it simply returns a value | ||
| // as a trailing expression to satisfy the | ||
| // return type. | ||
| v |
There was a problem hiding this comment.
I guess this answers my question to "how should the user write this code" -- instead of match void { } they would simply write void?
But that will only work if the uninhabited parameter has type !...? Or, maybe we changed that in this PR somewhere?
| let _ = match x {}; //~ ERROR non-exhaustive | ||
|
|
||
| let x: [Void; 1] = unsafe { std::mem::uninitialized() }; | ||
| let _ = match x {}; //~ ERROR non-exhaustive |
There was a problem hiding this comment.
I think if we have a feature-gate test, we should also have an equivalent test but with the feature-gate enabled so we can see the difference — is there such a test somewhere? (If so, I missed it in the PR)
|
Removing nomination: I don't think there is an urgent decision for lang team to make here, as I wrote earlier I am interesting in having a kind of "comprehensive" RFC on this topic and would like to work with someone on it. That seems like the proper venue to discuss. |
|
I'm going to write a little summary now and think about how to approach this more comprehensively. This PR was intended to increase the number of cases taken into account by the typeck divergence checking. This divergence checking is just used for error messages (e.g. One problem here is that it's being done on an expression level, which for uninhabited arguments arguably isn't the right place to warn, which is leading to most of the issues with the warning message changes here. The other problem is that although some warnings technically apply to uninhabited types in general, because Rust only coerces I think the correct solution for now is to split the changes in this PR up and change the warning for uninhabited parameter types to a new warning that's more informative. |
…, r=<try> Less conservative uninhabitedness check Extends the uninhabitedness check to structs, non-empty enums, tuples and arrays. Pulled out of #47291 and #50262. Blocked on #54123. r? @nikomatsakis
…dness-check, r=nikomatsakis Less conservative uninhabitedness check Extends the uninhabitedness check to structs, non-empty enums, tuples and arrays. Pulled out of rust-lang#47291 and rust-lang#50262. Fixes rust-lang#54586. r? @nikomatsakis
…, r=nikomatsakis Less conservative uninhabitedness check Extends the uninhabitedness check to structs, non-empty enums, tuples and arrays. Pulled out of #47291 and #50262. Fixes #54586. r? @nikomatsakis
This pulls in some of the ideas from #47291 about divergence checking as a first step, namely: if a function takes arguments of an uninhabited type, it must be diverging, and it is often possible to check for uninhabited types rather than the never type specifically.
There are still a few places special-cased on
is_never, which I left alone because I didn't want to mess around with the special coercions involving!, but I might have been over-cautious.This adds a new case,
Diverges::UnwarnedAlways, that specifically does not lint forunreachable_code, because I'm conscious it's going to break anything involving (for example) empty enum impls. If we're happy doing that, though, this case can be removed.(Arguably some/most of the changes involving
is_neverare not typeck related, but they seemed in a similar vein, so I included them in this PR. I can split if necessary though.)r? @arielb1
cc @nikomatsakis @canndrew