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

Stabilize never_type *again* #57012

Open
cramertj opened this Issue Dec 20, 2018 · 50 comments

Comments

Projects
None yet
8 participants
@cramertj
Copy link
Member

cramertj commented Dec 20, 2018

#49593 has now been fixed. This was the reason for the previous revert of stabilization (#50121). Previous stabilization report is [here]. Let's try this again!

@rfcbot fcp merge
cc #35121

(pnkfelix notes :the previous issue discussing stabilization of ! was #48950; its possible the [here] above was supposed to link to that...)

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 20, 2018

@rfcbot fcp merge

c'mon, rfcbot!

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 20, 2018

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and none object), 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.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 20, 2018

@rfcbot concern impls

The remaining unchecked checkbox in the description of #35121 is:

What traits should we implement for !? The initial PR #35162 includes Ord and a few others. This is probably more of a T-libs issue, so I'm adding that tag to the issue.

Stabilizing ! and then (in a) later (release cycle) adding a new impl of existing traits is not a breaking change, is it? If not, this doesn’t need to be a blocker and let’s mark this as resolved immediately.

If it is, let’s discuss it in this issue. Currently in https://doc.rust-lang.org/nightly/std/primitive.never.html we have:

impl Hash for !
impl Copy for !
impl Debug for !
impl Clone for !
impl PartialOrd<!> for !
impl Ord for !
impl Eq for !
impl PartialEq<!> for !
impl Display for !
impl Error for !
impl Termination for !

@rust-lang/libs, anything to add?

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Dec 20, 2018

@SimonSapin it's not a breaking change AFAIK, so we can add them over time.

I'm not sure if we should implement traits for !. Under some coherence related proposals, its possible to write impls guaranteeing that a type will never implement a trait - negative impls, in other words. I don't know the pros and cons of providing positive vs negative impls for ! (which could go either way) if that feature were added to the language.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 20, 2018

@rfcbot concern fallback

In addition to reverting the stabilization, #50121 also reverted “to the previous fallback semantics (i.e. it is once again dependent on whether the crate has opted into #[feature(never_type)]”.

As far as I understand, this is about type inference of a diverging expression like panic!(…), where there is no constraint from surrounding code.

When it previously landed in Nightly, we found out #35121 (comment) that the fallback change could cause some previously-valid code to not compile anymore: #48950 (comment), and some (other) previously-valid unsafe code to now have Undefined Behavior #48950 (comment) (returning Result<!, _>::Ok in reachable/reached code).

@SimonSapin

This comment was marked as resolved.

Copy link
Contributor

SimonSapin commented Dec 20, 2018

Is this a duplicate of #48950?

@cramertj

This comment was marked as resolved.

Copy link
Member

cramertj commented Dec 20, 2018

@SimonSapin That issue was created during the first round of stabilization to capture the conversation around that issue. This is the new issue for the new round of stabilization.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Dec 20, 2018

for<E> E: From<!>

Is nice for error handling, but I recall there are orphan/overlapping issues adding it later. I think this is because that impl is technically an overlapping impl with for<A> A: From<A>, so downstream impls that would overlap with it aren't orpans. But if we somehow hack that into core, then the downstream impls can't exist.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 20, 2018

@Ericson2314 I agree that impl<T> From<!> for T would be nice to have. Indeed, if we ever add it, that needs to happen before or in the same release cycle as stabilization of !. And as you said, we can’t "just" add it because it overlaps with impl<T> From<T> for T.

Adding "some hack" to bend the language rules and somehow allow both these impls (picking an arbitrary one to "win" is ok, since they behave the same at the intersection) has been discussed before in the abstract, but there has never been a concrete proposal. Concretely, this would need:

  • A plan for how this would work in terms of compiler implementation
  • Some buy-in from the language team that bending the language rules in libcore is not a non-starter
  • Someone who volunteers to implement it

Without all of these things soon, I suspect that consensus would be to give up on impl<T> From<!> for T and stabilize ! without it.

@Centril Centril removed the I-nominated label Dec 20, 2018

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 20, 2018

Noting so I/we remember from this week's T-lang meeting:

@rfcbot concern fundamental-and-fn-traits

