Add warning for () to ! switch #39009

canndrew · 2017-01-12T07:22:11Z

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.

rust-highfive · 2017-01-12T07:22:23Z

r? @nikomatsakis

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

canndrew · 2017-01-12T08:00:33Z

Can someone help me figure out what's causing that error? Coz I'm lost :(

It's coming from this line: https://github.com/canndrew/rust/blob/8940922e830dce009cce547a8bc488134803ee8e/src/librustc_trans/symbol_map.rs#L70

If I print out trans_item{1,2} there I get

trans_item1: DropGlue(Ty(core::result::Result<(), errors::DiagnosticBuilder>))
trans_item2: DropGlue(Ty(core::result::Result<(), errors::DiagnosticBuilder>))

Edit: Thanks @eddyb for solving it.

durka · 2017-01-12T19:27:31Z

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

nikomatsakis

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

nikomatsakis · 2017-01-17T17:44:40Z

src/librustc/traits/select.rs

+        // 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()


Nit: can you extract this into a helper function?

nikomatsakis · 2017-01-17T17:47:24Z

src/librustc/traits/select.rs

+            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 \


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.

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?

nikomatsakis · 2017-01-17T17:50:07Z

src/librustc/ty/relate.rs

        {
            if as_.len() == bs.len() {
-                Ok(tcx.mk_tup(as_.iter().zip(bs).map(|(a, b)| relation.relate(a, b)))?)
+                let defaulted = a_defaulted || b_defaulted;


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.

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

canndrew · 2017-01-21T08:14:24Z

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

canndrew · 2017-01-22T07:34:07Z

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.

nikomatsakis · 2017-01-23T21:56:34Z

@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 · 2017-01-25T06:35:00Z

@nikomatsakis
Here's a branch with the warnings changed to errors for cratering: https://github.com/canndrew/rust/tree/default-unit-errors

nikomatsakis · 2017-01-25T16:20:36Z

@canndrew ok I started a run from 2782e8f to 04a8fae -- though my runs have been failing lately, so I'll try to bother @brson too =)

nikomatsakis · 2017-01-25T16:23:42Z

@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.

bors · 2017-01-28T08:50:11Z

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

canndrew · 2017-01-29T16:48:42Z

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

bors · 2017-01-31T02:33:32Z

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

nikomatsakis · 2017-02-01T22:38:36Z

@canndrew I had some technical difficulties, but here are the results:

dyon-0.19.0 (before) (after) -- spurious
clocksource-0.2.0 (before) (after) -- spurious
acme-client-0.2.0 (before) (after)
dweet-0.1.0 (before) (after) -- unclear, probably spurious
ed25519-dalek-0.1.0 (before) (after) -- spurious
enamel-0.3.0 (before) (after) -- spurious
sidekiq-rs-0.6.2 (before) (after) -- ACTUALLY AFFECTED
twitter-stream-0.1.1 (before) (after) -- ACTUALLY AFFECTED

nikomatsakis · 2017-02-01T22:42:05Z

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.

nikomatsakis · 2017-02-01T22:44:27Z

@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);
}

eddyb · 2017-02-01T22:45:21Z

I think the issue in question is #39297.

canndrew · 2017-02-03T11:18:16Z

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.

canndrew · 2017-02-03T11:42:36Z

@nikomatsakis

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.

nikomatsakis · 2017-02-03T20:49:56Z

@canndrew

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.

nikomatsakis · 2017-02-03T20:50:11Z

@bors r+

bors · 2017-02-03T20:50:12Z

📌 Commit 40c9538 has been approved by nikomatsakis

nikomatsakis · 2017-02-03T20:50:23Z

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

bors · 2017-02-04T04:03:56Z

⌛ Testing commit 40c9538 with merge dd7ee9d...

bors · 2017-02-04T04:53:05Z

💔 Test failed - status-travis

canndrew · 2017-02-04T06:19:03Z

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.

alexcrichton · 2017-02-04T22:09:57Z

@bors: r=nikomatsakis

bors · 2017-02-04T22:09:58Z

📌 Commit 42f3ac5 has been approved by nikomatsakis

…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.

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:

bors · 2017-02-05T16:57:05Z

⌛ Testing commit 42f3ac5 with merge 9c8cdb2...

rust-highfive assigned nikomatsakis Jan 12, 2017

nikomatsakis requested changes Jan 17, 2017

View reviewed changes

canndrew mentioned this pull request Jan 29, 2017

fallback for diverging expressions leaks into the ? operator #39297

Closed

canndrew force-pushed the default-unit-warnings branch from 2b10dc2 to 3b3fe61 Compare January 29, 2017 16:47

canndrew added 7 commits February 3, 2017 18:48

Add warning for () to ! switch

2cc84df

Fix make tidy

c570cd6

Hash TyTuple's defaulted flag

5dbaefb

Add is_defaulted_unit helper method

085f046

Use a proper future-compatibility lint

5c90dd7

Fix test

6a99573

Fix test

40c9538

This was referenced Feb 3, 2017

Future-proof against inference changes tesaguri/twitter-stream-rs#2

Merged

Fix for inference changes dovahcrow/sidekiq-rs#2

Merged

canndrew force-pushed the default-unit-warnings branch from 3b3fe61 to 40c9538 Compare February 3, 2017 11:43

nikomatsakis mentioned this pull request Feb 3, 2017

Ignore expected type in diverging blocks #39485

Merged

nikomatsakis approved these changes Feb 3, 2017

View reviewed changes

canndrew added 2 commits February 4, 2017 14:03

Fix test

7444d07

Expand defaulted unit test

42f3ac5

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 14 pull requests #39551

Closed

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 20 pull requests #39562

Closed

frewsxcv mentioned this pull request Feb 5, 2017

Rollup of 19 pull requests #39563

Merged

bors merged commit 42f3ac5 into rust-lang:master Feb 5, 2017

canndrew mentioned this pull request Feb 21, 2017

Tracking issue for future-incompatibility lint resolve_trait_on_defaulted_unit #39216

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning for () to ! switch #39009

Add warning for () to ! switch #39009

canndrew commented Jan 12, 2017

rust-highfive commented Jan 12, 2017

canndrew commented Jan 12, 2017 •

edited

Loading

durka commented Jan 12, 2017 •

edited

Loading

nikomatsakis left a comment

nikomatsakis Jan 17, 2017

nikomatsakis Jan 17, 2017

canndrew Jan 21, 2017

nikomatsakis Jan 17, 2017

canndrew Jan 21, 2017

canndrew commented Jan 21, 2017

canndrew commented Jan 22, 2017

nikomatsakis commented Jan 23, 2017

canndrew commented Jan 25, 2017

nikomatsakis commented Jan 25, 2017

nikomatsakis commented Jan 25, 2017

bors commented Jan 28, 2017

canndrew commented Jan 29, 2017

bors commented Jan 31, 2017

nikomatsakis commented Feb 1, 2017 •

edited

Loading

nikomatsakis commented Feb 1, 2017

nikomatsakis commented Feb 1, 2017

eddyb commented Feb 1, 2017

canndrew commented Feb 3, 2017

canndrew commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

bors commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

bors commented Feb 4, 2017

bors commented Feb 4, 2017

canndrew commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

bors commented Feb 5, 2017

Add warning for () to ! switch #39009

Add warning for () to ! switch #39009

Conversation

canndrew commented Jan 12, 2017

rust-highfive commented Jan 12, 2017

canndrew commented Jan 12, 2017 • edited Loading

durka commented Jan 12, 2017 • edited Loading

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jan 17, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 17, 2017

Choose a reason for hiding this comment

canndrew Jan 21, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 17, 2017

Choose a reason for hiding this comment

canndrew Jan 21, 2017

Choose a reason for hiding this comment

canndrew commented Jan 21, 2017

canndrew commented Jan 22, 2017

nikomatsakis commented Jan 23, 2017

canndrew commented Jan 25, 2017

nikomatsakis commented Jan 25, 2017

nikomatsakis commented Jan 25, 2017

bors commented Jan 28, 2017

canndrew commented Jan 29, 2017

bors commented Jan 31, 2017

nikomatsakis commented Feb 1, 2017 • edited Loading

nikomatsakis commented Feb 1, 2017

nikomatsakis commented Feb 1, 2017

eddyb commented Feb 1, 2017

canndrew commented Feb 3, 2017

canndrew commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

bors commented Feb 3, 2017

nikomatsakis commented Feb 3, 2017

bors commented Feb 4, 2017

bors commented Feb 4, 2017

canndrew commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

bors commented Feb 4, 2017

bors commented Feb 5, 2017

canndrew commented Jan 12, 2017 •

edited

Loading

durka commented Jan 12, 2017 •

edited

Loading

nikomatsakis commented Feb 1, 2017 •

edited

Loading