Allow where clauses involving types which don't include a type parameter.

[eefriedman]

There isn't any semantic reason to disallow a where clause based on the fact that it doesn't contain a type parameter.

r? @nikomatsakis

[bors]

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

[arielb1]

Be careful not to cause an ICE when translating a function that contains a bogus bound:

#[no_mangle]
pub fn bogus<'a>(x: &'a mut ()) where &'a mut (): Clone {
    <&'a mut ()>::clone(&x);
}

[jroesch]

The original RFC specified this behavior, but I think we all agree this is a fine change.

[nikomatsakis]

Yes, I agree this is reasonable, but I think we may not want to just lift the restriction without possibly placing other restrictions. I had intended to write an RFC at some point, though it may be overkill. My thought is that we should make a best-effort to validate where-clauses, which means that if you define a function that contains a where-clause which is not satisfiable (e.g., NotHash: Hash), then you should get a compilation error on that function (and possibly callers of that function). We should make an effort to do this validation in a first-pass so that you first see the error on the fn with the invalid where-clause, and not on its callers. Similarly, we have to be careful about the issues that @arielb1 has highlighted.

This is probably fairly easy to implement. The existing ParameterEnvironment code already expands out the implications of all where clauses and then drops those that do not involve type variables (to avoid polluting caches with invalid values). I imagine we would do a similar process, but instead of dropping those clauses on the floor, we would try to validate them.

[nikomatsakis]

cc @rust-lang/lang

[nikomatsakis]

Nominating for "needs decision" by the @rust-lang/lang team.

[eefriedman]

If I'm understanding correctly, the suggested rule is something along the lines of "all where clauses which don't contain a type parameter must be satisfiable if you replace all generic lifetimes with 'static"? That seems reasonable...

[arielb1]

I would prefer to check for satisfiability in intercrate mode after you instantiate type parameters by type variables.

[eefriedman]

We already validate where clauses to an extent...

trait ZZ {}
fn _f() where std::fmt::Display : ZZ {}
fn main() {}

<anon>:2:1: 2:51 warning: the trait `ZZ` is not implemented for the type `core::fmt::Display + 'static` [E0277]
<anon>:2 fn _f<T:'static>() where std::fmt::Display : ZZ {}
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:2:1: 2:51 help: run `rustc --explain E0277` to see a detailed explanation
<anon>:2:1: 2:51 note: this warning results from recent bug fixes and clarifications; it will become a HARD ERROR in the next release. See RFC 1214 for details.
<anon>:2 fn _f<T:'static>() where std::fmt::Display : ZZ {}
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<anon>:2:1: 2:51 note: required by `ZZ`
<anon>:2 fn _f<T:'static>() where std::fmt::Display : ZZ {}
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Unfortunately, the current check completely ignores anything which includes a lifetime, for example:

trait ZZ<'a> {}
fn _f<'a>() where std::fmt::Display : ZZ<'a> {}
fn main() {}

I have no idea how to fix this.

[arielb1]

is this on a patched compiler?

[eefriedman]

@arielb1 This is with just db9e395. (On master, both of those examples trigger E0193.)

[eefriedman]

I think #27987 is related.

[nikomatsakis]

Sorry for the delay here. This got overlooked in our lang team triage due to the silliness that our filter was looking for issues, not PRs. I think we DID discuss it at some point anyhow, in fact, but none of us can recall the decision that was reached.

[nikomatsakis]

OK, so, sorry this took forever, but we talked about it in the lang team mtg and came to the conclusion that it is fine to lift this rule, and it does not have to be an error if a where clause is unsatisfied. Unfortunately, my notes from the discussion are kind of insufficient and I cannot remember all the reasoning that led to this conclusion. Certainly issuing hard errors is backwards incompatible, and feels in some sense a bit artificial, since in general we assume that where clauses hold, so this would require special code. I seem to recall that we realized that there were other equally invalid cases where we couldn't issue errors, so it was ok for such where clauses to be accepted, and we can always issue lint warnings later etc. @rust-lang/lang anybody remember more details?

[nikomatsakis]

I'm removing the I-nominated and I-needs-decision tags, in any case.

[nikomatsakis]

@eefriedman I'll happily r+ the patch if you rebase.

[arielb1]

@nikomatsakis

What should be done to cover this case:

#[no_mangle]
pub fn bogus<'a>(x: &'a mut ()) where &'a mut (): Clone {
    <&'a mut ()>::clone(&x);
}

I imagine a patch would require this not ICE-ing.

[nikomatsakis]

@arielb1 Ah yes, good question. Your earlier comment got overlooked, thanks for bringing it up again. I guess I can see three options:

1. Report error at type-check time, contradicting what I just wrote.
2. Report error at trans time.
3. Generate a panic, as the call ought to be unreachable.