We had some discussion around Fn* traits (which are #[fundamental]) and !. (Are there other fundamental traits?) @cramertj and @withoutboats had some ideas that would be nice to hear more in-depth about.

@rfcbot concern more-elaborate-report

It would be nice to have all the behavior changes more clearly specified in one place before we stabilize so it's clear what is being stabilized.

@rfcbot concern from-impl

Noting this as a concern temporarily until conversation has resolved itself.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 20, 2018

When it previously landed in Nightly, we found out that the fallback change could […]

I meant to add: in #35121 (comment) we discussed only making the fallback change in a new edition. We missed the 2018 train, so this would be for 2021 (or whenever we’ll make the next one).

Also, in the #48950 (comment) case, fallback occurred in the expansion of a macro_rules macro. But macros are not edition-hygienic, are they?

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Dec 21, 2018

Adding "some hack" to bend the language rules and somehow allow both these impls (picking an arbitrary one to "win" is ok, since they behave the same at the intersection) has been discussed before in the abstract, but there has never been a concrete proposal. Concretely, this would need:

I don't know whether this would actually be easier to implement than the "some hack" itself which would allow the From impls to coexist, but another option which could unblock this question for now, without forever giving up on the right to add the From<!> impl, would be to add a temporary hack which just forbids downstream crates from writing any impls which would conflict with it (as-if the impl did exist), without actually adding the impl itself in core yet.

To try to avoid confusion I'll call the (hypothetical) hack which allows From<!> and From<T> to coexist the "overlap hack", and the hack which merely forbids downstream from conflicting with it the "future-proofing hack".

Even if it's not easier to implement, the future-proofing hack requires fewer commitments -- we don't have to commit to the From<!> impl existing and being stable (IINM, impls are still insta-stable, exacerbating the whole issue), we don't have to commit to the overlap hack existing and continuing to work in the future, we can revert the future-proofing hack at any point if we decide it's not worth it.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 21, 2018

But the future-proofing hack blocks a useful use case.

For the same reason that impl<T> From<!> for T is desirable, if it’s not implemented then impl From<!> for Foo might be desirable as the next best thing.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Dec 21, 2018

Yes, which is why it's good that we can choose to revert it at any time.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 21, 2018

An alternative in the future would be to add the impl but ignore coherence completely for any From<!> impl and just allow overlap, since all possible implementations are equivalent (we could potentially do the same for any traits where all functions have uninhabited arguments, and there are no non-function associated items).

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 21, 2018

I like the idea of tweaking coherence not with a special case for a particular pair of impls but with a rule about allowing overlap when the intersection only has unreachable functions. There is somewhat similar precedent in https://rust-lang.github.io/rfcs/1268-allow-overlapping-impls-on-marker-traits.html, although like in that case we might want to make it opt-in.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 21, 2018

@Centril

concern fundamental-and-fn-traits
We had some discussion around Fn* traits (which are #[fundamental]) and !. (Are there other fundamental traits?) @cramertj and @withoutboats had some ideas that would be nice to hear more in-depth about.

TL;DR: We shouldn't implement the Fn traits for !, because they have an associated type and we'd have to choose what it is. One option would be to choose !, but there's no real logical / fundamental reason for this. Instead, providing this implementation would actually prevent users from implementing custom traits for ! where they also wanted to add a blanket impl over all T: Fn.... This seems like a potentially useful thing to do, whereas using ! as a Fn...<...> -> ! or Fn...<...> -> () seems not particularly useful.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Dec 22, 2018

@SimonSapin I really like that idea! We could use the same logic that deduces there is only one possible impl to also allow elliding the trait items:

impl<T> From<!> for T;
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 25, 2018

#57012 (comment)

it's not a breaking change AFAIK, so we can add them over time.

@rfcbot resolve impls


@cramertj How do you feel about making this issue explicitly not propose any change to type inference fallback from () to !, and leave that change to future discussions? This would resolve my "fallback" concern: #57012 (comment)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 25, 2018

#57012 (comment)

I like the idea of tweaking coherence not with a special case for a particular pair of impls but with a rule about allowing overlap when the intersection only has unreachable functions.

I believe this would be compatible to add later, after the never type is stabilized. And so this doesn’t need to block this FCP. @Centril, does this resolve your from-impl concern?

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Dec 25, 2018

@SimonSapin ah so to be clear, if we have Foo: From<!>, and then we add the blanket impl, the Foo impl becomes a harmless overlap (even if it is fully subsumed) by the same analysis.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Dec 26, 2018

Good point! Glad to see this hit stable!

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 26, 2018

@Centril

concern more-elaborate-report

It would be nice to have all the behavior changes more clearly specified in one place before we stabilize so it's clear what is being stabilized.

That was done here, and is linked above-- sorry if it was hard to find.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 26, 2018

This includes “Type inference will now default unconstrained type variables to ! instead of ()”, which as already mentioned in this thread I think we should not do without some mitigation, including at least only changing the default in a future language edition.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Dec 26, 2018

@SimonSapin Yeah, it does include it-- my intention in this issue was to include that. I think that this behavior change is correct and that it doesn't make sense to do as part of an edition change-- having different type fallback behavior across editions seems really complicated and potentially perplexing to end-users (e.g. why does my code silently have different runtime behavior when I change this Span from call_site to def_site). If we're ever going to make this change (and I think we should), then I think we should do it now and for all editions.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 26, 2018

TL;DR: We shouldn't implement the Fn traits for !, because they have an associated type and we'd have to choose what it is. One option would be to choose !, but there's no real logical / fundamental reason for this. Instead, providing this implementation would actually prevent users from implementing custom traits for ! where they also wanted to add a blanket impl over all T: Fn.... This seems like a potentially useful thing to do, whereas using ! as a Fn...<...> -> ! or Fn...<...> -> () seems not particularly useful.

@rfcbot resolve fundamental-and-fn-traits

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 26, 2018

If we're ever going to make this change (and I think we should), then I think we should do it now and for all editions.

Ok, then we are left with my concern from #57012 (comment). What do we do about code that used to compile but doesn’t anymore with this change? About unsafe code that used to be sound but has UB with this change?

I spent days in that particular debugging rabbit hole.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

Do we want to reopen #49039? cc @scottmcm

@rfcbot concern 49039-needs-decision


@SimonSapin

I believe this would be compatible to add later, after the never type is stabilized. And so this doesn’t need to block this FCP. @Centril, does this resolve your from-impl concern?

Our rules around coherence are not simple, so extending them needs careful thought. For now, in the absence of stable specialization, I think a better solution would be akin to what @glaebhoerl suggested and which @scottjmaddox has written about. I think this would also be useful on other occasions as well, so we should imo add some #[rustc_reserved] impl<T> From<!> for T {}.

If we're ever going to make this change (and I think we should), then I think we should do it now and for all editions.

I agree with @cramertj; Having tyvars fallback to () does seem like a singularly poor choice as @nikomatsakis put it.

Ok, then we are left with my concern from #57012 (comment).

What do we do about code that used to compile but doesn’t anymore with this change?

It's unfortunate, but I think we should fix what I can only call a bug in the type system. It's also explicitly noted in RFC 1122 that changes to type inference and fallback are considered acceptable (tho certainly we should be mindful of impact). The language team already decided once that it was OK with the change and I see no reason to change our minds.

About unsafe code that used to be sound but has UB with this change?

I spent days in that particular debugging rabbit hole.

For the UB, it seems to me that we could have a correctness lint akin to #39216 in the compiler or in clippy to catch these. Maybe we could limit the lint to unsafe { .. } blocks?


@cramertj

That was done here, and is linked above-- sorry if it was hard to find.

Thanks for the link; I did read that one but didn't find it satisfactory. Is that summary issue fully up to date? After reading the entirely of the RFC, tracking issue, and summary issue, ideally I'd still like a report in this issue that includes:

  • Everything in #48950.
  • Divergences from the RFC and why.
  • Locations of all relevant test files (run-pass, compile-pass, compile-fail, ui...). In particular, test files for fallback seems important.
  • A more exactly specified behaviour of what unification variables default to !.
    For example, None doesn't fallback to Option<!>. A rationale for this would be helpful.
    Also, after stabilizing, are there any unification variables falling back to ()?
    cc @nikomatsakis
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 28, 2018

concern 49039-needs-decision

The decision to do this was already made, only its execution was delayed. We need to make sure to land it in the same release cycle as stabilization of ! (I’m not sure where to track that exactly) but that happens after FCP, so it shouldn’t be a concern that blocks FCP from starting.

Maybe we could limit the lint to unsafe { .. } blocks?

In the #48950 (comment) case, the fallback occurred outside of any unsafe block. (The inferred type that changed from () to ! was used as the return type of a generic FFI call: https://docs.rs/objc/0.2.5/objc/macro.msg_send.html. The crate’s documentation suggests using let _: () = msg_send!(…); to force the return type, but many users didn’t.)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 28, 2018

The decision to do this was already made, only its execution was delayed. We need to make sure to land it in the same release cycle as stabilization of ! (I’m not sure where to track that exactly) but that happens after FCP, so it shouldn’t be a concern that blocks FCP from starting.

@SimonSapin ah; alright, just wanted to make sure we were aware / it didn't slip. :)
@rfcbot resolve 49039-needs-decision

In the #48950 (comment) case, the fallback occurred outside of any unsafe block. (The inferred type that changed from () to ! was used as the return type of a generic FFI call: https://docs.rs/objc/0.2.5/objc/macro.msg_send.html. The crate’s documentation suggests using let _: () = msg_send!(…); to force the return type, but many users didn’t.)

Alright, seems like we should lint outside of unsafe blocks as well then.

@Centril Centril added I-nominated and removed I-nominated labels Jan 3, 2019

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 8, 2019

@Centril

concern from-impl

Context: this is in reference to the fact that we cannot currently add impl<T> From<!> for T { ... } due to overlap with impl<T> From<T> for T { ... }, which will eventually (very likely) be resolved through specialization. Landing now without this would allow users to write impl From<!> for MyType { ... } which would overlap with a future addition of the above blanket impl.

Above we've outlined two broad solutions to this problem:

  • Specifically ban all implementations of From<..> for ! with the compiler until we can add the blanket impl. This has the very unfortunate side-effect that users would be unable to implement From<!> for MyType anywhere, which is a very useful impl to have for e.g. error type conversion and the like. This would make ! much less useful than a custom enum Never {} with the appropriate From impls, so I don't think we should do this if we can avoid it.

  • Allow a future implementation of impl<T> From<!> for T to not conflict with the existing impls. Since the From trait has only one associated item (a function) and it is un-callable because its argument is uninhabited, all implementations of From<!> are equivalent. A variant of this would be to tweak coherence in this way not just for From<!>, but for all trait impls that only provide unreachable functions (as determined by uninhabited argument types). This would be possible to add later, requires no additional future-proofing, and has additional use-cases beyond those strictly scoped to this feature.

I believe that we should go ahead and stabilize the feature with no additional future-proofing and with the second option as plan-of-record.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 8, 2019

@SimonSapin

concern fallback

As you noted in #57012 (comment), this feature tracks not only the stabilization of the ! type, but also the inference change to make diverging expressions such as panic!, return, break, etc. fall back to the ! type.

When this change happened previously, it unfortunately caused some previously-working code to stop compiling, and caused some unsafe code to become undefined behavior. Before I address why I think we should go ahead and do this, there's some justification required as to whether this is allowable under our backwards-compatibility rules or not. I believe that this is covered under the existing rule for "under-specified language semantics" in RFC 1122, specifically the part that says "details of type inference may change"-- it even specifically cites another type of inference fallback (integer fallback) as an area in which we may change the behavior in the future. I wasn't able to find any documentation stating that these type variables should fall back to !, so this seems like the same old allowable but unfortunate inference breakage. Some cases of this issue have been linted against for a while under the resolve_trait_on_defaulted_unit warning (see #39216 for more info).

As for why we should make this change, the RFC lists a couple of motivating factors, the most relevant of which is (IMO) better dead-code detection, such as in cases like this:

let t = std::thread::spawn(|| panic!("nope"));
t.join().unwrap();
println!("hello");

Under this RFC: the closure body gets typed ! instead of (), the unwrap() gets typed !, and the println! will raise a dead code warning. There's no way current rust can detect cases like that.

This change makes it possible to catch more bugs via dead-code warnings, expand the reachability analysis of the compiler allowing for smarter optimizations, and helps to provide "more correct" types like Result<(), !> rather than Result<(), ()> in more places (which may turn previously manual user assertions into no-ops). It also helps introduce the user to the concept of ! by giving a "more correct" return type to diverging expressions.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 8, 2019

Taking @cramertj's notes in #57012 (comment) into account I think I've come to the realization that the future proofing hack doesn't buy much. Either impl<T> From<!> for T will become available to us via specialization and the proofing doesn't affect that, or we'll use the coherence "hack". A third solution doesn't seem likely and as such:

@rfcbot resolve from-impl

Still, having a #[rustc_reserve] mechanism may prove useful in other circumstances but it need not block never_type's stabilization.

@glaebhoerl

This comment has been minimized.

Copy link
Contributor

glaebhoerl commented Jan 8, 2019

(I agree that since the actual coherence hack turns out to be one which can be done backwards compatibly, there isn't much motivation for future-proofing.)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 8, 2019

@cramertj I’m not disputing that the inference fallback change is desirable or that it’s allowed by the our documented stability rules. This concern is about: how do we deal with the real negative impact on the ecosystem? Last time around, we pretty much didn’t. One option that I proposed above is to only make the change effective in a future language edition. This seems to be precisely what editions are for, but maybe we could manage in some other way. (I also proposed separating this discussion from stabilization of the never type which would unblock other features, but there doesn’t seem to be interest in that.)

Are you proposing that we make the change in Rust 1.33 (or some other version) and don’t do anything else to help the ecosystem through the transition? Why is that more desirable than, say, gating on an edition?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 8, 2019

@SimonSapin Ah, thank you for clarifying your question!

Are you proposing that we make the change in Rust 1.33 (or some other version) and don’t do anything else to help the ecosystem through the transition?

We already went to some effort to help the transition go more smoothly by introducing the resolve_trait_on_defaulted_unit warning (see #39216 for more info). I'm not proposing any additional mechanisms to help the ecosystem transition, although perhaps a crater run with that lint set to deny could help? If that's something you're interested in, I'd be happy to put up a PR setting the lint to deny so we can try it out if it hasn't been done already. Still, I'd prefer not to block this issue on the completion of that experiment, since it's just a best-effort attempt to help people correct their code. (Edit: there was already a crater run done when the lint was set to deny, which is already its current default, and has been since July 2017-- see #42894).

Why is that more desirable than, say, gating on an edition?

I mentioned this a bit above, but I don't believe that we should use editions to modify inference behavior that is quite this subtle. I'm not even sure how we would do it-- in this world, type-checking would change how it worked depending on the edition of the particular Span it was adjusting. This seems likely to result in subtle and hard-to-debug issues, compared to introducing a consistent behavior everywhere. It's unfortunate that it's inconsistent across time, but I don't think we'd gain much by making this behavior edition-aware, and I think doing so could both make the compiler's logic more complex and frustrate users' ability to reason about the semantic behavior of code with imprecise Spans. There's also the fact that the next edition won't be released for several years, and I'd like to get rustaceans used to the new behavior as soon as possible, which will help introduce them to the ! type and prevent more soon-to-be-broken code from being written.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 8, 2019

@Centril

I'd still like a report in this issue that includes:
Everything in #48950.
Divergences from the RFC and why.
Locations of all relevant test files (run-pass, compile-pass, compile-fail, ui...). In particular, test files for fallback seems important.
A more exactly specified behaviour of what unification variables default to !.
For example, None doesn't fallback to Option<!>. A rationale for this would be helpful.
Also, after stabilizing, are there any unification variables falling back to ()?
cc @nikomatsakis

What is being stabilized

What is not being stabilized

Exhaustive pattern-matching for uninhabited types. eg.

let x: Result<u32, !> = ...;
let Ok(y) = x;

This code will still complain thatOk(_)is a refutable pattern. This can be fixed by using the exhaustive_patterns feature gate. See RFC 1872 for progress on this issue. See https://github.com/rust-lang/rust/tree/master/src/test/ui/feature-gate-exhaustive-patterns.rs for the testcase which confirms that this behaviour is still gated.

Divergence from the RFC

I reread the RFC and didn't find any significant divergence, although the end of the detailed design section says this:

Add an implementation for ! of any trait that it can trivially implement. Add methods to Result<T, !> and Result<!, E> for safely extracting the inner value. Name these methods along the lines of unwrap_nopanic, safe_unwrap or something.

We have added some of these trait impls and methods, but this stabilization doesn't include any such methods, and the set of trait impls provided for ! is relatively small. This set can be expanded in the future backwards-compatibly.

Unresolved Questions

The RFC included one unresolved question as to whether we should automate creation of traits whose only items are non-static methods. This isn't something that we're doing today, but is something I'd personally be interested in seeing in the future, definitely excluding unsafe traits and perhaps "marker" traits. In any case, this isn't currently RFC'd or implemented and isn't being considered in this round of stabilization.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 8, 2019

@cramertj Thanks for the explanation, this makes sense.

perhaps a crater run with that lint set to deny could help? If that's something you're interested in, I'd be happy to put up a PR setting the lint to deny so we can try it out if it hasn't been done already.

Yes please. Can we make this lint cause errors in dependencies despite Cargo passing --cap-lints=allow?

Also, because most Rust code in the world is not tested (or even reachable) by Crater, it would be nice to provide instructions for projects to run a similar experiment on their own dependency graph. Ideally with an unmodified Nightly build. Would a RUSTFLAGS="-Dresolve_trait_on_defaulted_unit" environment variable override Cargo’s --cap-lints=allow?

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 8, 2019

@SimonSapin #42894 already made the lint deny-by-default, but #39216 suggests the possible next step of making it a hard error. However, a crater run was already done in #42894 and there was only one regression, which was fixed in embali/rjq#1. I doubt that another crater run would turn up anything new since this has been a deny-by-default lint since July of 2017.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 8, 2019

@rfcbot resolve more-elaborate-report
@rfcbot reviewed

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 9, 2019

Do we know if the lint indeed would capture the cases that @SimonSapin pointed out as having occurred in the past? (have those cases been fixed in the meantime...?)

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 9, 2019

@nikomatsakis The issue that broke servo was fixed in tomaka/winit#428. I don't know whether the current lint would have triggered on that code or not (though my impression was that that was the point of the lint?).

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 9, 2019

The instances that we know of have been fixed. The problem with having this lint being a lint is that Cargo silences it in dependencies. For most warnings like "this API is deprecated, you should use that one instead" this makes sense: downstream users can’t easily fix them, and keeping them unfixed has basically no negative consequences. This is not the same.

When we are (potentially) silently introducing UB in previously-valid programs, mitigations should be in a form that is not disabled in dependencies. Servo currently has 462 [[package]] entries in its Cargo.lock, testing them manually one by one is not feasible.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 9, 2019

@SimonSapin If they're on crates.io, then they've already been tested by the crater run.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 14, 2019

That is plainly not true.

Let’s take a recent run for example: https://crater-reports.s3.amazonaws.com/beta-1.32-2/index.html. Out of 50551 crates tested, 6446 (12%) are in "error" state: both "before" and "after" builds failed, and so Crater cannot deduce anything about them.

There could be a number of reasons for this. For example, tomaka/winit#428 that we discussed above was related to macOS-specific usage of the objc crate, but Crater only runs Linux. Some other crates might rely on native dependencies that happen not to be available in Crater’s environment.

Crater is extremely valuable but it can never cover everything, for any definition of "everything". Even if we consider what it does cover a sample, this sampling is not at all random and is full of selection bias. I wish we wouldn’t rely so much on Crater results to make decisions.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Jan 14, 2019

@cramertj So for the purposes of making progress and reaching consensus, even if you think a new crater run wouldn't be beneficial, let's please do a new one anyways along the lines @SimonSapin suggested in #57012 (comment).

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 14, 2019

@SimonSapin The crater run on them happened in #42894, when the warning was moved to deny-by-default. This would have tested direct compilation of all crates on crates.io, so cap-lints is irrelevant here.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 20, 2019

In the diff for #42894, RESOLVE_TRAIT_ON_DEFAULTED_UNIT looks most relevant. When running RUSTFLAGS="-Dresolve_trait_on_defaulted_unit" cargo build in a project with multiple dependencies, I get some output for each local (path dependency) crate but not crates.io dependencies.

This suggests that --cap-lints=allows as passed by Cargo to crates.io dependencies "wins" over -D passed through RUSTFLAGS. This is unfortunate, for this scenario. Is there some other mechanism for denying a given lint in a whole dependency graph?

However in this particular case the output is:

warning: lint `resolve_trait_on_defaulted_unit` has been removed: `converted into hard error, see https://github.com/rust-lang/rust/issues/48950`
  |
  = note: requested on the command line with `-D resolve_trait_on_defaulted_unit`

@cramertj Does this message mean that any code that would be impacted by the fallback change would cause a hard error (not a lint affected by --cap-lints), and have for several release cycles now? This would completely resolve my concern, if the change (this time around) doesn’t change meaning of previously-accepted programs but only makes more programs accepted by rustc.

@cramertj

This comment has been minimized.

Copy link
Member

cramertj commented Jan 21, 2019

@SimonSapin Yup, looking at it now it looks like the lint was already removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment