Skip to content
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

Confusing/incorrect error message with incoherent implementations and async blocks #67651

Closed
syntacticsugarglider opened this issue Dec 27, 2019 · 8 comments · Fixed by #68884
Closed
Assignees
Labels
A-async-await A-diagnostics AsyncAwait-Polish AsyncAwait-Triaged C-enhancement D-confusing T-compiler

Comments

@syntacticsugarglider
Copy link
Contributor

@syntacticsugarglider syntacticsugarglider commented Dec 27, 2019

use std::future::Future;

pub trait Bound {}

pub struct Error;

impl Bound for Error {}

pub struct Wrap;

impl<T: Bound> From<T> for Wrap {
    fn from(input: T) -> Self {
        Wrap
    }
}

impl<T: Into<Wrap>> From<T> for Error {
    fn from(input: T) -> Self {
        Error
    }
}

fn fail() -> impl Future<Output = Result<(), Error>> {
    async move {
        let a: Result<(), Error> = Ok(());
        let a: () = a?;
        Ok(())
    }
}

This fails with type annotations required: cannot resolve Error: std::convert::From<Error> (E0283) instead of the expected conflicting implementations of trait std::convert::From<Error> for type Error (E0119). Changing fail to

fn fail() -> Result<(), Error> {
    let a: Result<(), Error> = Ok(());
    let a: () = a?;
    Ok(())
}

i.e. eliminating the async block leads to the expected error message being produced.

playground demonstration

@jonas-schievink jonas-schievink added A-async-await A-diagnostics C-enhancement D-confusing T-compiler labels Dec 27, 2019
@tmandry tmandry added AsyncAwait-Polish AsyncAwait-Triaged labels Jan 7, 2020
@tmandry
Copy link
Contributor

@tmandry tmandry commented Jan 7, 2020

@Centril said they'd look into minimizing this further.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Jan 7, 2020

Also assigning @nikomatsakis to look into this along with #66312.

@Centril
Copy link
Contributor

@Centril Centril commented Jan 7, 2020

Reduced:

trait From {
    fn from();
}

impl From for () {
    fn from() {}
}

impl From for () {
    fn from() {}
}

fn af() -> impl core::future::Future<Output = ()> {
    async move { From::from() }
}

/*
fn f() -> () {
    From::from()
}
*/

@Centril Centril removed their assignment Jan 7, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 8, 2020

I've been investigating this. I'm not sure I understand yet what is going on, but I'm going to leave a few breadcrumbs as I go. To start, I think part of the problem lies on this line:

return tcx.typeck_tables_of(def_id).node_type(hir_id);

Specifically what is happening here is that we are executing the type_of query on a generator, and this is triggering us to do a full typeck. There is probably a good reason for this, but it's not generally how things are meant to work. This query is meant to be "modular", in that we can create the type for all items without type checking their bodies (this is possible because, for example, Rust requires us to fully specify the type signature for functions). I'm not quite sure why generators behave differently -- closures, for example, do not, as you can see if I give a bit more context:

Node::Expr(&hir::Expr { kind: hir::ExprKind::Closure(.., gen), .. }) => {
if gen.is_some() {
return tcx.typeck_tables_of(def_id).node_type(hir_id);
}
let substs = InternalSubsts::identity_for_item(tcx, def_id);
tcx.mk_closure(def_id, substs)
}

Here, on lines 1398-1399, you can see that for a closure we create a type with "identity substitutions", which means a closure type like Closure<P0...Pn> where Pi is a reference to a type parameter.

(I see that opaque types also behave differently; it seems like tcx.type_of(opaque) is used to fetch the concrete type of an opaque type. I actually think we should make that a distinct query, but I think it's neither here nor there.)

(I'm going to look a bit more at why generators behave this way now, not obvious to me that they must -- @Zoxc may have some details here?)

UPDATE: Based on some exploration of git history, this line goes way back to the original gen branch at least.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 8, 2020

OK, I can confirm that the "naive fix" causes downstream errors, I haven't investigated those much yet:

@@ -1392,12 +1392,12 @@ fn type_of(tcx: TyCtxt<'_>, def_id: DefId) -> Ty<'_> {
         Node::Field(field) => icx.to_ty(&field.ty),

         Node::Expr(&hir::Expr { kind: hir::ExprKind::Closure(.., gen), .. }) => {
-            if gen.is_some() {
-                return tcx.typeck_tables_of(def_id).node_type(hir_id);
-            }
-
             let substs = InternalSubsts::identity_for_item(tcx, def_id);
-            tcx.mk_closure(def_id, substs)
+            if let Some(movability) = gen {
+                tcx.mk_generator(def_id, substs, movability)
+            } else {
+                tcx.mk_closure(def_id, substs)
+            }
         }

         Node::AnonConst(_) => {

I suspect they can be fixed though

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jan 8, 2020

@nikomatsakis Generator types contain the types of the values inside which are live across a yield statement, that is why typeck_tables_of is required there. We could move getting the interior type of the generator into a separate query, but that would only help if the problematic caller of type_of does not actually look at these.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 13, 2020

@Zoxc I don't think that's sufficient reason by itself. Closure types also contain details that result from inference (e.g., the types that their upvars use), but type_of applied to a closure just gives you a fully generic type.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Feb 4, 2020

We hit an instance of this in Fuchsia. Minimal example: (playground)

mod queue {
    pub trait TryMerge {}
    
    impl TryMerge for () {}
    
    pub fn work_queue<W, C, WF>(work_fn: W)
    where
        W: Fn(C) -> WF,
        C: TryMerge,
    {}
}

impl queue::TryMerge for () {}

fn main() {
    queue::work_queue(move |_: ()| {
        async {}
    })
}

Gives

error[E0283]: type annotations needed for `()`

instead of the expected error, which is what you get if you remove the async block:

error[E0119]: conflicting implementations of trait `queue::TryMerge` for type `()`

cc @JakeEhrlich

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 7, 2020
Make the `type_of` return a generic type for generators

Fixes rust-lang#67651.

r? @nikomatsakis
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 8, 2020
Make the `type_of` return a generic type for generators

Fixes rust-lang#67651.

r? @nikomatsakis
@tmandry tmandry added this to Low priority in wg-async-foundations triage Feb 11, 2020
@tmandry tmandry moved this from Low priority to High priority in wg-async-foundations triage Feb 11, 2020
@tmandry tmandry added this to In progress in wg-async work Feb 11, 2020
bors added a commit that referenced this issue Feb 29, 2020
Make the `type_of` return a generic type for generators

Fixes #67651.

r? @nikomatsakis
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 20, 2020
Make the `type_of` return a generic type for generators

Fixes rust-lang#67651.

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this issue Mar 24, 2020
Make the `type_of` return a generic type for generators

Fixes rust-lang#67651.

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this issue Mar 24, 2020
Make the `type_of` return a generic type for generators

Fixes rust-lang#67651.

r? @nikomatsakis
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 24, 2020
Make the `type_of` return a generic type for generators

Fixes rust-lang#67651.

r? @nikomatsakis
wg-async work automation moved this from In progress to Done Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await A-diagnostics AsyncAwait-Polish AsyncAwait-Triaged C-enhancement D-confusing T-compiler
Projects
Development

Successfully merging a pull request may close this issue.

6 participants