Add warning for () to ! switch by canndrew · Pull Request #39009 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation34 Commits9 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

With feature(never_type) enabled diverging type variables will default to ! instead of (). This can cause breakages where a trait is resolved on such a type.

This PR emits a future-compatibility warning when it sees this happen.

r? @nikomatsakis

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

I think future compatibility warnings are supposed to have tracking issues now (ex: #38260).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty reasonable! We will need a test or two, though, and I left a few nits.

// Test whether this is a `()` which was produced by defaulting a
// diverging type variable with `!` disabled. If so, we may need
// to raise a warning.
if let ty::TyTuple(_, true) = obligation.predicate.skip_binder()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can you extract this into a helper function?

if raise_warning {
let sess = tcx.sess;
let span = obligation.cause.span;
let mut warn = sess.struct_span_warn(span, "code relies on type inference rules \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a proper future-compatibility lint. RFC 1589 describes the general idea of how a future compatibility lint works. In terms of the code, you can ripgrep around for LIFETIME_UNDERSCORE to see how the lint is defined.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a new future-compatibility lint resolve_trait_on_defaulted_unit. However it comes with the message "this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!" which, for this warning, isn't exactly true. Does this matter?

{
if as_.len() == bs.len() {
Ok(tcx.mk_tup(as_.iter().zip(bs).map(\|(a, b)
let defaulted = a_defaulted \|

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if this should be a_defaulted && b_defaulted? My thinking is this: if we are relating two () values, and one of them came directly from the user, then that type will continue to be () even when ! fallback is changed. Probably it doesn't matter because in such a case the fallback won't actually trigger anyhow.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I couldn't see when it would ever matter so I decided to err on the side of creating more warnings.

@nikomatsakis We should probably crater this before merging it. I'm curious to see what the fallout is.

Is there any chance of getting this into the next release or is it far too late? I'd like to start raising these warnings as soon as possible.

@canndrew not sure, sorry I'm a bit delayed, was out of town last week. Regarding crater, not a bad idea. Probably best would be to crater a version where the warnigns are set to deny-by-default so we can identify the affected crates most accurately.

@canndrew one thing, can you add a more straight-forward test to the PR? The existing test works because of how we desugar ?, but I consider that a separate bug, it'd be nice to have a test that explicitly used a return and relied on the resulting type fallback in order to compile.

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

I changed the test to use match/return explicitly rather than ?. How'd the crater run go?

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

Of the two affected crates, the second one seems to be the ? case -- it may indeed be spurious in that case, I'm not sure. @eddyb and I were talking more about how this definitely seems like a bug, though I can't remember if we opened an isssue on that or what.

@canndrew that test will do, I suppose, but I was thinking of a test where the type is in fact part of dead-code. I think something like this has been found in the wild (and it currently type-checks):

fn foo<T: ::std::fmt::Debug>(_: T) { }

fn main() { let x = return; foo(x); }

I think the issue in question is #39297.

Ay, more broken crates :( I really hope we can get this warning into the next release.

I'll PR the the two affected libraries.

Of the two affected crates, the second one seems to be the ? case -- it may indeed be spurious in that case,

Depends what you mean by "spurious". Even if that's going to become an error for another reason, we still need to warn about it.

This was referenced

Feb 3, 2017

that test will do, I suppose, but ...

This PR deliberately checks for cases like that and doesn't raise a warning. The () to ! switch won't affect that code. It compiles now and it will compile then and both versions of the code will do the same thing.

This PR deliberately checks for cases like that and doesn't raise a warning. The () to ! switch won't affect that code. It compiles now and it will compile then and both versions of the code will do the same thing.

Do you mean because Debug is implemented for !? Then pick your favorite trait for which ! is not implemented.

📌 Commit 40c9538 has been approved by nikomatsakis

In any case, I'm happy enough with this, and sick of waiting. Let's land this already. =)

⌛ Testing commit 40c9538 with merge dd7ee9d...

Do you mean because Debug is implemented for !? Then pick your favorite trait for which ! is not implemented.

Ah, I getchya. I've added it to the test.

📌 Commit 42f3ac5 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request

…ikomatsakis

Add warning for () to ! switch

With feature(never_type) enabled diverging type variables will default to ! instead of (). This can cause breakages where a trait is resolved on such a type.

This PR emits a future-compatibility warning when it sees this happen.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request

…ikomatsakis

Add warning for () to ! switch

With feature(never_type) enabled diverging type variables will default to ! instead of (). This can cause breakages where a trait is resolved on such a type.

This PR emits a future-compatibility warning when it sees this happen.

bors added a commit that referenced this pull request

Rollup of 19 pull requests

Successful merges: #38518, #38921, #38959, #38983, #39009, #39107, #39193, #39289, #39312, #39393, #39442, #39443, #39453, #39454, #39471, #39477, #39478, #39527, #39552
Failed merges: