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

Properly ban the negation of unsigned integers in type-checking. #38776

Merged
merged 1 commit into from
Jan 5, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jan 2, 2017

Lint-time banning of unsigned negation appears to be vestigial from a time it was feature-gated.
But now it always errors and we do have the ability to deref the checking of e.g. -0, through the trait obligation fulfillment context, which will only succeed/error when the 0 gets inferred to a specific type.

The two removed tests are the main reason for finally cleaning this up, they need changing all the time when refactoring the HIR-based rustc_const_eval and/or rustc_passes::consts, as warnings pile up.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@nagisa
Copy link
Member

nagisa commented Jan 2, 2017

How complicated is it to retain the HELP use ! message? People of C background do -1 for what really is a !0 all the time and will have a hard time figuring out the binary not by themselves 100% of the time.

//~^ ERROR E0080
//~| unary negation of unsigned integer

-S; // should not trigger the gate; issue 26840
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression test for #26840. Perhaps should be retained in some way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, what? Why is there a duplicate file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the problematic split. It has to do with how there's typeck, followed by lints and then followed by trans, and lints themselves can have these errors and optionally const eval warnings.
I think @oli-obk knows exactly why this was needed in the first place, but now there is no point.

@eddyb
Copy link
Member Author

eddyb commented Jan 2, 2017

@nagisa You'd want to do that on uN: Neg not implemented, which is something we should do in general (have a way to customize error messages for certain combination of unimplemented types).

@eddyb
Copy link
Member Author

eddyb commented Jan 2, 2017

Travis error should be fixed by rust-lang/libc#484, in theory, waiting for a merge of that.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

@eddyb wrote:

You'd want to do that on uN: Neg not implemented, which is something we should do in general (have a way to customize error messages for certain combination of unimplemented types).

Hmm, was PR #33401 expressive enough to cover this scenario?

Update: maybe not; that may have been more about dealing with mismatches on type parameters other than Self.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

Okay, I was worried at first when I saw the two tests being removed, but now I understand that there remains the variants that do not have the 0 suffix.

But ...

@eddyb: shouldn't const-eval-overflow.rs be updated with cases for all the -uN::MIN that are being removed here? i.e. undo the effect of bac3eec for this file?

Update: @eddyb pointed out that this question didn't make sense. You can't bring back runtime tests of negating a uN; the whole point is that the operation is rejected by typeck now.

bors added a commit to rust-lang/libc that referenced this pull request Jan 3, 2017
Support Neg and Not in no_core mode.

Needed by rust-lang/rust#38776 which requires the traits to be implemented even for integer types.
This is already the case with binary operator traits, which always require the trait and its impls.
@eddyb eddyb force-pushed the unsigned-means-unsigned branch from 7047a8d to fbdadcb Compare January 3, 2017 19:48
@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

r=me after travis passes.

@eddyb
Copy link
Member Author

eddyb commented Jan 3, 2017

@bors r=pnkfelix

@bors
Copy link
Contributor

bors commented Jan 3, 2017

📌 Commit fbdadcb has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Jan 5, 2017

⌛ Testing commit fbdadcb with merge 2f56207...

bors added a commit that referenced this pull request Jan 5, 2017
Properly ban the negation of unsigned integers in type-checking.

Lint-time banning of unsigned negation appears to be vestigial from a time it was feature-gated.
But now it always errors and we do have the ability to deref the checking of e.g. `-0`, through the trait obligation fulfillment context, which will only succeed/error when the `0` gets inferred to a specific type.

The two removed tests are the main reason for finally cleaning this up, they need changing all the time when refactoring the HIR-based `rustc_const_eval` and/or `rustc_passes::consts`, as warnings pile up.
@bors
Copy link
Contributor

bors commented Jan 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 2f56207 to master...

@bors bors merged commit fbdadcb into rust-lang:master Jan 5, 2017
@eddyb eddyb deleted the unsigned-means-unsigned branch January 5, 2017 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants