[Pre RFC] Cargo: report future-incompat

The example given is not a soundness hole, is it?

You made a general point about C-future-compat issues, not this specific one. The running example of the C-future-compat lint is a case of a bug (as decided per RFC) rather than a soundness hole. If you want to discuss that bug further, please do so on the relevant tracking issues. Editions are however not for fixing mistakes in the implementation.

1 Like

I'm somewhat weary of the idea of implementing this as cargo magic, rather than enabling rustc with a feature like -S, --summarize OPT Summarize the respective lint, and having cargo enable that?

One of my biggest frustrations, is with the lack of configurability around lints #5034, regarding dependencies, and transitive dependencies... a separate issue but should cargo implement this on it's lonesome specifically for the future-incompatible lint. That would make rust capable of applying lints that it wants on transitive dependencies while ignoring in some cases the

.cargo/config rustflags = "--forbid ..."

having this machinery without the ability to configure it for the user strikes me unfortunate, and I'd much rather see better lint configurability and a summary option to rustc that people could configure, than baking these specific behaviors into cargo.

That is to say, I would vastly prefer it if for this behavior cargo limited itself to setting appropriate defaults and not hard coding specific behaviors on the future-incompatability lint which cannot be configured for other lints.

I don't understand the proposal here. Are you suggesting that this unspecified --summarize option would somehow allow the machinery to live entirely within rustc?

Part of the goal of the RFC as written is to make the feedback regarding future-incompatibilities succinct; that is the main reason why I am proposing putting machinery into Cargo: So that there can be a few lines of output (perhaps just one) at the end of the compilation of all of the crates, which spans many invocations of rustc.

That is, there are customers of Rust, such as Servo, that bristle at the idea of being spammed with more diagnostic output that they will end up just ignoring.

But for Cargo to know that it needs to emit the succinct output at the end of building the crate graph, it needs to know that future-incompatibility lints were triggered. So it needs feedback from rustc as it runs, as discussed in the RFC text.

Is the problem with the fact that future-incompatibility lints are being given special treatment, and that this functionality specified here (which spans both cargo and rustc by necessity) should be generalized so that the user could specify a broader collection of lints that they want to receive this treatment?

I'd write down more questions prompted by your post, but I suspect we would both be better served if you spelled out a bit more what --summarize even is, and what solution you envisage to this problem with the use of --summarize (perhaps combined with --forbid -- though I suspect that would be too severe an approach and thus not a solution for the problem I am describing here)

It is arguably a long-standing compiler bug.

But I certainly don't want discussion of this RFC to get bogged down in debate about whether certain lints should not be included in this treatment.

My claim is that there exist at least some future-incompatibility lints that today are not being addressed by upstream crates, and their existence (or more importantly, their usage by downstream clients) impedes progress on making those into hard errors.

Based on this dialogue, I will probably revise the RFC text to:

  1. Use a different example future-incompat lint, that is more obviously a compiler bug
  2. Add some text stating that we will make case-by-case decisions about whether to promote each existing future-incompat lint into the set that falls under this mechanism. (That is, there may be exceptions that we would prefer to e.g. use an edition break for introducing the hard error.)
1 Like

This is an interesting point.

