Impl Future: lost fact that a closure is FnOnce?


#1

I have the following error (vastly truncated):

error[E0284]: type annotations required: cannot resolve `the closure `copy_changeset::{{closure}}` implements the trait `FnOnce``
   --> scm/mononoke/cmds/blobimport.rs:331:29
    |
331 |         .map(|copy| cpupool.spawn(copy))
    |                             ^^^^^
    |
    = note: required because of the requirements on the impl of `futures::Future` for `futures::AndThen<futures::future::FromErr<std::boxed::Box<futures::Future<Error=mercurial::Error, Item=mercurial::changeset::RevlogChangeset> + std::marker::Send + 'static>, Error>, futures::MapErr<std::boxed::Box<futures::Future<Error=blobrepo::Error, Item=()> + std::marker::Send + 'static>, fn(blobrepo::Error) -> Error {<blobrepo::Error as std::convert::Into<Error>>::into}>, [closure@scm/mononoke/cmds/blobimport.rs:239:23: 243:14 csid:mercurial_types::NodeHash, blobstore:std::sync::Arc<blobstore::Blobstore<GetBlob=std::boxed::Box<futures::Future<Error=Error, Item=std::option::Option<std::vec::Vec<u8>>> + std::marker::Send + 'static>, ValueOut=std::vec::Vec<u8>, Key=std::string::String, Error=Error, ValueIn=bytes::Bytes, PutBlob=std::boxed::Box<futures::Future<Error=Error, Item=()> + std::marker::Send + 'static>> + std::marker::Sync + 'static>]>`

(full errors here)

I have a function which contains a series of combinators including an .and_then() that takes a closure, and then the function returns impl Future<...> for the whole thing. That eventually gets passed into CpuPool::spawn - but the compiler complains that “copy_changeset::{{closure}}” doesn’t implement FnOnce. I assume it means the closure being passed to .and_then(), but I’m not sure why it isn’t implementing FnOnce.

I wonder if being passed through impl Future has erased that bit of type information? What type annotation does is it actually looking for?

The full source for that file is here but I haven’t constructed a standalone example that can be compiled yet.

(That type somewhat pretty-printed is:

    futures::AndThen<
      futures::future::FromErr<
        std::boxed::Box<
          futures::Future<
            Error=mercurial::Error, Item=mercurial::changeset::RevlogChangeset
          > + std::marker::Send + 'static
        >,
        Error
      >, 
      futures::MapErr<
        std::boxed::Box<
          futures::Future<
            Error=blobrepo::Error, Item=()
          > + std::marker::Send + 'static
        >,
        fn(blobrepo::Error) -> Error {<
          blobrepo::Error as std::convert::Into<
            Error
          >
        >::into}
      >,
      [closure@scm/mononoke/cmds/blobimport.rs:239:23: 243:14
        csid:mercurial_types::NodeHash,
        blobstore:std::sync::Arc<
          blobstore::Blobstore<
            GetBlob=std::boxed::Box<
              futures::Future<
                Error=Error, Item=std::option::Option< std::vec::Vec<u8> >
              > + std::marker::Send + 'static
            >, ValueOut=std::vec::Vec< u8 >,
            Key=std::string::String,
            Error=Error, ValueIn=bytes::Bytes,
            PutBlob=std::boxed::Box<
              futures::Future<
                Error=Error, Item=()
              > + std::marker::Send + 'static
            >
          > + std::marker::Sync + 'static
        >
      ]
    >

)


#2

Worth noting that this is the 1.20 release compiled as a dev channel, so perhaps there’s a bug that’s already been fixed since then.


#3

At a guess it is missing Error + Send + 'static


#4

My guess would be the same as @jonh’s but it’s hard to say by just eyeballing the gist. I would try to strip down copy_changeset to find the point where it doesn’t compile.


#5

If I just apply impl Future to copy_changeset, then it compiles without problem. It’s only when I convert its callers to impl Future that this error arises.

But thanks @jonh for the clue, adding where Error: Send + 'static did help, and now I’m getting a new error. This does seem to be an area where the compiler’s diagnostics could be better.


