Implement future incompatibility report support

[Aaron1011]

cc rust-lang/rust#71249

This implements the Cargo side of 'Cargo report future-incompat'

Based on feedback from alexcrichton and est31, I'm implemented this a
flag --future-compat-report on cargo check/build/rustc, rather than
a separate cargo describe-future-incompatibilities command. This
allows us to avoid writing additional information to disk (beyond the
pre-existing recording of rustc command outputs).

This PR contains:

• Gating of all functionality behind -Z report-future-incompat.
  Without this flag, all user output is unchanged.
• Passing -Z emit-future-incompat-report to rustc when
  -Z report-future-incompat is enabled
• Parsing the rustc JSON future incompat report, and displaying it
  it a user-readable format.
• Emitting a warning at the end of a build if any crates had
  future-incompat reports
• A --future-incompat-report flag, which shows the full report for
  each affected crate.
• Tests for all of the above.

At the moment, we can use the array_into_iter to write a test.
However, we might eventually get to a point where rustc is not currently
emitting future-incompat reports for any lints. What would we want the
cargo tests to do in this situation?

This functionality didn't require any significant internal changes to
Cargo, with one exception: we now process captured command output for
all units, not just ones where we want to display warnings. This may
result in a slightly longer time to run cargo build/check/rustc from
a full cache. since we do slightly more work for each upstream
dependency. Doing this seems unavoidable with the current architecture,
since we need to process captured command outputs to detect
any future-incompat-report messages that were emitted.

[rust-highfive]

r? @Eh2406

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

[Aaron1011]

cc @ehuss @pnkfelix

[Aaron1011]

This can be tested on https://github.com/Aaron1011/arrayinto. The current output:

Building:

   Compiling arrayinto v0.1.0 (/home/aaron/repos/arrayinto)
    Finished dev [unoptimized + debuginfo] target(s) in 0.24s
warning: the following crates contain code that will be rejected by a future version of Rust: arrayinto
note: to see what the problems were, run `cargo describe-future-incompatibilities`

Report:

The `arrayinto` crate currently triggers the following future incompatibility lints:
> warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added.
>  --> src/main.rs:4:12
>   |
> 4 |     ["hi"].into_iter();
>   |            ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter`
>   |
> note: the lint level is defined here
>  --> src/main.rs:1:10
>   |
> 1 | #![allow(future_incompatible)]
>   |          ^^^^^^^^^^^^^^^^^^^
>   = note: `#[allow(array_into_iter)]` implied by `#[allow(future_incompatible)]`
>   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>   = note: for more information, see issue #66145 <https://github.com/rust-lang/rust/issues/66145>
> 
> warning: this method call currently resolves to `<&[T; N] as IntoIterator>::into_iter` (due to autoref coercions), but that might change in the future when `IntoIterator` impls for arrays are added.
>  --> src/main.rs:5:9
>   |
> 5 |     [1].into_iter();
>   |         ^^^^^^^^^ help: use `.iter()` instead of `.into_iter()` to avoid ambiguity: `iter`
>   |
>   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
>   = note: for more information, see issue #66145 <https://github.com/rust-lang/rust/issues/66145>

[Aaron1011]

This check doesn't get taken into account by Cargo's caching - adding/removing this flag from cargo build will not cause anything to rebuilt. I'm unsure how big of an issue this is, since all of this will go away once things get stabilized.

[ehuss]

Thanks!

I wouldn't worry about the caching of the -Z flag. I'm not expecting this to stay unstable for very long, but will give us a chance to test drive it.