I think the official answer today for people who want that kind of feedback is that they are supposed to use cargo --verbose or perhaps even cargo -vv. (I'm pretty sure at least one of those causes the --cap-lints to switch from allow to warn, which then causes the underlying diagnostics to be visible.)

But I will admit up front that this is not really a palatable solution, because that will produce a lot of output, including output for all of the non-future-incompat diagnostics in upstream creates. That is not a useful text to wade through.

Maybe some way to tell a cargo invocation that a specified subset of the lints are not to be capped would be a reasonable way to get the feedback you are asking for here.

Not entirely within rustc, cargo would have to sniff the option and provide some behavior, The behavior of --summarize and cargo I imagine would have to be something like like:

  • --summarize would not affect the lint level, like the other lint options, it would continue allow/warning/forbidding as specified,

  • It would run the lint collect data about whether it was hit, and it would emit this at the end of compilation one time. Omitting context, or providing context seperately in a way which is easily uniquely collapsible by external tools. I.e. you could grep '^summary:' | sort | uniq for each message exercised there would be 1 line of output.

Finally cargo performs the above unique'ing and adds crate names, alternately cargo pipes the summary information to some summarizer which makes the final summary

Yes that future-incompatibility lints are being given special treatment. Programs like cargo-geiger already build summaries for lints like unsafe_code. This would add a very similar behavior for future-incompatability to cargo and so It's at least worthwhile considering how it might fall fit together ideally.

If cargo ever accepts an implementation of something like #5034 It would be best if this summary behavior could be configured by the same mechanism that allow, deny , forbid, and all this is currently bottoming out in the compiler ensures that this mechanism needs to take this option into account.

That said it is a bit different than allow, deny, forbid in that it is more like an option than an enum. Does that clarify the matter?

I think that a single line would be quite easily missed. It should be more noticeable and yes, more annoying than that.

With all due respect to Servo, they are far from everyone.

That seems out of scope for this RFC. Any decision to turn a C-future-compatibility lint (a deviation in rustc from the spec) into an edition breaking change rather than eventually turning it into a hard error will need the full language team's approval (and speaking personally, it's unlikely that I would approve of that). The default is always to turn deviations from the spec into eventual errors.

I totally agree that we need a bit of annoyance. I just worry about the amount of it. Please also think of brash's maintainer, who has a full-time non-Rust job, 6 month old and 3 year old kids, is maintaining brash in their limited free time and is now beset by people who have been annoyed into spamming them on an issue for this. (That is, the amount of annoyance generated in the downstream will somewhat converge upstream -- and getting that balance right seems pretty tricky.)

1 Like

That seems warranted. However, I put it to you that a single de-duplicated warning from a crate is the right balance. It's not that much and @pnkfelix is suggesting something even milder than that.

One would hope that people look at the issue tracker and see that there's already an issue / PR open for the problem. Ideally, the compiler is also providing an actionable suggestion that helps in fixing the problem quickly. It's also worth noting that C-future-compatibility issues are rare. They only happen if crater finds that the number of regressions are too many and that we cannot fix the bug outright.

1 Like

I apologize for being overly negative, but from my perspective you’re basically saying: “It’s worth noting that warning users about breaking changes is rare. Most of the time we make breaking changes with no warning.”

Because of course, not all code is on crates.io.

I still think this RFC is probably a good idea, since some things really are worth breaking changes – namely, fixes for soundness holes – and it’s good to make sure users are forewarned about them. But I’d hate if the warning system helped normalize the idea of “future incompatibility” among the community any more than it already is…

2 Likes

The implication I am making here is that it is unlikely that brash's maintainer will be beset by annoyed people because:

  1. C-future-compatibility lints are rare:

    a) If we break something with no warning outright it is because the regressions were so few that we managed to opened PRs for those crates and they have been merged.

    b) Most bugs occur in niche situations, otherwise they tend to be reported quickly. NLL-migration-sized regressions are one-offs. (Also worth noting that NLL migration had unconditional warnings and thank god for that...)

  2. If the crate is important then there's already likely an issue or PR open.

These days, crater indexes a whole lot more than just crates.io. A lot of the time these are abandon-ware.

Thank you for providing further details of your proposal.

I will have to reflect on it. My gut feeling is that we should keep the amount of machinery added to rustc minimal here, simply because a lot of the functionality I would like to see is a better match for living in cargo, in my opinion.

Having said that, I can appreciate that we should keep in minds the needs of non-cargo clients using rustc, and it would be best if whatever machinery we do add to rustc can be readily adapted to those other clients as well.

I suspect that in the end, we will want a couple different solutions in tandem here. As @Centril has pointed out (or at least according to my interpretation of some of their feedback), other machinery like minimum-lint-levels are not opposition to changes like the one proposed here. We may well end up adopting some solution like that, or something more general such as what has been proposed in cargo#5034.

My text was explicitly treating Servo as a representative example of a non-trivial set of customers.

I don't know what you are trying to establish with your response to my text. It makes it sound like Servo's experience is an exceptional case (though you have chosen language that is vague enough that you could back away from such an interpretation).

I am standing by my assertion: I do not think Servo is an exceptional case here.

