Rust: Future-incompatible warnings should always print a warning, even if lints are allowed

[nikomatsakis]

Currently, if a future-incompatible lint is set to allow (either manually or via cap-lints), no warning is printed. This is bad because it means that if you author a library that is widely used, but which you are not actively using at the moment, you might not notice that it will break in the future -- moreover, your users won't either, since cargo will cap lints when it builds your library as a dependency.

So basically the behavior for future-incompatible lints would be that they can either be "warn" or "deny" but can never be _completely_ silenced. If cap-lints is set to allow, then while other lints will be set to allow, the future-incompatible lints will be set to warn (but not deny).

cc @rust-lang/compiler @brson

[nikomatsakis]

I'm marking this as mentor because I'd be happy to mentor it (though until July 15 I'm on vacation). I marked it as easy because _think_ this should be relatively straightforward, though I may be wrong about that. I'll try to look at the code in a bit and print out a summary of what would need to be done.

[hgallagher1993]

If it is fairly straight forward I'll have a go at it

[brson]

You got it @hgallagher1993!

[hgallagher1993]

Hi @nikomatsakis I was just wondering did you get a chance to look at this task just to see if it's fairly straightforward :smiley:

[pnkfelix]

@hgallagher1993 doubtful since @nikomatsakis noted that he was on vacation until July 15th.

[hgallagher1993]

@pnkfelix I read his message as he was going on vacation _on_ the 15th...woops :laughing:

[nikomatsakis]

@hgallagher1993 glad to hear you are interested! I never did take a look, but I can do so now!

[nikomatsakis]

@hgallagher1993 ok, sorry for the delay. I did a bit of digging. I think the changes you need to make are roughly this:

In src/librustc/lint.rs, there is a function called set_level -- it changes the level of reporting for a lint. Currently, it first checks something called the lint_cap, which records a maximum level (this is used by cargo so that you don't see "unused variable" warnings for your dependencies).

What we want to do I think is change that function to also check whether self.future_incompatible(lint).is_some(), in which case this is a future-incompatibility warning. In that case, after applying the lint-cap, we should set the level to max(level, warn) -- in other words, ensure the level is _at least_ warn, regardless of what the lint-cap says.

We can also add some test cases for this using the testing framework. The simplest test would be something that takes some future-incompatible lint and sets it to #[allow] -- we should still get a warning. We can also have a test that passes -A cap-lints=allow, which should yield the same result. I can help you more w/ writing said tests if you need.

(One twist: we may want to add a "dummy" future-incompatible lint just for testing purposes...)

[nikomatsakis]

That said, @wycats has been arguing to me that this would be a bad thing to do. It may be worth having a wider discussion if there are other approaches we might consider.

[nikomatsakis]

I think, if I may take a stab at summarizing @wycats's point-of-view, that his position is:

• seeing warnings in code you don't control is annoying (true)
• users are unlikely to file issues or know how to respond (seems true)
  
  
  
  • they may well respond by just downgrading
  
  
• we would be better off finding other ways to alert crates.io maintainers of upcoming problems, and finding other ways to alert end-users of the need for action
  
  
  
  • for example, we already try to alert crate owners to warnings that we uncover via crater runs, but we could try to have some kind of dashboard or other thing you can look at...or something...
  
  
  • this might include other statistics too, like docs etc
  
  
  • we could encourage people to cargo yank soon-to-be-broken versions and give a reason, for example citing the upcoming change
  
  
  • this could then warn users that they should update their dependencies to newly available version
  
  

[jacwah]

@hgallagher1993 Do you still want to fix this issue? Otherwise I'd be willing to handle it.

@nikomatsakis Considering @wycats' comments, do you think the implementation you outlined above still should be followed?

Some thoughts:

users are unlikely to file issues or know how to respond

Maybe the message could link to the crate's homepage/repository and invite the user to contact the maintainer?

seeing warnings in code you don't control is annoying

This is very true. Say there's a very stable application working well and doing it's thing. There's no reason to change it or upgrade Rust version. This is a case where silencing future incompatible warnings may be desirable.

[nikomatsakis]

@jacwah I'm honestly not sure what to do here. What we're doing now doesn't seem right, but just enabling the warnings also doesn't seem ideal. I think perhaps what is needed is better integration with cargo and maybe even crates.io -- i.e., if we (the compiler team) could leave annotations on abandoned crates, and those could be summarized to people who depend on them, that might be ideal.

cc @wycats @alexcrichton

[alexcrichton]

I agree that warnings in deps you don't control is extremely annoying (to the point of frustration) and is something that I'd like to avoid. That being said I think we've been very principled so far about how we approach this downside with --cap-lints allow and being conservative with lints as well.

I think it's still worth it to implement this issue as-is and not take "don't have deps generate warnings" be a hard constraint. Plus we can always tweak it later if it gets too onerous.

[hgallagher1993]

@jacwah Yes I actually do have time for this! I completely forgot about it and it's actually after reminding me about another task I was assigned for Servo.

[nikomatsakis]

@alexcrichton I wonder, how hard would it be to do something like this: Have cargo capture the output when compiling dependencies (Does it already? I think so?). If an error results, we would print it out as normal. But if no error results, we would scan for a future-compatibility warning -- if one is found, we would print something like:

Warning: version 0.3 of crate `foo` is relying on a bug fix and will break in future releases of `rustc`

We could then check whether newer versions of foo are available and suggest upgrading if so, or give other helpful tips.

[alexcrichton]

@nikomatsakis that sounds like a great strategy to me.

Cargo doesn't currently capture output from the compiler because if it does so then colors are lost, but we could perhaps fix this with a generic-enough json format which describes how to re-apply colors perhaps?

[nikomatsakis]

@alexcrichton

Cargo doesn't currently capture output from the compiler because if it does so then colors are lost, but we could perhaps fix this with a generic-enough json format which describes how to re-apply colors perhaps?

Bah humbug. However, this adds another use case to something that I wanted to have in the JSON. In particular, I've long advocated that the JSON should include a "fully rendered" section that specifies exactly how the compiler would display the error (in addition to having semantic fields). This thread describes it briefly, but I feel like there is a better cite. Anyway, my main motivation was to allow emacs and other "half-IDEs" to capture the output as JSON but easily reproduce the precise output of the compiler (since it is likely to improve and keep improving, and I wouldn't want to have to reproduce the logic in emacs of e.g. how to format the snippets). However, it seems like cargo might be another consumer!

[alexcrichton]

@nikomatsakis yeah that sounds totally plausible to me, and I'd be down for doing that in Cargo!

[Mark-Simulacrum]

Unmarking as E-easy and E-mentor since this doesn't have an easy path forward and isn't really ready to be mentored. We should re-add those once we come to a decision.

[SimonSapin]

I think this is the wrong decision. Doing a full Servo build now spams the console with a lot of warnings like warning[E0122]: trait bounds are not (yet) enforced in type definitions because of code in third-party library.

There is nothing we can do in the Servo repository about this warning. Showing it to Servo contributors is not useful.

[petrochenkov]

@SimonSapin
The decision hasn't happened yet, E0122 is not a lint, it's one of few hardcoded warnings not controlled by lint levels. Ideally, it should be turned into a lint or into an error, someone just needs to send a PR. Or the bounds could be actually enforced, but that's harder.
See issues https://github.com/rust-lang/rust/issues/21903 and https://github.com/rust-lang/rust/issues/40640.

[SimonSapin]

https://github.com/rust-lang/rust/issues/40640#issuecomment-287601238 claims that showing this warning despite --cap-lints=allow is on purpose.

[nikomatsakis]

@SimonSapin that is indeed a stupid warning, but it is (indeed) also orthogonal from this particular issue. I have some plans to work on fixing that warning (the right way), but no ETA yet on those changes. Probably better to discuss in one of those more specific issues.

[SimonSapin]

@nikomatsakis Do you mean fix the fact that this warning appears even with --cap-lints=allow, or entirely remove the need for that warning?

[est31]

@nikomatsakis

If an error results, we would print it out as normal. But if no error results, we would scan for a future-compatibility warning -- if one is found, we would print something like:

Warning: version 0.3 of crate `foo` is relying on a bug fix and will break in future releases of `rustc`

This seems like a path forward, no? It is not spammy if you only emit it once per crate.

I don't think we should wire this up to crates.io, as the same problem applies with path and git dependencies.

The goal of emitting forward compatibility warnings is to make people fix these issues. Otherwise stuff breaks when the compiler switches the lint to a hard error.

[pnkfelix]

I'm prioritizing this as P-high because our ability (in the future) to change these warnings into hard errors is going to critically depend on actually reporting the problems to developers

[pnkfelix]

triage: nominating for discussion at next compiler team meeting, to confirm that this does deserve a P-high label, and if so, see if we can find a volunteer.

[pnkfelix]

(removing nomination label since @Centril self-assigned. I'm just going to assert "yes, this is important" until someone convinces me otherwise.)

[pnkfelix]

triage: re-prioritizing to P-medium; this is being addressed, and while the task is high priority, it does not need to be given the same level of urgency that I would like to associate with P-high.

[Centril]

Reassigning to @pnkfelix since they are doing the work on this right now.

[pnkfelix]

The aforementioned work was invested in drafting https://github.com/rust-lang/rfcs/pull/2834