As for errors, the answer is "it depends". Most of Cargo uses string-based errors with anyhow, and most errors are propagated with ?. We also try to be careful to add context to every message (particularly for file I/O which often results in confusing errors without context). There is a chain_err method to help with that (which should eventually be replaced with anyhow's with_context which is essentially the same).

I would recommend moving the code in the command module to the ops module. Generally the code in command is just arg parsing, and then the actual implementation is in an ops submodule. The ops side can return a CargoResult which is a little easier to work with than CliResult.

Since this is modifying the JobQueue, that area of Cargo is much more sensitive to managing errors since it must wait for all threads to finish even if there is an error. So, as you may have noticed there is some special handling of errors there.

It looks like you already found drop_println!, which deals with I/O errors.

As for tests, in the short term I think it would be fine just to pick a warning that is unlikely to change soon. An alternative would be to create a fake rustc that emits the appropriate JSON (the tests do that in several places like this). But that wouldn't be testing that it actually integrates with the real rustc properly. Another alternative would be to embed a fake lint in rustc directly, but I'm not sure if that's necessary.

As for handling canceled builds, it looks like you have support for that, which looks good to me. Is there a specific sequence of interactions that you are concerned about?

Let me know if you have any specific questions or particular areas you'd like me to look at. There's also the new Contributor Guide which briefly provides some overviews of things like error handling. If there's something missing there, it may be useful to help highlight what needs expanding.

[Aaron1011]

As for handling canceled builds, it looks like you have support for that, which looks good to me. Is there a specific sequence of interactions that you are concerned about?

If a user performed the following actions:

• Run a build to completion, which produces a future-incompat-report
• Change a bunch of things
• Start a new build, but immediately kill it before we opened and truncated the previous report
• Ran cargo describe-future-incompatibilities

Then 'stale' warnings would be shown. I'm not sure that this really matters - I would expect people who really care about these warnings to have the commands run on CI, where they would always run to completion.

[ehuss]

Yea, I wouldn't worry to much about that specific sequence. We can always gauge later if people run into confusing situations or stale entries or whatever. It should be relatively easy to tweak and change this command.

When reading the file, it might be good to be somewhat tolerant of errors (print anything if possible, and then display an error if it has problems deserializing it). That might help in case the file gets partially written or corrupted or was generated with a different version of Cargo that changes the format, but even that I wouldn't worry too much about.

It's tough to predict every confusing circumstance, but here's one possibility:

1. cargo test: shows a warning for a dev-dependency
2. User's editor runs cargo check in the background which will delete the file (and not generate a warning since it is not checking dev-dependencies).
3. User runs cargo describe-future-incompatibilities and it prints nothing.

I'm not sure how likely that is. I'm not too worried about it, but those are the kind of weird things that might confuse people.

[Aaron1011]

User's editor runs cargo check in the background which will delete the file (and not generate a warning since it is not checking dev-dependencies).

Oh, that's quite subtle. I think it would be a good idea to add a header (or footer) to the displayed message to tell the user which cargo command generated the report.

[est31]

Nice work! I have only one minor criticism about the implementation, which is that it only prints the offending crate's name and never prints the version. The RFC specifies that cargo shall print the version if there are multiple packages with that name in the cargo build graph. I'd personally extend it from the build graph to the entire DAG encoded in Cargo.lock, basically using the same logic that adds version numbers to crates in Cargo.lock formats 2 onwards.

The reasoning why I asked for this to be added to the RFC was to make sure that users know which version is the offending one, so that they can base further actions on that.

From the maintainer POV of crates that yield such warnings, it might even be very useful to have the version number printed all the time, because the warning might not happen at the latest version of the crate but an older one. If the report includes the older unmaintained version number, the maintainer can immediately close the issue report and tell people to update (if that's their choice, maybe they consider the older version branch still maintained, then they want to publish an upgrade). If there is no version number in the report, the maintainer has to ask first which version of the crate creates the warning, or investigate themselves whether the latest version of their crate creates that warning, or whether it's an older version. It's more helpful to be told by the users though, because that's the version the users care about. Maybe it's 3 semver breaks ago, one never knows (and it doesn't help that the download graph of crates.io only lists a limited number of versions).

[alexcrichton]

One of the reasons Cargo doesn't have the issues of reading files from old builds is that it's not something that Cargo does today, basically for all the reasons mentioned here because it's quite hard. A case like @ehuss mentions above seems pretty worrisome to me, and could perhaps be solved by adding a CLI flag to cargo build (and other subcommands) rather than adding a new subcommand?

[Aaron1011]

@pnkfelix What are your thoughts on @alexcrichton 's proposal? Would this require amending the RFC?

[est31]

Actually that's a good point @alexcrichton . If the cargo describe-future-incompatibilities command would instead be a flag of cargo build/cargo check etc. it would be possible to reuse the cached json files that contain the output of rustc, thus removing the need for the extra level of caching that this PR adds. A bad alternative would be to make cargo describe-future-incompatibilities call cargo build etc. internally, but one doesn't know which precise command the user has put in previously, so one has to make a guess. The guess would be close but not precise though. Often it can lead to rebuilding of the entire crate graph only to give a result that maybe enables different cargo features etc. So a flag on cargo build/cargo check etc might be the way to go.

[Aaron1011]

@alexcrichton @est31: I've re-implemented this as a --future-incompat-report flag on cargo check/build/rustc, as you suggested. In additional to eliminating the race condition issues, this also made the implementation significantly less complicated.

[Aaron1011]

@ehuss: This is now ready for review.

While I believe this still follows the spirit of RFC 2834, the describe-future-incompatibilities command does not exist at all. Do we need to amend the RFC with this change - if so, what is the process for doing so?

[ehuss]

Oh, sorry, I just saw you sent an update here. I apologize, I did not post a comment sooner before you made some changes. We discussed this a little in our team meeting. There were two things that we discussed:

The concern of the temporal nature of the cache: If the cache is deleted and recreated each time a command is run, there is some concern that it will be confusing if it fluctuates (based on what is built), and the time lag between seeing the notification, and the user running the command to display the report (during which other commands may have been run). I personally don't feel strongly about it, but would it be possible to change the on-disk representation to accommodate that concern? Perhaps it could retain the last N runs (maybe as separate files, or all in one file), and the command would by default display the last report, but have an option to display a specific (older) report? This is instead of using a flag as suggested above. Would you be willing to maybe go in that direction?

The other thing we discussed was perhaps changing the top-level command name. I feel a little uncomfortable adding a lot of highly specific top-level cargo commands, since it can make it a little harder to document and for users to navigate. I was wondering what you think about changing it so that there is a cargo report command that takes a sub-command argument, like cargo report future-incompatibilities. This would allow us to add additional report-like features, like "show build timings", or "explain why the last build wasn't cached", and they can be grouped within a single command.

I don't think in general that design tweaks need to go through any sort of process to amend the RFC. As long as the spirit of the original RFC is retained, I think it is up to each team to decide on the final implementation.

[Aaron1011]

Perhaps it could retain the last N runs (maybe as separate files, or all in one file), and the command would by default display the last report, but have an option to display a specific (older) report? This is instead of using a flag as suggested above. Would you be willing to maybe go in that direction?

Personally, I think this is very unlikely to be used:

• If I want to see the report for a specific command (e.g. cargo check), I think just adding --future-incompat-report will be generally faster than looking up a specific report in a list. Adding a flag is fast and easy to type, while viewing a list requires the user to find the particular command they are looking for in a potentially long list of commands.

• I think the two best use-cases of this feature are gating PRs on CI, and 'nudging' local users to update their dependencies when working on a crate locally. I don't think retaining a list of past reports is useful in either of those scenarios, and would require dealing with the various race conditions that @alexcrichton and @est31 were worried about.

[alexcrichton]

This runs the risk of slowing down incremental compiles since this may do nontrivial work for all upstream crates (which can be a lot), can you try to do some testing locally to see what the impact is?

[Aaron1011]

I tested with actix-web (211 dependencies) - without running any kind of precise benchmarking, it looks like this may have increased the runtime of cargo build (with a full cache) by a few hundredths of a second (0.08 to 0.09 seconds before, 0.11 to 0.12 seconds after).

I think this worst case for this change would be:

1. Running cargo build -vv on a compilation graph with a significant number of normal upstream warnings.
2. Repeatedly running cargo build with the cache prepared by cargo build -vv, which cauess Cargo to process and discard a large number of cached diagnostic messages.

However, this seems like a pretty unlikely scenarion, and I haven't been able to measure any kind of significant impact locally (testing with actix-web again).

[Eh2406]

Drive by comment, what system was this testing on? Can we check on windows?

[Aaron1011]

I tested on Arch Linux

[alexcrichton]

I agree that if we stick with this scheme that we'll want to test on Windows which typically has a slower filesystem than other platforms. Additionally the case I'm worried about is not when upstream compiles have output but rather they have no output so Cargo opens and closes a whole lot of files rapidly which can increase the latency of Cargo itself.

[alexcrichton]

Does this still need to happen unconditionally?

[Aaron1011]

Yes - we still want to display a note when running cargo build without any arguments.

[alexcrichton]

This is another reason I would prefer to remove the flag on cargo build in the initial implementation. I don't believe this change is necessary if it's a separate subcommand, right? If so that would make the performance issue moot.

[alexcrichton]

Hm ok I believe I'm wrong that this would change if there were only a subcommand. We'd still need to scrape files from past cached compilations to have the same report every time.

I don't think any testing of this has happened on Windows yet though?

[ehuss]

The concern with a flag is that it may not be easy, or even possible to run cargo directly. For example, if it is wrapped in another build system (like x.py), it may not be obvious which command to run, where to run it, which arguments to pass, etc. Another concern is if someone is using an editor which uses cargo to drive messages, it also may be confusing to know how to recreate the same command on the command-line, and people sometimes have difficulty figuring out how to add additional cargo flags to the editor config.

In terms of the list, the intent is that the error message would specify exactly what you can run, maybe something like:

to see what the problems were, run `cargo report future-incompatibilities --report=7`

The user would copy and paste that, and see exactly which messages to display. If they ran the command without any options, it could just display the last report.

I don't feel strongly about the difference, but I think the spirit of the RFC was to make it easy to view the report, regardless of what kind of environment cargo may be running in.

[Aaron1011]

@ehuss: It looks like @alexcrichton and @est31 both expressed a preference for a flag over a command. Is there any more work for me to do at the moment, or should I wait for a decision to be made?

[est31]

What about printing the path containing the report? Then you can just cat it.

[bors]

☔ The latest upstream changes (presumably #9175) made this pull request unmergeable. Please resolve the merge conflicts.

[Aaron1011]

Rebased

[ehuss]

Can you also add some brief documentation to unstable.md?

[ehuss]

I wanted to write a few more concerns about the implementation. I don't think any of these need to be addressed now, but I think they need to be considered eventually.

• The way color is handled seems a little awkward. One of the use cases is that the user runs cargo in an editor, sees the message that they need to run cargo describe-future-incompatibilities, they do that, and the message isn't colored. Not a big deal, color isn't super important, but that seems like a common situation. Ideally I would think the report data would be saved to the file in a more raw form, and then just re-render it as requested.
• This doesn't really work in editors (the user will never see the warning). It's a bit of an unfortunate behavior of Cargo in general (Provide Cargo messages as JSON messages #8283), and the way most editor integration works. That seems like a concern, since in many cases that means the messages won't be seen.
• Requiring -Z future-incompat-report with cargo build -Z unstable-opts --future-incompat-report seems a bit awkward (it could be implied by the presence of --future-incompat-report). I understand why it is done that way, and hopefully it won't be too much of a problem.
• I think the command should save at least a few reports in the on-disk file, so that if there are things like background jobs (like cargo watch), it doesn't make it difficult to view a report. Per my suggestion far above, I think it would be easy to save them in an array, and just trim it to N reports. It could also use monotonic numeric IDs instead of the strange random IDs (the next ID could be computed by 1+ the max ID in the file). Then the report command could default to showing the report with the greatest ID when not given an ID.
• The message to "run cargo describe-future-incompatibilities" could also include the target directory if using a non-default one.
• The RFC discussed a mechanism to silence the warnings. I'm not sure exactly what that should look like, but un-silenceable warnings can be frustrating, especially if it is not possible to update the offending dependency (or no new version is available).

[Aaron1011]

I think the command should save at least a few reports in the on-disk file, so that if there are things like background jobs (like cargo watch), it doesn't make it difficult to view a report. Per my suggestion far above, I think it would be easy to save them in an array, and just trim it to N reports. It could also use monotonic numeric IDs instead of the strange random IDs (the next ID could be computed by 1+ the max ID in the file). Then the report command could default to showing the report with the greatest ID when not given an ID.

I'm concerned that this PR is adding an increasingly large amount of complexity to deal with a very unusual situation. See my previous comment #8825 (comment) - can you come up with a concrete sequence of commands that will actually cause a report to get overwritten with a different one (as opposed to an identical version of the existing report)?

[ehuss]

If I run cargo test in my terminal, then head over to my editor and make some changes while that works. The editor runs cargo check, which does not include dev dependencies, and overwrites the report from cargo test. I had back over to my terminal and see the warning. I run cargo describe-future-incompatibilities --id=abcd, and it returns the error: Your report may have been overwritten by a different one.. Two things:

• That's an unpleasant experience. It does print the ID for the last report, but it seems confusing to me.
• The information for cargo test may be different from cargo check due to different dependencies.

Can you help me understand what the complexity would be? I would expect it to just be a few lines of code. Something like:

let mut on_disk_report = load_reports()?;
if on_disk_report.reports.len() > MAX_REPORTS {
    on_disk_report.reports.remove(0);
}
let id = on_disk_report.reports.iter().max_by_key(|report| report.id).unwrap_or(0) + 1;
on_disk_report.reports.push(Report{id, contents});
on_disk_report.save()?;

I'm fine if you don't want to implement it here, it can be done later.

[Aaron1011]

@ehuss: I'd like to implement that later, if it's okay with you. I think it would be good to get some version of this available on nightly, so that people can start testing this out in various scenarios.

[Aaron1011]

@ehuss: I've addressed your review comments. Could you add your other implementation concerns to the rustc tracking issue?

[bors]

☔ The latest upstream changes (presumably #9022) made this pull request unmergeable. Please resolve the merge conflicts.

[ehuss]

Yep, I think addressing those later is fine. I pushed a few minor changes and updated the tracking issue.

Thanks! 🎉

@bors r+

[bors]

📌 Commit 139ed73 has been approved by ehuss

[bors]

⌛ Testing commit 139ed73 with merge 7441e8c...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 7441e8c to master...

[LukasKalbertodt]

@Aaron1011 Thanks for your work on this! This feature will be a great tool for developing the standard library.