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

Tracking issue for RFC 2438, "Deny the overflowing_literals lint for the 2018 edition" #54502

Closed
1 of 2 tasks
Centril opened this issue Sep 23, 2018 · 16 comments
Closed
1 of 2 tasks
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@Centril
Copy link
Contributor

Centril commented Sep 23, 2018

This is a tracking issue for the RFC "Deny the overflowing_literals lint for the 2018 edition" (rust-lang/rfcs#2438).

Steps:

  • Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
  • Adjust documentation (see instructions on forge)

Unresolved questions:

None

@Centril Centril added P-high High priority T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 23, 2018
@Centril
Copy link
Contributor Author

Centril commented Sep 23, 2018

Marked as P-high for T-compiler due to edition 2018.

@Centril Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Sep 23, 2018
@eddyb
Copy link
Member

eddyb commented Sep 23, 2018

@nikomatsakis Do we have a mechanism for per-edition default lint levels?

@varkor
Copy link
Member

varkor commented Sep 23, 2018

Do we have a mechanism for per-edition default lint levels?

Yes we do. We should just be able to replace:
https://github.com/rust-lang/rust/blob/f0bae412e97db7ef26e24546bce5199c6a086152/src/librustc_lint/types.rs#L38-L42
with:

declare_lint! {
    OVERFLOWING_LITERALS,
    Warn,
    "literal out of range for its type",
    Edition::Edition2018 => Deny
}

@varkor varkor added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Sep 23, 2018
@ollie27
Copy link
Member

ollie27 commented Sep 23, 2018

Is there any reason it can't be made deny by default in the 2015 edition as well?

bors added a commit that referenced this issue Sep 24, 2018
Deny the `overflowing_literals` lint for the 2018 edition

Closes #54502
r? @varkor
@Centril Centril reopened this Sep 24, 2018
@Centril
Copy link
Contributor Author

Centril commented Sep 24, 2018

Documentation is still todo

@pnkfelix pnkfelix self-assigned this Sep 27, 2018
@pnkfelix
Copy link
Member

triaged at T-compiler meeting. assigning to self to resolve doc issue(s).

@pnkfelix
Copy link
Member

discussed with @Centril last night, mainly to find out what sort of documentation they expect to see here. It sounds like they would be satisfied with some sort of note in the Guide for the 2018 Edition...

@varkor
Copy link
Member

varkor commented Sep 28, 2018

Note that, as far as I can tell, there is no such documentation for any of the other lints whose default level depends on the edition, so if overflowing_literals is mentioned in the 2018 Edition Guide, then the others ought to be too.

@pnkfelix
Copy link
Member

pnkfelix commented Sep 28, 2018

Following up on @varkor's point: While initiating an attempt to write the documentation, I tried to find any discussion of lints in "The Rust Programming Language" second edition. By this I mean "the linting system as a whole", or even "any particular lint." E.g. discussion or introduction of #[warn(bar)], #[allow(foo)], #[deny(baz)], etc

Maybe my search terms were ill-chosen, but I couldn't find any discussion of lints.

I'm not really inclined to spend time trying to document the changes to the default values (apart from e.g. the release notes) without having some documentation in a url-referable place that discusses the lint system as as whole.

@pnkfelix
Copy link
Member

pnkfelix commented Sep 28, 2018

The decision of what to do at this point is probably best laid in the hands of the docs team, not the compiler team.

So I'm unassigning myself, reassigning the issue to the docs team, and tagging with I-nominated to try to promote its discussion amongst the docs team.

  • (note: I left the T-lang tag on here, but the I-nominated tag is not targetted at T-lang...)

@pnkfelix pnkfelix added A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools I-nominated and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 28, 2018
@pnkfelix pnkfelix removed their assignment Sep 28, 2018
@steveklabnik
Copy link
Member

steveklabnik commented Sep 30, 2018 via email

@Centril Centril removed the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 4, 2018
@ollie27
Copy link
Member

ollie27 commented Nov 2, 2018

Well I couldn't find any reason for it to only be deny by default on the 2018 edition so I've submitted #55632 to make it deny by default on all editions.

@Centril Centril removed I-nominated A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools P-high High priority labels Feb 25, 2019
@Centril
Copy link
Contributor Author

Centril commented Feb 25, 2019

Only rust-lang/edition-guide#146 remains now that #55632 is merged; closing therefore.

@Centril Centril closed this as completed Feb 25, 2019
@TheDan64
Copy link
Contributor

TheDan64 commented Mar 13, 2019

This broke some of my doc test code in inkwell while updating to 2018e which looked like this:
assert_eq!(0b1110_0000i8 >> 1, 0b1111_0000i8);

It was used as an example as to how sign extended bitshifting works.

I don't really understand why this isn't valid. For example, looking at an even simpler example:
let _x = 0b1110_0000i8; also won't compile:

error: literal out of range for i8
 --> src/main.rs:2:14
  |
2 |     let _x = 0b1110_0000i8;
  |              ^^^^^^^^^^^^^ help: consider using `u8` instead: `0b1110_0000u8`
  |
  = note: #[deny(overflowing_literals)] on by default
  = note: the literal `0b1110_0000i8` (decimal `224`) does not fit into an `i8` and will become `-32i8`

However, it is my intent to specify the bit representation of an -32i8 (and it clearly has the i8 suffix), so how is this an overflow?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 13, 2019

Rust's literals are not the bit representation of the final value. If they were, you'd get different result depending on whether you are on a big endian or a little endian machine (ok not for u8 and i8, but everything else).

Instead the binary literals are the canonical binary representation of a positive integer, just like decimal literals are the canonical decimal representation of a positive integer. If you write 255_i8 that's the same thing as writing 0b1111_1111_i8, and 255 is not a valid i8.

@TheDan64
Copy link
Contributor

I guess that makes sense, thanks for explaining. But it's certainly surprising behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

8 participants