impl<T> From<T> for ()

[SimonSapin]

Don’t try to merge this yet.

At the moment this PR causes a compiler error:

../src/libcore/convert.rs:222:1: 224:2 error: conflicting implementations of trait `convert::From<()>` for type `()`: [E0119]
../src/libcore/convert.rs:222 impl<T> From<T> for () {
../src/libcore/convert.rs:223     fn from(t: T) -> () {}
../src/libcore/convert.rs:224 }
../src/libcore/convert.rs:222:1: 224:2 help: run `rustc --explain E0119` to see a detailed explanation
../src/libcore/convert.rs:215:1: 217:2 note: conflicting implementation is here:
../src/libcore/convert.rs:215 impl<T> From<T> for T {
../src/libcore/convert.rs:216     fn from(t: T) -> T { t }
../src/libcore/convert.rs:217 }

The two impls overlap on From<()> for (), even though they happen have equivalent behavior there.

Specialization may help with this, but:

• Only if the lattice rule is implemented
• This would (I think?) also allow users outside of libcore to specialize further.

CC @aturon & libs team

---

I use the try! macro a lot, usually with () as the error type since I don’t care that much about tracking data about errors. When using some existing function or method that returns Result
with a different error type, I find myself using code like this:

try!(foo.map_err(|_| ()))

… which is more verbose than I’d like.

The try! macro already uses the From trait to convert errors, some implementations like impl From<std::num::ParseIntError> for () would help. But rather than adding them one by one to many error types, a generic solution would be nice.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[nagisa]

I feel like this potentially makes code like banana.into() where given banana: T there was a sole implementation of From<T> not compile. Is it really worth it?

[SimonSapin]

Was that ever the case, though? There’s already From<T> for T.

[bluss]

Into implementations shadow each other in a way that's not immediately obvious.

I don't know how to explain that adding a trait bound in foo3_error removes an into conversion that was present in foo2_ok.

(playground)

fn foo1_error<T>(value: T)
{
    let conv = value.into();
    // ^ error: unable to infer enough type information abou
}

fn foo2_ok<T>(value: T)
{
    let conv: T = value.into();
}

fn foo3_error<T>(value: T)
    where T: Into<String>
{
    let conv: T = value.into();
    // ^ error: mismatched types: expected T, found String
}

fn foo4_ok<T>(value: T)
    where T: Into<String>
{
    let conv = value.into();
}

[SimonSapin]

@bluss build error… looks like a bug? Both impls should be available to multi-dispatch, one shouldn’t shadow the other. (But I’m saying this with only superficial knowledge of how dispatch works.)

[bluss]

@SimonSapin I hope it's something that's /not/ fixed since it will introduce ambiguities in lots of places. It would be very bad for the usability of Into. I think it deflects any problem of the type @nagisa brought up.

[eddyb]

@SimonSapin It's very much intentional, where clauses on type parameters are considered "inherent" and thus take priority over other candidates.

I've told @bluss on IRC that they can try to change this line to push to this.extension_candidates instead and see what breaks.
I doubt you can get past libcore, but I could be wrong.

[bluss]

eddyb dug out this https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/method/probe.rs#L471-L539 which has it as explicit behavior. Trait bounds on type parameters are added as if inherent methods on the type, and take precedence.

[bluss]

@SimonSapin Maybe an extension trait and a simple new method is a better way to achieve this, creating something like this:

forgetful_try!(foo);

or this:

try!(foo.forget_err());

with the same effect (mapping to an error type ()).

[arielb1]

@eddyb

I do consider it a bug (inference guessing). Why would fixing it cause issues?

[eddyb]

@arielb1 Well, anything that depends on a where clause to remain unambiguous and ends up being fed into a method call will cause trouble. Should just try the change and see what happens.

[SimonSapin]

@bluss That doesn’t seem better than try!(foo.map_err(|_| ())) enough to justify a new API.

[aturon]

cc @rust-lang/libs

@arielb1 @eddyb This behavior is absolutely intentional, and removing it could cause breakage any time there are blanket impls that might apply as well (and hence yield selection ambiguity). We long ago made an explicit pragmatic choice to allow this kind of influence; another example is http://smallcultfollowing.com/babysteps/blog/2014/09/30/multi-and-conditional-dispatch-in-traits/ (cc @nikomatsakis)

That said, people routinely call into even when there aren't explicit where clauses, and this PR could cause ambiguity in those cases (assuming that the existing blanket impl was otherwise ruled out). The only way to know for sure is a crater run.

@SimonSapin The conflict you mention between impls does not require the lattice rule to resolve -- it works with vanilla specialization. However, we aren't introducing such public uses of specialization in std yet.

[nikomatsakis]

It's something of an aside, but the point that @bluss raises in this comment is basically a shortcoming of the current trait solver, which -- if where clauses are present -- uses those to guide decisions in way that may cause it to overlook impls that could apply (but which also is helpful in avoiding errors in other cases). This is a downside of the flexibility of the trait system (the upside is that we can permit many kinds of impls we couldn't otherwise). I do hope at some point to revise the way trait matching works to make it more capable of handling such cases, though that probably carries risks of longer compilation times.

[nikomatsakis]

