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

Cargo report future-incompat #2834

Open
wants to merge 15 commits into
base: master
from

Conversation

@pnkfelix
Copy link
Member

pnkfelix commented Dec 10, 2019

rendered

Summary: Cargo should alert developers to upstream dependencies that trigger future-incompatibility warnings. Cargo should list such dependencies even when these warnings have been suppressed (e.g. via --cap-lints or #[allow(..)] attributes.)

Cargo could additionally provide feedback for tactics a maintainer of the downstream crate could use to address the problem (the details of such tactics is not specified nor mandated by this RFC).

Pre-RFC internals thread

@pnkfelix pnkfelix changed the title Cargo report future incompat Cargo report future-incompat Dec 10, 2019
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
Copy link
Contributor

ehuss left a comment

This seems to boil down to "how annoying is an un-silencable warning". This seems to be a pretty decent balance, though it is tough to predict how people will react.

We could try conservative steps to see how it goes. For example, only turn it on for 1 lint, and see how long it takes for packages on crates.io to update.

Another possibility is to have a policy of a grace period when a future-incompatible lint is first introduced, leave it as capable for 1 Rust release, and then make it un-capable. This would give 6 weeks for opportunistic updates before forcing the warning on everyone. I don't know if that is really necessary, though, or what the typical timeline will look like. I assume existing lints have already been around for a while.

text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
text/0000-cargo-report-future-incompat.md Outdated Show resolved Hide resolved
Co-Authored-By: Eric Huss <eric@huss.org>
@nrc

This comment has been minimized.

Copy link
Member

nrc commented Dec 18, 2019

IMO, for most users, most of the time this would be useless and therefore annoying - the only way to exert pressure upstream is submitting a PR and most users can't or won't do that. Where this might be useful is where the maintainer of the downstream crate also maintains the upstream crate.

The Cargo team just discussed the PR. We think this would be nice to have (especially if @pnkfelix can implement), but not with every build. I.e., we should rate limit in some way. Some suggestions from the Cargo team:

  • Do it every time as proposed.
  • Only warn when Cargo is updated (i.e., rustup update).
  • Do it every time but only from a given time (either a fixed time before the warning is intended to be made into an error or manually specified by the compiler team).
  • Ramp up warnings on a schedule towards the time when the warning becomes an error (e.g., no warning at first, warnings once per week 6 weeks before, then every time 2 weeks before).

Once we decide on the warning schedule then we can accept the RFC and begin implementation with the caveat that we would intend to iterate on the frequency of warnings in the future as they are found to not have the desired effect or be too annoying.

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Dec 18, 2019

the only way to exert pressure upstream is submitting a PR

Not to detract from the rest of your points, but why is this the only way? Issues, mentioning the author in forum posts / social media, etc. are all ways to encourage a library to update.

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Dec 19, 2019

@nrc wrote:

Ramp up warnings on a schedule towards the time when the warning becomes an error (e.g., no warning at first, warnings once per week 6 weeks before, then every time 2 weeks before).

Ooh, I like this telescoping warning frequency.

Of course, doing this would exacerbate the problem I mentioned in the RFC text when I covered the annoyance as a drawback:

  • (Perhaps we could restrict it so that it only gets re-emitted once each day or some other way to modulate the annoyance level; but of course then the diagnostic would have to state very clearly that it is doing that strange magic.)

But I really like this approach, especially the idea of tying the frequency to when the lint is actually going to become a hard error. It seems like right balance to strike.

And as @nrc says, the details of the actual frequency is a policy matter that can be resolved later, in tandem with implementation.

I will update the RFC text to incorporate annoyance modulation into the system. (I personally would like to expose knobs to let the end-user tweak this, but that is yet another detail that can be resolved later.)

@Diggsey

This comment has been minimized.

Copy link
Contributor

Diggsey commented Dec 19, 2019

It would be useful if there was a way to generate a "future-incompat" report for use in CI/integrations with GitHub etc.

Does cargo currently have any kind of telemetry?

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Dec 19, 2019

but why is this the only way?

I should have said something like "only effective way" - unless the maintainer is just unaware of the change (which seems unlikely if the crate is maintained) then I think either the maintainer will eagerly correct the issue, or won't correct the issue no matter how many issues are filed or complaints made (apologies for the end of year cynicism :-) ).

@dekellum

This comment has been minimized.

Copy link

dekellum commented Dec 20, 2019

RUSTFLAGS=-Dwarnings is a useful setting for CI, as used in several projects. Does anyone know (or should this RFC address) how this would interact? I couldn't find a mention. Right now, setting that ENV variable does not cause any (--cap-lints applicable) warnings in dependencies to be denied (or shown for that matter).

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented Dec 30, 2019

@nrc

unless the maintainer is just unaware of the change (which seems unlikely if the crate is maintained)

I maintain a bunch of crates. Most of those do not get regular active updates. I don't run the compiler on all my crates every time a new rustc is released to check for new future incompat links, and I don't want to do it. I'd claim that most of crates.io crates are maintained that way.

won't correct the issue no matter how many issues are filed or complaints made (apologies for the end of year cynicism :-) ).

Yeah some maintainers can be stubborn. Then it's especially valuable to have downstream be informed about the situation, as the upstream maintainer dangers their build. People can switch away for example, fork the crate, etc.

Ramp up warnings on a schedule towards the time when the warning becomes an error (e.g., no warning at first, warnings once per week 6 weeks before, then every time 2 weeks before).

In fact I think this would lead to more annoyance, not less. If it's always warning, you'll encounter it when you update your rustc or introduce a crate to your dependencies either through an update or an addition to a Cargo.toml. You know the warning is there and can blend it out. If it appears out of nowhere to you while you are doing something completely unrelated, you will be far more annoyed. You'll be even more annoyed if since then, you have closed the console, overflown the buffer or you gotten a report by a user but can't reproduce it because the report was done on a different day and now the user can't tell you the precise crate with the problem either.

And after you committed a fix, how are you sure it's actually a fix to your issue? Consider you don't want any dependencies that create such warnings in your tree. How do you review PRs for them?

TLDR: I don't think that volatile warnings based on time, a random variable or such would be good ideas.

I will update the RFC text to incorporate annoyance modulation into the system. (I personally would like to expose knobs to let the end-user tweak this, but that is yet another detail that can be resolved later.)

Knobs to always turn it on would be very useful. They could only live in the CI of a project to prevent introduction of problematic crates.

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Dec 30, 2019

And after you committed a fix, how are you sure it's actually a fix to your issue? Consider you don't want any dependencies that create such warnings in your tree. How do you review PRs for them?

This is a compelling argument to me that the default setting should be what I described in the RFC, while @nrc's argument still convinces me that users should be given some form of an annoyance modulation knob.

pnkfelix added 11 commits Dec 30, 2019
… mechanisms.

Just state that I do not intend for us to blindly convert every future
incompat lint to use this system.
before discussing anything with crates.io.
Also, fixed some text that clearly dates from the now-irrelevant issues
we used to have with diagnostics on path-dependencies.
So get rid of text wondering about making that a step in the implementation strategy.
@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Dec 30, 2019

RUSTFLAGS=-Dwarnings is a useful setting for CI, as used in several projects. Does anyone know (or should this RFC address) how this would interact? I couldn't find a mention. Right now, setting that ENV variable does not cause any (--cap-lints applicable) warnings in dependencies to be denied (or shown for that matter).

I would think that this is an orthogonal issue/bug from the one described here?

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Dec 30, 2019

I will update the RFC text to incorporate annoyance modulation into the system. (I personally would like to expose knobs to let the end-user tweak this, but that is yet another detail that can be resolved later.)

Okay, commit 7bebac8 is my attempt to resolve this (hopefully last?) remaining point.

I always relish any opportunity to make absurdly subtle references to my past jobs

@dekellum

This comment has been minimized.

Copy link

dekellum commented Dec 30, 2019

RUSTFLAGS=-Dwarnings is a useful setting for CI [...]

I would think that this is an orthogonal issue/bug from the one described here?

I think it intersects with the change proposed in this RFC, in that if -Dwarnings is interpreted to mean "deny all visible warnings (including in any dependency)" then it will break people's CI builds and render the existing feature unusable for the same purpose on practical grounds.

If on the other hand the definition of -Dwarnings, in light of this RFC change, is (re-)interpreted to mean "deny all visible warnings in my current project/crate" then it could continue to work as presently used in the CI context.

Alternatively, if that is considered an undesirable change, then this RFC should add some new rustc flag, like -Dmy-warnings with the last definition, preserving the use case and giving users a workaround/upgrade path.

What you propose here seems otherwise reasonable to this user, thanks!

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Dec 31, 2019

I think it intersects with the change proposed in this RFC, in that if -Dwarnings is interpreted to mean "deny all visible warnings (including in any dependency)" then it will break people's CI builds and render the existing feature unusable for the same purpose on practical grounds.

Oh, okay, I think I see the issue now.

At least, it depends on whether one interprets the feedback from rustc (or the final report) as a warning that will be promoted to an error by RUSTFLAGS=-Dwarnings. I could understand a user seeing -Dwarnings and thinking that final report would be promoted to an error by that flag.

However, the final report, if emitted at all, will be emitted by cargo. I don't think that cargo itself changes its own internal behavior in response to RUSTFLAGS.

And the feedback delivered by rustc itself is not a warning diagnostic. I want to stress this sub-bullet from the reference-level explanation:

    • In the future-incompatibility checking mode of invocation, emission of the diagnostics themselves may still be silenced as specified by --cap-lints=allow or #[allow(..)] attributes.

Note that under this RFC, rustc itself will continue to make the same choices about what diagnostics to render (or not render). And I would think that it would likewise make the same decisions about when (and when not) to promote a warning to an error.

Anyway: Did I understand the scenario correctly?

Its possible that I missed some detail; but if this is something that there is a case of ambiguity in how this construct should interact with -Dwarnings, my intention is that this would also mean that the effect of RUSTFLAGS=-Dwarnings would be unchanged here: if it doesn't cause an error in a dependency today, then it should not start causing an error in an dependency in the future under the changes of this RFC.

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Dec 31, 2019

(I'm tagging this with A-stability, since turning lints to hard errors puts pressure on our stability promises, but I have been vacillating on this point, because I tend to agree that the lints here are categorizing bugs, not features, and thus it may not actually fall under A-stability...)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 6, 2020

Sorry I meant to comment on this before break but am only just now getting around to it!

I personally think this'll be a great feature to have, and clearly a lot of thought has gone into the UX here which, in my mind, is the most important thing to consider here. Today Cargo is on one end of the spectrum of "you never receive warnings for upstream crates at all ever" where the other end of the spectrum is "you get all the warnings for all crates". This RFC is striking a middle ground for some warnings which I think is a great balance to get to.

I also think that the compiler team has a track record of being adequately conservative in these warnings (if not overly conservative!) and so given that I would personally be ok with the UX being tweaked a bit. I would be fine simplifying it to remove the configuration proposed here and simply warn on every single build. Having a concise warning at the end of the build matches Cargo's currently nightly behavior of emit-warnings-on-every-build. The main thing to consider with this is the annoyance factor, but I think this'll be ok because:

  • The warning is small and concise, doesn't take up screens of information.
  • The warning is highly likely to be actionable. This is a direct consequence of the compiler team's conservativity in emitting these warnings. This would need to be codified though. The warnings emitted need to be actionable, so a warning in theory shouldn't become future incompatible until most upstream crates have a published fixed version so users simply need to do "cargo update"

The latter one is crucial in my opinion, warnings need to be immediately actionable to be useful to show up all the time. Compiler warnings, for example, are trivially actionable by changing the code you're working on, but warnings about dependencies can be tricky. I think a "social contract" is all we need here though where we all coordinate on what sorts of warnings get promoted to future incompatibility things. For example I don't think nightly-only lints should be future incompatibility lints, rather only warnings which have ridden all the way to stable can be promoted to future incompatibility warnings.


The final comment I'd have is that I think we'll want to avoid something like cargo describe-future-incompatibilities and instead of something like cargo build --describe-future-incompatibilities (perhaps not quite so long). Using a flag instead of a subcommand would help scenarios like cargo bench or cargo test or anything else requiring specific build flags to trigger (features, etc).

Cargo would always slurp up all warnings for all crates, and they'd all be cached on the filesystem so when running cargo build with the "say everything" flag even if you've already built it'll still replay the messages.

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Jan 9, 2020

The warnings emitted need to be actionable, so a warning in theory shouldn't become future incompatible until most upstream crates have a published fixed version so users simply need to do "cargo update"

Hmm. The policy proposed above, at least taken literally, may be difficult to implement. Upstream crates won't necessarily have pressure to deploy fixes to future-incompatibility issues without getting pressure from their clients downstream, and one of the assumptions underlying this RFC is that downstream rates won't exert such pressure without first making the lint in question into a future-incompatiblity lint.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 10, 2020

Oh so I should also mention that I'm assuming a similar transitionary role that happens today, which looks something like:

  • Behavior is changed, crates says impact is huge, warning is implemented instead
  • Time passes, some authors see the warning, some crates are updated
  • Eventually another rust-lang/rust PR attempts to make the warning a hard error, crater still says there's lots of breakage
  • rust-lang/rust authors proactively send PRs to core broken crates. Some changes are merged
  • this is when the future incompat lint is turned on, literally everyone gets warnings
  • More crates are published and more builds are warning-free

I think it's true that taken literally it's a bit too strict to require something actionable. I feel though that when future incompat lints are turned on is late enough in the process that your action item is very likely to either be cargo update or "go ping the PR and ask for a merge"

@pnkfelix

This comment has been minimized.

Copy link
Member Author

pnkfelix commented Jan 10, 2020

Oh so I should also mention that I'm assuming a similar transitionary role that happens today, which looks something like: [... workflow omitted ...]

Okay, yes, I think I agree that it would be best if the people planning these future changes (or more broadly, the Rust community at large) do upfront work to update existing crates identified via crater or similar tools, before turning the lint into a future-incompatibility lint.

I don't think the details of that process need to be specified in this RFC though, right?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 10, 2020

I don't really mind one way or another whether it's in the RFC itself, but I think it may be worthwhile to perhaps write a bit about when we expect future incompat lints to get turned on. For example the idea that given a warning it's "at least somewhat actionable", with examples such as cargo update or "go poke an already-existing PR" or something like that may be good to have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.