-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make typeck aware of uninhabited types #108993
base: master
Are you sure you want to change the base?
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred in src/tools/rust-analyzer cc @rust-lang/wg-rls-2 |
eed5cd7
to
bbf2a43
Compare
☔ The latest upstream changes (presumably #109015) made this pull request unmergeable. Please resolve the merge conflicts. |
bbf2a43
to
9bfac82
Compare
The Miri subtree was changed cc @rust-lang/miri |
☔ The latest upstream changes (presumably #108872) made this pull request unmergeable. Please resolve the merge conflicts. |
9bfac82
to
5e396d2
Compare
I know too little about this area. |
☔ The latest upstream changes (presumably #107270) made this pull request unmergeable. Please resolve the merge conflicts. |
The lang team discussed this in our meeting (notes). We didn't see any obvious concerns but want to understand more about what this change does that we and/or the types team should be looking closely at. (Is it just the fact that the compiler accepts more programs now that it didn't before?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some questions and nits, apart from that r=me on the impl 🤔
let res = self.typeck_results.borrow().qpath_res(qpath, callee.hir_id); | ||
!matches!(res, Res::Def(DefKind::Ctor(..), _)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am a bit confused why that doesn't warn 🤔
Constructors are carved out on purpose, to avoid an "unreachable call" diagnostic on them, which is not very useful as they are inert.
I don't understand that comment 🤔 I would assume the outer constructor to keep warning here?
struct Foo<T>(T);
fn foo() -> Foo<String> {
Foo(return Foo(String::new()))
}
@@ -2960,3 +2990,41 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
} | |||
} | |||
} | |||
|
|||
fn expr_may_be_uninhabited(expr: &hir::Expr<'_>) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a comment here. is this about which expressions can actually introduce an uninhabited type. E.g. arrays is not considered because in [return;]
its the nested expression that is uninhabited 🤔 but then why do we return false
for ExprKind::Continue
and ExprKind::Return
?
@@ -470,6 +470,7 @@ impl Constructor { | |||
(IntRange(self_range), IntRange(other_range)) => self_range.is_covered_by(other_range), | |||
(FloatRange(void), FloatRange(..)) => match *void {}, | |||
(Str(void), Str(..)) => match *void {}, | |||
#[allow(unreachable_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not remove that branch in this case 😅
Dlsym::FreeBsd(dlsym) => | ||
freebsd::EvalContextExt::call_dlsym(this, dlsym, args, dest, ret), | ||
#[allow(unreachable_code)] | ||
Dlsym::Linux(dlsym) => linux::EvalContextExt::call_dlsym(this, dlsym, args, dest, ret), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here? do we warn for unreachable code but you still can't remove the match arm or sth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, removing the arm gives a non-exhaustive pattern error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's a pretty big issue if the unreachable code lint has false positives. I guess we could instead write match Dlsym {}
here. Or what's actually happening is that the fact that Dlsym
is uninhabited is conceptionally private to the linux shim.
I think the fix should be to instead change Dlsym
to
pub struct Dlsym(Void);
i guess one issue is that by doing it eagerly during hir typeck, we're missing cases which only diverge after type inference: (playground) struct Wrapper<T>(Option<T>);
impl<T> Wrapper<T> {
fn something(&self) -> T {
panic!()
}
}
// using `FnOnce` bound to use `!` on stable.
fn infer<F: FnOnce() -> R, R>(_: Wrapper<R>) {}
pub fn with_never() {
let x = Wrapper(None);
if false {
// currently doesn't warn as liveness analysis intentionally
// skips `!`, won't warn with this PR
//
// warns that `y` is unused which just seems buggy?
let y = x.something();
let _ = y;
}
infer::<fn() -> !, _>(x)
}
enum Void {}
pub fn with_void() {
let x = Wrapper(None);
if false {
// currently warns that the assignment is unreachable,
// will stop warning with this PR
let y = x.something();
let _ = y;
}
infer::<fn() -> Void, _>(x)
} i do think that is acceptable though 🤷 |
From what I can tell the only meaningful change is that expressions which have an uninhabited type are now also considered as diverging by typeck (instead of only being considered so for the tests/ui/uninhabited/break-diverging-value.rs and tests/ui/uninhabited/break-diverging-value-pass.rs for examples of that change it feels pretty straightforward to me and should be the responsibility of t-types, so @rfcbot fcp merge |
Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
5e396d2
to
8b92f6a
Compare
This comment has been minimized.
This comment has been minimized.
If I understand correctly then this PR would be a breaking change to do after the For example, this is accepted today (play): #![feature(min_exhaustive_patterns)]
#![allow(unused)]
fn infer<F: FnOnce() -> R, R>(_: Result<bool, R>) {}
enum Void {}
fn with_void() {
let mut x = Ok(true);
match x {
Ok(_) => {}
}
infer::<fn() -> Void, _>(x)
} Given how rust usually behaves with inference, I think it would be consistent to not accept this. But we have to decide quickly. |
82200be
to
fbc635f
Compare
This comment has been minimized.
This comment has been minimized.
@WaffleLapkin this may be interesting to you re: the never type |
fn wild_let() -> u32 { | ||
let ptr: *const Void = std::ptr::null(); | ||
unsafe { | ||
//~^ ERROR: mismatched types | ||
let _ = *ptr; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect: we should not infer that this function diverges, because let _ = *ptr;
does not cause a read. In fact let _ = *(std::ptr::null::<Void>());
is valid code that creates a null pointer and does nothing with it (or maybe we need dangling()
, not sure). Typechecking should distinguish place expressions from value expressions in order to be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not pre-existing though? I did not have time to work on #117288 and neither anyone else as far as I'm aware. Do we have any infrastructure to distinguish places & values in hir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk if that's the same issue, but we shouldn't regress behavior.
Place vs value is just a matter of matching on ExprKind, there's probably a helper somewhere to distinguish the two
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what you are saying is that an expression is a place iff it's Unary(Deref, _)
, Index(..)
or Field(..)
? But are those always places? Or does it not matter because in cases where we care they are places?...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use MemCategorizationContext
, I think that's the right helper for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is
rust/compiler/rustc_hir/src/hir.rs
Lines 1622 to 1626 in bb78dba
/// Whether this is a place expression. | |
/// | |
/// `allow_projections_from` should return `true` if indexing a field or index expression based | |
/// on the given expression should be considered a place expression. | |
pub fn is_place_expr(&self, mut allow_projections_from: impl FnMut(&Self) -> bool) -> bool { |
MemCategorizationContext
looks right for this, thanks for the pointer!
match secretely_void { | ||
_ => {} //~ ERROR unreachable | ||
_ => {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, how come this "unreachable" is removed but functions like option_never2
above still compile? Can you try running just this on its own:
fn option_never(x: Void) -> impl Copy {
if false {
match option_never(x) {
None => {}
}
}
Some(x)
}
This should error as non-exhaustive, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I know: when there are no explicit patterns like in match secretely_void { _ => {} }
, exhaustiveness calls inhabited_predicate
on the whole type. When there are patterns, it calls it on the subtypes after normalizing. So in my option_never
example it sees that Some
contains Void
.
In other words, this makes exhaustiveness inconsistent:
let secretely_void = SecretelyVoid(opaque_void);
match secretely_void {
SecretelyVoid(_) => {} // WARN: unreachable
}
match secretely_void {} // ERROR: non_exhaustive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the simple thing to do is keep the old normalizing behavior when calling is_uninhabited
for exhaustiveness checking. This also solves the breaking change issue with min_exhaustive_patterns
.
pub const C: () = { | ||
pub const C: () = { //~ ERROR evaluation of constant value failed | ||
let E::A(ref a) = unsafe { &(&U { u: () }).e}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark regarding places vs expressions: there is no value of type E
created here, so I don't think there should be any unreachable code reached.
if !span.is_desugaring(DesugaringKind::CondTemporary) | ||
&& !span.is_desugaring(DesugaringKind::Async) | ||
&& !orig_span.is_desugaring(DesugaringKind::Await) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a distinction between span
and orig_span
that has been lost, was that intentional?
@@ -0,0 +1,26 @@ | |||
// Verify that we do not warn on type-dependent constructors (`Self::A` below). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment no longer makes sense now that you warn for constructors
☔ The latest upstream changes (presumably #123569) made this pull request unmergeable. Please resolve the merge conflicts. |
@Nadrieril thanks for the ping <3 I'm actually proposing to remove diverging block special casing in the next edition: #123590. And I don't really see how semantic changes in this PR are useful. I do not think this is a good idea. The lint changes are probably positive though, can we change the lint without changing semantics? |
fbc635f
to
756cb48
Compare
This comment has been minimized.
This comment has been minimized.
in case it's difficult to find my new comment |
there has also been some discussion between @WaffleLapkin and @Nadrieril here on whether this approach is desirable I think? May also be good for one of you two to take the review here |
So the change proposed in #123590 did not work out, because language team didn't feel that the simpler semantics justify the breakage (esp in the light that there are code patterns that become more awkward). The sentiment generally being "if we would create a new language now: yes. but given the state of rust, probably not". Still, I'm opposed to adding more such special cases. I don't think that the code example in the PR description should compile: enum Void {}
fn get_void() -> Void {
panic!()
}
fn loop_break_void() -> i32 {
let loop_value = loop { break get_void() };
} (block ends in a statement, why do we want to allow this?) As I already said in #108993 (comment) changing the lint seems like a good idea. But changing the language semantics doesn't sound good. |
I don't recall all the details of this here PR, but I do remember reaching the conclusion that "exhaustiveness checking must not change as a result of this PR". Also it seems that |
Haven't accounted for all the comments yet. |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #122551) made this pull request unmergeable. Please resolve the merge conflicts. |
Avoid deriving PartialOrd on Diverges since it includes fields which should not affect ordering.
7e37e36
to
b777188
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #132581) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebase of #100288, thanks @camsteffen for doing the hard work!
This PR modifies the unreachable code lint to operate from within typeck, based on inhabitedness of types discovered as the analysis goes. As a consequence, the lint fires in many extra places. Constructors are carved out on purpose, to avoid an "unreachable call" diagnostic on them, which is not very useful as they are inert.
In addition, inhabitedness information is used from within typeck to guide inference. This allows the following snippet to compile, as the compiler is not able to prove that
loop_break_void
diverges on all paths.Inhabitedness information is privacy aware, so this is only possible if
Void
is visibly uninhabited at the use site. Seetests/ui/break-diverging-value.rs
test for a case where it is not.