Ah, well, I see that there were points raised about the fact that where clauses are treated as inherent. That is also a factor (and may be the dominant factor in this particular example). That behavior dates way back. At some point pre-1.0 @aturon and I were contemplating removing it, but I think it's too late now. The main motivator was cases like:

fn foo<T: some_mod::SomeTrait>(x: &T) {
    /* now I can call methods of SomeTrait, even thogh it is not imported into scope */
}

In any case, with respect to this behavior as well as what I mentioned in my previous comment, I still do hold out hope we'll be able to teach the compiler to be smarter, but it is certainly tricky to maintain compatibility. It may be the kind of thing where we have to carefully plan a transition if we really want to address it (it's not clear to me yet that we do).

[arielb1]

@aturon

@SimonSapin The conflict you mention between impls does not require the lattice rule to resolve -- it works with vanilla specialization. However, we aren't introducing such public uses of specialization in std yet.

The 2 conflicting impls of Into are not comparable to one another. I don't see how ordered specialization can help here?

Maybe if you do some crap with associated types.

@arielb1 @eddyb This behavior is absolutely intentional, and removing it could cause breakage any time there are blanket impls that might apply as well (and hence yield selection ambiguity)

What is intentional is that if only a single impl/clause is available it is selected even if there are inference variables - the "crate-concatenability-breaking" part. What I consider a bug (I even wrote that in a comment in traits::select) is that rustc picks a where-clause even if there are impls available for a different assignment to the type variables.

[arielb1]

For example:

use std::ptr;
trait Foo { fn foo(&self) {} }
impl Foo for *const u8 {}
impl<T: Fn()> Foo for *const T {}

fn ambig() {
    ptr::null().foo();
    //~ ERROR unable to infer enough type information about `_`
    // commenting one of the impls out makes it work
}

fn inambig<T>() where *const T: Foo {
    ptr::null().foo();
    // this works (but is just as ambiguous as the previous example)
}

fn main() {}

[nikomatsakis]

On Sat, Apr 23, 2016 at 05:16:29AM -0700, arielb1 wrote:

What is intentional is that if only a single impl/clause is
available it is selected even if there are inference variables - the
"crate-concatenability-breaking" part. What I consider a bug (I even
wrote that in a comment in traits::select) is that rustc picks a
where-clause even if there are impls available for a different
assignment to the type variables.

I would consider this a...known limitation. That is, we certainly
chose this behavior somewhat affirmatively, because otherwise we were
encountering ambiguities in some scenarios. Mainly I think if we were
going to change things here, that would be ok, but we'd have to be
careful about producing new errors (e.g., ambiguity errors) in cases
where things used to compile.

[nrc]

In a future world where ? allows conversion between different carriers, this becomes even more important for the expected 'subtyping' behaviour.

[brson]

This bothers me.

Being able to convert anything to () - by just discarding the value - seems weird, and against the spirit of a "conversion". Is there any other use case besides the convenience in the OP? Seems like a potential source of errors.

Furthermore, as it stands I consider using () for errors to be an anti-pattern - because () does not implement std::error::Error. When you use () for an error type then it is not compatible with other error types and code expecting errors to implement Error need to consider your error specially. (I've run into this with url specifically). Maybe () should implement Error of course for the convenience, but even then I'm not comfortable with what this patch is trying to do.

[brson]

Just as one hypothetical example of how this could go wrong, imagine an old code base with a big function that uses Result<_, ()>. During maintenance some new programmer does the normal thing of using try! for some new method call, not realizing that the conversion discards error information. Maybe it was the original authors intent that future revisions to this function always discard their errors, but I wouldn't bet on it, since () is a typical 'stub' value to use before you've thought hard about what a function should do.

[nrc]

To expand on why I want this, in a world where ? works with Option, etc., it would be nice to have a value of type Result<T, E> and use ? on it in a function with return type Option<T>. For that to work we have to be able to convert Result to Option, which, in any implementation I can think of, requires us converting the E to ().

Now you could argue that you shouldn't be using ? to convert between Result and Option like this, and that seems a reasonable position. But I've wanted this and not having it means falling back to unergonomic match where we are in a situation which (at least to me) should get the ergonomic benefits of the ? error handling.

Anyway, this seems like a discussion worth having, but maybe once we've got some experience with the new error handling stuff?

[nikomatsakis]

On Mon, May 02, 2016 at 09:55:33PM -0700, Nick Cameron wrote:

To expand on why I want this, in a world where ? works with Option, etc., it would be nice to have a value of type Result<T, E> and use ? on it in a function with return type Option<T>. For that to work we have to be able to convert Result to Option, which, in any implementation I can think of, requires us converting the E to ().

I don't know; I think I would expect to have to make this explicit, e.g. via result.ok()?.

[alexcrichton]

The libs team discussed this PR during triage today and the conclusion was that we don't want to merge this at this time. It seems like an antipattern to discard error information and also to use () in an error type of Result as it doesn't implement Error nor does it allow for future extensibility to add more contextual information.

We discussed the idea of having a useful "universal error type" that could be easily used, and @sfackler pointed out that this is largely Box<Error> today but we could also perhaps add more helpers to std::error which crucially do not lose information.

In the meantime thought we concluded that we would like to not pursue this particular strategy, so I'm going to close, but feel free to reopen or submit another PR with a different approach!