Fix negate_unsigned feature gate check

[nagisa]

This commit fixes the negate_unsigned feature gate to appropriately
account for inferred variables.

This is technically a [breaking-change], but I’d consider it a bug fix.

cc @brson for your relnotes.

Fixes#24676
Fixes#26840
Fixes#25206

[rust-highfive]

r? @brson

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

[nagisa]

Note for reviewers: I put this into TypeLimits lint pass; I think negate_unsigned is strongly related to that, but if you want me to create a new pass I’ll do.

[eefriedman]

Umm, are you sure this patch does the right thing? Specifically, it looks like the lint code only triggers for integer literals.

[nagisa]

Negation of unsigned variables is another lint, if I understand what you’ve in mind.

EDIT: interestingly there’s also a lint for unsigned literal negation, but I never seen it firing for some reason.

[bluss]

Well it will be a very palpable breaking change, so just "technically a breaking change" is misleading.

[eefriedman]

Feature gates are hard errors; lints are not (at least, https://github.com/nagisa/rust/blob/overflowing-unsigned/src/librustc_lint/builtin.rs#L153 is not).

[nagisa]

Yes. I put this feature gate in a lint, because otherwise the feature gate doesn’t quite work properly. Might need a comment or a better place if feature gate being in a lint is not acceptable.

[nagisa]

cc @pnkfelix, because you implemented the original gate.

[eefriedman]

Ignoring where exactly the code is, feature gate errors must be emitted using emit_feature_err.

Alternative approach: master...eefriedman:unary-typecheck . Still WIP because I was having some trouble with the diagnostics, but it works otherwise.

[nagisa]

feature gate errors must be emitted using emit_feature_err

Isn’t that exactly what this PR does? My approach was to do it as simply as possible, though; if you find any better ways to fix the issue, great!

[eefriedman]

Ugh, I was just confusing myself; the current UNSIGNED_NEGATION lint is essentially dead.

Anyway, the problem is that the new code you inserted into the lint only runs for integer literals.

[nagisa]

Anyway, the problem is that the new code you inserted into the lint only runs for integer literals.

Indeed, I was of impression that this is exactly how it should be, but having checked it now, currently feature gate actually works for unsigned variables. Will update!

[nagisa]

Updated!

Removed the dysfunctional lint as well. Tell me if you want it back for whatever reason.

[eefriedman]

It would be nice to print a "deprecated lint" warning for existing code using #![allow(unsigned_negation)], instead of an unknown_lint warning.

[nagisa]

Implemented lint deprecation in nagisa@2250215.

[pnkfelix]

If I understand correctly, This PR goes straight from 1.0/1.1 accepting some code to now a hard error with an associated feature gate.

I think the current protocol for breaking changes is to first put in code that detects the scenario and emits a warning (potentially a hard warning, namely one that cannot be disabled), but not an error, at least not until the warning mode has made it to a release channel.

cc @rust-lang/lang

[nagisa]

If I understand correctly, This PR goes straight from 1.0/1.1 accepting some code to now a hard error with an associated feature gate.

This is indeed the case. Due to very unfortunate implementation bugs, nor lint (added a while ago) nor the feature gate (added during 1.0 beta) fired for some cases of unsigned integer negation.

On the other hand, this PR also makes some valid code that was previously feature-gated (#26840) actually valid.

I think the current protocol for breaking changes is to first put in code that detects the scenario and emits a warning

This is easy to change (simple change from emit_feature_err to span_warn), but in that case I’m not overly enthusiastic on making sure feature gate still gates cases that were previously gated (that is, I’d propose to remove the gate altogether and emit a warning instead if we go this way).

[pnkfelix]

@nagisa to be clear: my only concern is that code that used to compile continues to compile for a cycle (potentially with a warning). I do not think we care so much about ensuring that the feature gate continues to gate the same cases -- that is, it is okay for the compiler to become more liberal in what it accepts, as long as we are sure its headed in the right direction. :)

[pnkfelix]

Actually on further reflection, I might be simply wrong to apply the aforementioned migration policy in this case. In particular, this is not exactly analogous to the object lifetimes-defaults change, where we were concerned about the silent injection of new detaults causing hard-to-diagnose errors in downstream code -- in this case, it sounds like all of the cases where we inject an error should be pretty narrowly focused and thus easy for a user to understand the problem (and put in an appropriate fix)

[pnkfelix]

(The lang team discussed this last week but I forgot at that time to write a note)

The decision regarding policy was this: in general, assume that PR's with known breaking changes of the form "X becomes a compile-time error" simply have to go through a warning cycle before the hard error is turned on.

We did not discuss the detail about ensuring that the feature gate continues to feature gate the same things. But my personal suspicion is that it is okay for the behavior there to change -- my main concern is about how we treat supposedly stable code.

For stable code, if we are correcting a previous oversight, then we are supposed to go through a warning cycle first.

So, in short, I think your "simple fix" from your previous comment should be fine.

[pnkfelix]

@bors r+ 0ca8e49

[bors]

⌛ Testing commit 0ca8e49 with merge 1855750...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt