Fix confusing error and docs wrt. virtual manifests

[RalfJung]

Fixes #3966

Contains updated version of #3967

[rust-highfive]

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

[matklad]

I would be surprised if only a single tests needs to be fixed after this change. However, if this is the case, that probably means that current function is almost not used anymore, and it might be a good idea to try to get rid of it altogether: probably all commands should work with virtual manifests just fine.

[RalfJung]

cargo test says this is the only one that needs fixing.

The code flow is a little spaghetti-esque currently, but I didn't want to change that as I have no idea how this is all architectured. Before #4335, when calling cargo build, what happens is that Packages::from_flags returns Packages::Packages(&Vec::new()), which then leads in compile_ws (at

cargo/src/cargo/ops/cargo_compile.rs

Line 234 in bf0b81a

let root_package = ws.current()?;

) to calling current(), which errored on virtual manifests. However, it seems we can also end up in that branch of there is a workspace but it is empty.
With #4335 merged, that's in fact the only way to end up there, as all will always be set (unless -p is given). I have no idea if there are other ways to have compile_ws call current, or if other places call current; grepping for such calls is doesn't really work due to the rather generic name of the function.

Looking at into_package_id_specs (which has to return an empty list to end up in this branch of compile_ws), the only way it can return empty is if ws.members() is empty or if there was no -p flag and this is not a workspace -- so, it seems actually that in compile_ws, that branch will always trigger an error. If you want I can change that, and then see if there are other users of current remaining, but notice that I have no idea what I am doing here. The question is do you trust the test suite coverage enough to let me still do such changes.^^

[RalfJung]

Turns out my analysis is incorrect. For non-virtual packages, we can still get Packages::Packages(&[]), and then it still uses that other path. current will however of course always succeed, then.

I suppose one could change from_flags to not ever return Packages::Packages(&[]), but instead fill in the current package name there or so; not sure how that would look.

[matklad]

So turns out there are a handful usages left:

Function
    current
Found usages  (8 usages found)
    cargo.ops  (8 usages found)
        cargo_compile.rs  (1 usage found)
            234 let root_package = ws.current()?;
        cargo_doc.rs  (1 usage found)
            31 let root_package = ws.current()?;
        cargo_package.rs  (3 usages found)
            30 let pkg = ws.current()?;
            201 let pkg = ws.current()?;
            279 let pkg = ws.current()?;
        cargo_pkgid.rs  (1 usage found)
            13 None => ws.current()?.package_id(),
        cargo_run.rs  (1 usage found)
            17 0 => ws.current()?,
        registry.rs  (1 usage found)
            42 let pkg = ws.current()?;

Originally, ws.current() appeared as a stop gap to make workspace implementation feasible: with .current(), it was possible to make Cargo workspace aware incrementally. So it would be great to get rid of it altogether some time in the future, but probably not right now.

It seems to me that cargo publish still may hit this function from virtual manifest, and so this error message might be wrong in that case (and presumably this is not tested :( ).

So... I think we definitely should update the documentation, however, I am not sure that updating this particular error message is correct for all cases... The simplest tactical fix would be to add a if ws.members().is_empty() check, and emit either old or new error message.

The strategic fix (which should not be part of this PR) would be to look at the usages of current and try to get rid of them: I think those insidecargo_package are unnecessary because it seems to me that we'll have a current package higher in the call stack anyway, and those in cargo_compile, cargo_run and cargo_doc should probably be folded into into_package_id_specs function.

[RalfJung]

What about this? Tests all pass, locally at least...

[matklad]

This looks great! I feel a bit uneasy about that hunk of dead code in the else branch, which seemed to do something useful, but we should have good test coverage in that area.

What happens if we remove that if specs.len() > 0? I think that maybe cargo build in an empty workspace should be a no-op, and not an error?

[RalfJung]

I think that maybe cargo build in an empty workspace should be a no-op, and not an error?

It was an error so far, just with a confusing error message. So this seemed like the most compatible choice. I will see what happens.

I feel a bit uneasy about that hunk of dead code in the else branch, which seemed to do something useful, but we should have good test coverage in that area.

Yeah, I was somewhat unsure. However, notice that cargo build --all builds already do not run all that code, so I am reasonably sure that this mostly reduces code duplication. if anything, this will uncover already existing bugs because now more things use the same code path.

[RalfJung]

What happens if we remove that if specs.len() > 0?

As expected, the only test that fails is virtual_build_no_members.

However, I don't think there is ever a situation where you'd want an entirely empty project. And this is an error now. If such a situation ever comes up, this can still be trivially changed.

[matklad]

Yeah, agree that it's a nice touch to give a specialized error message for empty workspace!

@bors r+

Thanks @RalfJung!

[bors]

📌 Commit 682af71 has been approved by matklad

[bors]

⌛ Testing commit 682af71 with merge 8e36873...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing 8e36873 to master...