#6

Right. I meant keep the callers but strip away copy_changeset until caller compiles. Then you’d know which part of it is violating requirements.

But now that you added the constraints manually you probably get a compiler error that’s telling you the same problem.


#7

Now I’m getting

cannot move a value of type futures::Join<...>: : the size of futures::Join<...> cannot be statically determined

where the type term in Join<...> is about 4k of text (each). Still digging into it.


#8

Yikes! If you muster up the courage to gist up the pretty print of the type I’d be happy to torture myself into looking at it :slight_smile:

I will say that the original gist you showed is some of the more combinator-happy Rust code I’ve seen. That’s not to say you shouldn’t do that but the downside is obvious here: if compiler isn’t happy with something you get C++ template-esque errors.


#9

I managed to convince rustfmt to format it by using a placeholder Closure type for closures: https://gist.github.com/jsgf/e536158899f175704ffb6485e277a2fb

Yeah, this is early experimental code, and I’m not sure I’d write new code like this from scratch. But the “zero cost” promise of futures is very alluring, but you only get that by using concrete types - hence my desire to use impl Future.


#10

Where’s the exact error? Would help to see the surrounding code and also perhaps a gist with new code in that file.


#11

Ah, right. In blobimport.rs, the failure is on line 311:

stderr: error[E0161]: cannot move a value of type futures::Join<…>: the size of futures::Join<…> cannot be statically determined

I can make this go away by adding a .boxed() on line 296, but I don’t understand why - I can’t see why the size can’t be statically determined, and I don’t understand why boxing it would help. (I checked that the types are Sized, so I’m not sure what else is required to statically determine size.)


#12

Hmm, I can’t see why boxing it would change anything either, apart from potentially bypassing some less-than-fortunate type inference that occurs with the unboxed chain. Another possibility is that the error message isn’t talking about the real problem.

I’ll see if I can stare at this a bit more but these monster types are hard to untangle.


#13

So stared a bit more but can’t say I know what the issue is. The boxed call does two things there: it erases the type of the chain and also wraps it in a box (obviously). There are a few impl trait types appearing in the chain that is boxed (in the workaround). Perhaps the compiler thinks it’s trying to move a trait rather than a concrete struct; slapping a box around it sort of sidesteps that.

I’m also guessing the move it’s complaining about is happening in the map(|_| ()) call there on line 311. To that end, I wonder if boxed'ing the Join right before that map call also works.

I’m quite interested to know the root cause of all this. A lot of people are excited about trait impl, especially to use them in cases like this (chaining futures). Figuring this out would be useful to that end. And perhaps there’s a compiler bug lurking there too.

cc @alexcrichton in case he can eagle-eye the issue :slight_smile:


#14

No, that doesn’t work either. At line 311, I tried:

    put.join(manifest).boxed().map(|_| ())   // same error
    put.boxed().join(manifest).map(|_| ())   // same error
    put.join(manifest.boxed()).map(|_| ())   // ICE!

Here’s the full internal error dump. It must have been later in the compile because it already printed all the deprecated warnings, and judging from “Unexpected tail in unsized_info_ty” it looks like it has lost track of the type’s size.


#15

Have you tried using the latest nightly compiler? If it still ICEs, then I think a github issue is warranted. Perhaps the ensuing investigation will shed light on the problem as a whole.


#16

My guess is this is just a bug in rustc either with impl Trait or with error reporting. This may also be related to recursion maybe? Looks like rustc can trivially overflow its stack and with impl Trait it may be easy-ish to develop a cycle, which adding a boxed would help b/c it’d break the recursion for size checks at least.


#17

I don’t think any of the types are flat-out recursive this way, but I was wondering if it was something hitting a depth limit and giving up because the overall type is too large.

Does this look like something that may have already been fixed in beta/nightly, or should I try to pull out a simpler repro case?


#18

Any chance you can try this on the nightly channel? If it still fails to compile and ICEs in that other case, I think a github issue is warranted.