-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Minimum lint levels for C-future-compatibility issues #59658
Conversation
This comment has been minimized.
This comment has been minimized.
I'm not sure whether it does more good than harm, and the existing system worked pretty well so far. As an example, |
I don't particularly agree with this; we have a large number of compat lints and not much progress on them; this should make the migration more effective in crates that use other crates with problems.
The system in this PR includes a macro
Sure; we can make relaxations in specific cases. Also, bugs are bugs. It's a good way to find out about false positives tho if people report them. |
7619292
to
fdc2589
Compare
I'm not immediately certain that we should switch all of the current future-compatibility lints en masse... I admit that it sounds nice to go ahead and take care of all of them, and I also admit that it seems relatively simple to undo individual lints on a case-by-case basis, if needed. But the conservative side of me thinks it might be better to first land the support infrastructure developed here, and then slowly turn on the forced-warnings for specific future-compat lints (or sets of lints) according to the near term plans of the stakeholders for those future-compat lints. |
@pnkfelix Sure; the "meat" of this PR is the infrastructure anyways ;) However, a few future compat lints are used to test the infrastructure itself... We need to keep those or find a different method to do the testing. I think we should switch most lints over relatively quickly after this PR, as most of them should be fine, so as to not get so much process overhead. |
Here's one argument for taking the more conservative approach: if I were to approve this PR as it currently stands, converting all the future-compat lints en masse, I would essentially be rubber-stamping all of the blessed output. And I don't know how comfortable I am with that. Delaying (and thus, factoring out) the conversion of the future-compat lints will allow the relevant expert stakeholders to actually review the blessed changes, which increases the chance that a "mistake" (which I guess, in this context, is a lint that we actually do not want to classify as future-compat) will be caught before it lands in nightly. |
So this looks fine to me personally, apart from the nits I noted in my review commits on individual commits. But this does represent an important shift in the compiler's behavior, especially with respect to downstream crates. While that shift in behavior is probably a bug fix (as discussed in issue #34596), I want to lift this up to a team-level discussion. Marking a T-lang (as an initial lower-bound approximation to the set of stake-holders) and nominating for discussion at the next T-lang design meeting. |
Responses from dependency maintainers and dependency updates can take weeks and months, the warnings should be
In that specific case the false positives are known for years, but the fix is not implemented yet, the warnings should be |
The criterion for un-silenceable warnings being acceptable is probably that they shouldn't "flood" the user and prevent normal work during those days/weeks/months that the warning fix can take. |
In the case of diesel that you mentioned the fix had already been done in diesel before I noticed it in rfcbot but the dependency was outdated. I did appreciate the warning -- which wouldn't have happened if they weren't macros. If fixes in a dependency takes weeks or months one should consider moving to a fork / patched version without the problems and abandoning the dependency if it takes too much time. Imo this is a desired effect of future compatibility lints: people should stop relying on this piece of code that will stop working, especially for an unmaintained library. Instead of having sudden breakage you get advance warning at the cost of some annoyance due to warnings.
Right; so we can leave this lint as
This depends on how much work it takes to fix a warning (can it be fixed?), how many warnings there are, how deep in the graph the bad dependency is, how responsive the maintainer is, whether a point release in the dependency can be made, and so on. These all require case-by-case evaluation. A default policy of making things unsilenceable should work well in most cases; if false positives are found or something caused widespread warnings in key crates we can adjust when needed (and we can adjust several times if the key crates have migrated). |
fdc2589
to
63ec1e1
Compare
63ec1e1
to
121aa97
Compare
@pnkfelix Adjusted PR according to review. :) |
This comment has been minimized.
This comment has been minimized.
121aa97
to
42b7518
Compare
So, I feel like the bits here to ignore On the other hand, the idea of ignoring I'd like to propose treating these two things as separable issues, perhaps polling folks to confirm that the first of the two (bypassing Does that seem like a reasonable next step? |
An honest question about your comment: Do you think "ignoring" (My own personal take is that I think some users might use |
On Thu, Apr 11, 2019 at 12:55:10PM -0700, Felix S Klock II wrote:
@joshtriplett
An honest question about your comment: Do you think "ignoring" `#[allow(warnings)]` (when we encounter an instance of a future-incompatible coding pattern and want to lint on it) also yields the same sort of reaction from a developer, compared to "ignoring" `#[allow(the_actual_future_incompat_thing)]` ?
(My own personal take is that I think some users might use `#[allow(warnings)]` as a blanket way to convince the compiler to stop yelling at them about "minor" stuff, and that there is decent argument to be made that the behavior of `--cap-lints` should behave similarly to `#[allow(warnings)]`... but I'm very curious to hear the opinions of others here.)
I'm slightly bothered that it's *possible* to write a blanket
`allow(warnings)` at all, so I don't have a strong opinion on what
people who write that end up seeing. :)
|
@joshtriplett another question: If, after you did ... would that also elicit the angry reaction of "the compiler isn't listening to me!" ? In essence, I'm wondering if we can distill this down into something where the local experience is that the developer gets light prodding about issues like this, but where we avoid inundating them with diagnostics that they simply do not care about addressing in the short term. |
On Fri, Apr 12, 2019 at 04:34:44AM -0700, Felix S Klock II wrote:
@joshtriplett another question:
If, after you did `#![allow(the_actual_incompat_thing)]`, the compiler did not warn you about the instances of the future-incompatible coding pattern, but *did* warn you about your use of `#![allow(the_actual_incompat_thing)]` itself (saying that the lint in question is, after all, a future-incompatibility lint about something that may (or will, in most cases) become a hard error in the future) ...
... would that also elicit the angry reaction of "the compiler isn't listening to me!" ?
... That's an interesting meta-question. That seems entirely reasonable
to me, with the caveat that there should probably still be an
`allow(suppress_future_compatibility)` for that.
(I can actually think of a legitimate use case for that: suppose that
you have code targeting two different versions of the compiler, and in
the workaround code targeting the old version, you use code that you
know that old compiler will understand.)
|
This is definitely something that has happened and annoyed Firefox, where the lint can't be fixed on the version Firefox ships with, but local devs on nightly are getting spammed with warnings (which can't even be suppressed because the lint doesn't exist on Firefox's release rustc). And Firefox actually is built via a pretty recent version of Rust, in case of a lot of other Linux packages this can be much worse. Usually large released software follows a slower tooling cadence. And it's not just their inability to deal with such things that becomes an issue, it's the bad reputation rust gets every time the Rust toolchain upgrade ruins the day of not-necessarily-rustacean releng/build folks. We really need to tread carefully here imo. |
☔ The latest upstream changes (presumably #59949) made this pull request unmergeable. Please resolve the merge conflicts. |
@rfcbot concern product-story I appreciate the goals of this PR, but I continue to feel uncomfortable with the way it's going about things. I think the root of my discomfort is that this feels like a kind of ad-hoc change in a complex space. I feel like it would be better to try and tackle this problem more holistically. At minimum, I don't feel comfortable with this being a T-lang decision made on a PR -- it feels like a "product" decision that affects the "UX". We're basically trying to figure out what things we can do to productively nudge people into updating code, which isn't really a "language design decision" per se -- or at least only partially. I don't know what's the best team for this -- it falls a bit across teams. I think it would best be approached by a working group that draws a bit on T-cargo, T-rustc and T-community. It would probably involve talking to production users and a few other stakeholders as well (e.g., authors of popular crates, compiler coders). If this sounds like a big effort, well, it is. (I also think this ties into a somewhat bigger conversation around future compatibility -- I've noticed quite a few conflicts around this topic lately. It seems clear that Rust has grown quite a bit since the 1.0 days when we last formed our policy, and it's perhaps a good time for us to think about revisiting it. We've evolved a lot of practice and gotten a lot more experience with effects in the real world. We also have a lot of production users now and hence a lot of code that is not visible on crates.io.) (I'm adding this to the lang team list of "deeper discussion topics" for possible follow up.) |
On Thu, Apr 18, 2019 at 03:23:30PM +0000, Niko Matsakis wrote:
(I also think this ties into a somewhat bigger conversation around future compatibility -- I've noticed quite a few conflicts around this topic lately. It seems clear that Rust has grown quite a bit since the 1.0 days when we last formed our policy, and it's perhaps a good time for us to think about revisiting it. We've evolved a lot of practice and gotten a lot more experience with effects in the real world. We also have a lot of production users now and hence a lot of code that is not visible on crates.io.)
(I'm adding this to the lang team list of "deeper discussion topics" for possible follow up.)
I agree that this needs a broader discussion, rather than rehashes of
portions of it each time it comes up.
|
Overall builtin lints, diagnostics, and future compat guarantees are various parts of this story about the UX that kinda get kicked around, responsibility-wise. |
Triage: Not sure where this is at. Should this PR be closed in favor of having a broader discussion? |
The question boils down to how annoying the compiler should be before a compat lint something gets turned into a hard error. See my final suggestion below. Overview and problem descriptionFuture compat changes are being introduced in leves of increasing annoyance so that people have time to fix them without it directly breaking their build. Right now, the life of a forward compat change looks like this:
The only non-suppressable stage is stage 3, which is the direct error. Also, due to how cap-lints works, any crate that I use won't emit any notice that it uses something that will be broken in the future. So out of the blue, my build may break. This has nothing to do with the maintainer being available or not. It could be your own crate you may just haven't done any changes to it for a long time because there was no need for it. There could be a new version already out there on crates.io but you or the dependency in between hasn't updated yet. The whole point of doing future compatiblity warnings this way is that people know there is going to be breakage before it actually happens. And for me this doesn't just involve the crate maintainers because it won't just break for them, it will break for everyone. So after maintainers had been notified for long enough to fix the bug themselves and publish updates, everyone who builds the code in any fashion should be notified, too, so that they can update before there is a breakage, either their own crates or crates that they are using. @joshtriplett #59658 (comment)
From the perspective of the person who's code is causing the warning, I agree. But from the perspective of users of that person's code, they deserve to know when some crate is going to break in the future. Maybe because they want to use a different library/fork, maybe to discuss with the maintainer why the maintainer has added a E.g. the ring crate one broke because of a future compat warning, because the Suggested behaviourAs a suggestion to what should be done. If spam hadn't been an issue, I'd have suggested amending stage 2 that makes the lint error by default to also make the lint at least a warning. However, as some future compat warnings may require changes in hundreds or thousands of instances, their warnings can get really spammy. To quote @SimonSapin:
Imagine some lint goes from stage 1 to stage 2 in the new model. This may be the first time downstream maintainers are notified that there is an issue and immediately they are being spammed with warnings. Final suggestionSo I think the most optimal model would be to to add an always-on warning specifically meant for suppressed stage 2 forward compatibility lints. It would fire only when a stage 2 would fire but is suppressed due to either an allow directive or cap lints. It could idk point towards lib.rs or something. And it would be the only warning not affected by Downstream users are mainly interested in whether there is any forward compat lint at all or not. To get details, they can build the crate themselves, maybe the warning can even suggest this, looking for updates, removing the Ultimately, if wanting to turn even that warning off is still a concern, one could think about adding |
I'd like to remind everyone that these "lints" are rare and only occur due to bugs in rustc rather than some regular occurrence (e.g. the
People frequently also get angry (and file issues) when the type system or parser doesn't listen to you (and the compiler understands what the user wants (recovery) but refuses it for soundness reasons or because the type system doesn't support it). Nevertheless, the type system and parser is there and continues to refuse ill-formed programs. The main difference between the situation of C-future-compatibility warnings and existing errors is that an error in the analysis led to not emitting the error. The distinction does not seem fundamental to me.
We/compiler are still "here to help you" because when these bugs occur, they are often also easy to fix and the diagnostic explains the issue in a clear way. Moreover, there's always a link provided which takes you to a C-future-compatibility issue with a description of the problem and how to fix it.
I don't particularly agree that this use case is legitimate. For the new version of the compiler, it is most likely the case that a) you can rewrite the code in some different way which the old compiler also supports, b) the code is fundamentally broken, may be unsound and should be removed because it can lead to mis-compilation. In the case of unsound code, I don't think it does anyone a favor to use conditional compilation to avoid bumping the MSRV. Moreover, most Rust users use stable and I don't think we should facilitate some people's conservatism.
Firefox's devs have more resources and should fix the problem on nightly. I don't see why it's fair that rustc's developers should bear so much of the cost for filing all the issues, the PRs, and deal with the technical debt that a C-future-compatibility lint entails (e.g. in the case of NLL we could have moved much quicker).
The original PR has already been substantially diluted such that the policy for using
To me it is clear that it is within the language team's gift to make this decision (but we may consult with others like we do with most other decisions). In particular:
Please bear in mind that this would be a time-consuming effort to add to all our other big efforts. The problem is something I think we need to fix urgently, not in a year.
I think downstream users would be interested to see what the shape of the problem is and they do not get that problem if they don't see a warning on any of the actual instances of the problem. If the goal is to avoid spam, the better approach would be to prune more aggressively by using |
Yeah, something like this would alleviate my concerns too. Reporting a single message for the combination of |
Let me say that this is an awesome idea because it gives maintainers a small time window to fix the issues before downstream users are being notified. They'd have no chance to get ahead of the curve if
I don't think that it's neccessary, after all anyone curious can just clone the project and check it for themselves, but if it's done in a way to avoid the spam problem, it'd work for me! One shouldn't bikeshed too much on this, it can be easily changed once in tree. |
Please read what I said. The problem could not be fixed on the version of rust Firefox ships with, it relied on features that were relatively new. It's not easy for released software (remember, Linux distros are picky about this) to just decide to upgrade its rust version. Besides, the point of bringing this up was not to specifically talk about Firefox, but rather to say that this has happened in the past. We did work around it, and it was ugly and hacky. That's not the point. The point was that future compat lints put a rust codebase in a sticky situation, and this change is making them worse. Firefox may have the resources for this, but not everyone does. Rust is already fighting a reputation for being unstable. This kind of thing matters for adoption.
This is precisely what maintaining a production compiler is about. This is software people use, we have to put effort into it so that using and upgrading it is smooth. |
We've had this discussion before: The lang team never really followed the lint portion of that doc in practice, for years. But regardless of what the doc says, what we're trying to say is that we should revisit this policy and figure out a proper home for compiler UX concerns, which includes lints and diagnostics. Right now it's largely an ad-hoc thing. The compiler has a diagnostics WG which could take over, but it's starting out with a narrower focus. |
unassigning self. (Personally I now think this PR should be closed and we should work on designing a solution that handles this problem a bit more gradually, rather than potentially flooding the downstream crates with a bunch of insuppressable messages.) In any case I'm going to absent until mid September, so someone else should take the reins on this. |
Closing this for now to reduce backlog; Will rework the PR per:
and #59658 (comment). |
Introduce a notion of
min_level: Level
forLint
s. The minimum level cannot be loosened further.min_level: Warn
fornested_impl_trait
and then you#[allow(nested_impl_trait)
then a warning will still be emitted regardless of--cap-lints
.#[allow(nested_impl_trait)]
has no effect, this will triggerunused_attributes
.#[deny(warnings)]
is used, this is also taken into account. In particular, the minimum level will result in a warning being triggered which will cause#[deny(warnings)]
to produce a compilation failure. However,--cap-lints
will be honored here such that a warning is still emitted fornested_impl_trait
butdeny(warnings)
becomes$the_cap(warnings)
. We do this so that if crate A depends on crate B and B violatesnested_impl_trait
, then this does not produce a compilation failure in A.Also, we introduce a macro
declare_unsuppressable_lint!
which setsmin_level: Warn
.We then use
declare_unsuppressable_lint!
forILLEGAL_FLOATING_POINT_LITERAL_PATTERN
.fixes #34596
cc @nikomatsakis
r? @pnkfelix