Seems in some way analogous to matching on an empty enum -- what do we do in that case again?

[nikomatsakis]

What the hey, i'll nominate again to think this over :)

[arielb1]

I think generating an LLVM unreachable would be the right thing (but I also think this would be right for bad transmutes).

[nikomatsakis]

From a brief look, it appears that matching on an empty enum invokes LLVM unreachable, so I think I'm ok with that, just for consistency sake (though I don't particularly like that we use it for an empty enum). In the @rust-lang/lang meeting, @huonw mentioned something about an LLVM instruction that generates an illegal instruction, but I couldn't find any such thing (maybe I didn't look hard enough). @huonw any links?

In general, some kind of failure or fault seems better than arbitrary undefined behavior -- but I think the same holds for match. One difference between match and this case is that I imagine a match might occur in the midst of some other fn, and using unreachable may permit better optimization, but in this case it's kind of bogus even to enter the fn at all.

Anyway I think ultimately I would prefer to handle all such "logically impossible" cases in the same way.

[eefriedman]

The intrinsic to generate a trap is http://llvm.org/docs/LangRef.html#llvm-trap-intrinsic . Whether it makes sense to generate trap vs. just unreachable is basically just a question of how much hand-holding we want to do for unsafe code.

[arielb1]

@nikomatsakis

What's your problem with matching on an empty enum being an unreachable? It's not like it is easy to accidentally match on such an enum, and the range metadata we emit can totally turn to an unreachable in other "invalid discriminant" cases.

That would however require adding an unreachable terminator to MIR.

[nikomatsakis]

On Fri, Oct 23, 2015 at 01:03:58PM -0700, arielb1 wrote:

@nikomatsakis

What's your problem with matching on an empty enum being an unreachable?

My problem is that it results in undefined behavior if somebody makes
a mistake somewhere. I prefer to be more robust against human error.
I find this a hard line to draw. The same applies to a number of
cases, though -- e.g. today every match on an enum makes the
assumption that the variant is valid, which can easily be violated.

[arielb1]

@nikomatsakis

how could one match on an empty enum by accident? I think assuming that variants in enums are valid makes sense.

[nikomatsakis]

@arielb1 one could create an instance of an empty enum by accident, or have the enum being empty by accident. Basically something went wrong in your logic if we go to that point, clearly, so doing undefined things doesn't seem like it's going to help anybody.

[pnkfelix]

/me suggests emitting a trap for such matches in debug-builds and an unreachable in release-builds.

[nikomatsakis]

On Thu, Oct 29, 2015 at 02:38:37PM -0700, Felix S Klock II wrote:

/me suggests emitting a trap for such matches in debug-builds and an unreachable in release-builds.

I could get behind this, though I might prefer a panic in debug builds
and a trap in release builds, but I think at this point we're beyond
the scope of this PR thread. We probably ought to open this general
question up for wider discussion. I'd also like to try to come up with
a more comprehensive list of places that we intentionally introduce
undefined behavior on the premise that certain code paths cannot
occur. Seeing the full list may inform the decision.

[arielb1]

@nikomatsakis

Anyway, a match on an empty enum is occasionally used to intentionally introduce an unreachable. IIRC for sane codegen we must allow matches on enums to assume the discriminant is valid (we emit range metadata, and this is equivalent to an unreachable in some cases). We also emit notnull for references.

Creating an instance of an empty enum is not particularly easy.

[nikomatsakis]

@arielb1 I know it is used that way; the question I think is whether a trap would be equally effective for optimization (and if not, what kind of impact we are talking about)

[steveklabnik]

What is the status of this PR?

[nikomatsakis]

We talked about this in the @rust-lang/lang meeting but didn't reach a firm conclusion. I think at this point probably the most appealing option is to permit such where-clauses but report errors if they don't hold (at the definition site). Yes, this is the opposite of what I wrote before, but it sidesteps @arielb1's example, and there isn't really a compelling reason to go out of our way to permit such a fn. Furthermore, if we ever encounter such a reason, it is backwards compatible to permit such where-clauses.

The only hazard is that this might break some code, given that it will presumably also apply to all elaborated where clauses. I'd be surprised, though, if that affected any real code in practice. In any case it seems that RFC 1214 is already enforced some of these regulations (but I think not all, right?). I'd have to go back over and check more carefully, can't remember off the top of my head.

[nikomatsakis]

@eefriedman are you interested in trying to implement the "report errors at definition site if the where-clauses don't hold" semantics? I think it would be relatively straightforward -- there is already some code that is careful not to add such where-clauses into the parameter environment, presumably it could be made to return those clauses and then we could check them instead (or something like that, in any case).

[nikomatsakis]

Closing in favor of issue #21203