Spamming users with diagnostic output is not a good user experience, especially diagnostic output that they cannot resolve themselves in the short term. In my own personal experience, a slew of warnings tends to obscure the important messages.

1 Like

Servo has unusual requirements and build systems. I do indeed think Servo is not an average user but I would not suggest they are incredibly rare or some such either.

What I dispute is the assertion that a single de-duplicated message per crate constitutes spam. It would be different if you saw 15 identical warnings of that kind. That would qualify as spam.

As I've noted before, such diagnostic outputs have things you can act on:

  • File a bug report to upstream or better yet, make a PR.

  • Bump the version of your crate -- this is often enough to make the error go away.

  • Temporarily use a patched fork.

But we are not talking about "a slew of warnings". Far from it.

1 Like

I had read those points and debated about responding to them in my previous message, but opted to keep that message succinct.

So let's address them here.

In the first example of filing a bug report: I think you are conflating taking action with achieving resolution. Of course I agree that it would be good to file a bug upstream: I explicitly mentioned a way to assist with that process in the "Future Work", as you saw. But this does not resolve the diagnostic locally; at least, not until the bug is fixed, the fix is merged, and the crate updated!

Bumping the version of the crate is good, when the option is available and works. Again, the RFC text explicitly points out that we would like to aid in this process. So, no argument there: If it works, that's great.

Using a patched fork may or may not be reasonable, depending on how much effort it takes to fix the problem. (I view using a patched fork as similar to making a PR, in terms of effort expended versus long-term payoff.)

But in any case, I think you are underestimating the effort involved to reach "resolution" for the diagnostics, except for the case of version-bumping. We are talking about cases where the compiler is able to compile the code, its not rejecting it yet, and hopefully isn't generating code with semantics counter to the user expectation. So there's a trade-off the developer must evaluate, in terms of whether to do the work to silence the warning, or just ignore it.

I suspect that if the diagnostic is able to provide feedback about how soon the Rust compiler will start rejecting the code, that will help project managers decide whether to invest resources in making a PR and/or fork.

If we de-duplicate the diagnostics so that you only see one per (crate, future-incompat-lint-id), then I think agree with this.

(I still think the ideal UX would provide a report at the end that further reduces the information, so that you get at most one line per crate; or perhaps even just one line with all the crates. But reaching that goal, or progress towards it, need not stop us from doing the aforementioned de-duplication.)

I agree with your description. However, I don't think instant gratification needs to be a goal here. Waiting a bit to see a single warning go away is not that onerous.

I think we should consider how often the various scenarios happen. From my experience with crater it often happens that a crate simply needs to bump its version because upstream has already fixed it. At least that's the case with older bugs.

Seems right.

And I think by being very subtle and not-at-all annoying, we promote inaction on the users part.

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/lint/struct.FutureIncompatibleInfo.html <-- add a version field here and some tweaks and we should be set.

This was the suggestion that I was going to get around to implement sometime (but the list of things to implement seems to just grow lol...).

I like the general idea here, I think it is important. I am on the side of minimizing annoyance - I've found warnings from path deps to be pretty annoying and this sounds like it would be similar. I also think that what a downstream maintainer can do is pretty limited - assuming you've updated and filed an issue, there is now nothing you can do, but you keep seeing the warning (which might be about code you don't even use). In particular, I think one can't expect a downstream maintainer to work out if such a warning is significant or not (i.e., should they panic or ignore it) because we'd be doing this for soundness issues which are usually subtle and would need a lot of context to reason about.

compiler and language teams have been relatively conservative in changing existing future-incompatibility lints into hard errors.

I think this is a feature, not a bug.

My favoured solution would be for rustc to ignore cap-lints for future compat lints and always warn (modulo de-duping, I suppose). That seems like a minimal change and Cargo is not involved at all. Showing the warning once when the compiler or crate is updated seems like the right level of annoyance to me. I don't personally think there is a lot of benefit in adding the extra info about what to do about it - that will be obvious to most users, and for those who it isn't, they are just going to ignore it. I think any benefit is outweighed by having more lines of error message.

2 Likes

Okay I've digested pretty much all of the feedback (I think). Sorry it took me so long to circle back to this.

I incorporated the feedback into the text and posted it as an actual RFC PR, here: https://github.com/rust-lang/rfcs/pull/2834