Emit a warning when manifest specifies empty dependency constraints

[felixc]

This warns when encountering dependencies of either of these forms:

  [dependencies]
  foo = {}

and

  [dependencies.foo]

(with nothing further provided).

In the future, this should likely become a hard error.

Related to #2147

[rust-highfive]

r? @huonw

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

[felixc]

Question for reviewer: Is there a convention for phrasing to indicate this really should be changed and may become a hard error in the future?

Similarly, is there anything (comment, annotation, etc) that should be left here in the code as a marker for future cleanup?

[alexcrichton]

Ah yeah this looks ok to me, we typically avoid capital letters and ending punctuation, however

[felixc]

In the related bug (#2147), @alexcrichton suggests this should perhaps become a hard error (breaking change) in the future.

1. Is there consensus on this?
2. Is there a process to track/enact that change in a release cycle or two?

[alexcrichton]

Thanks! Can you be sure to add a test for this as well?

Also, could you reword the warning to indicate that it will become a hard error in the future?

[alexcrichton]

Ah and to clarify, we don't currently have a process for ensuring this becomes a hard error, but it's fine if we just wander across this in a few months and flip the switch.

[felixc]

I've updated the wording of the warning as suggested, but am having trouble writing a sensible test. By definition the test manifest has to list an empty dependency for us to try and compile. That means I can't create some dummy files and use path to point to them; but rather than we have to fall back to checking crates.io. Trying to download and build some arbitrary crate doesn't seem like a good idea in a small test.

Is there a better way? Or at least a designated empty test crate that's controlled by the Cargo developers and used for this kind of purpose?

For reference, I've got something like this:

test!(empty_dependencies {
    let p = project("empty_deps")
    .file("Cargo.toml", r#"
        [package]
        name = "empty_deps"
        version = "0.0.0"
        authors = []

        [dependencies]
        foo = {}
    "#)
    .file("src/main.rs", "fn main() {}");

    assert_that(p.cargo_process("build"),
                execs().with_status(0).with_stderr_contains("\
warning: dependency (foo) specified without providing a local path, Git repository, or version \
to use. This will be considered an error in future versions
"));
});

At present, this fails because foo isn't a real crate on crates.io; but I'm hesitant to actually use a real one.

[alexcrichton]

Ah there are some examples throughout the test codebase for this sort of dependency. I believe you just need to do this.

[felixc]

Thanks for the pointer! Beautifully simple. Tests added and PR updated.

[alexcrichton]

@bors: r+ 1e985cc

Thanks!

cc @brson, what do you think is the best way to make sure this gets into the release notes?

[bors]

⌛ Testing commit 1e985cc with merge d9e8c60...

[bors]

☀️ Test successful - cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64