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

overflowing_literals should have an option for "ignore signed overflows" #99195

Open
clarfonthey opened this issue Jul 12, 2022 · 9 comments
Open
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

I have recently been working with code where using the two's complement form of signed literals is useful, but this doesn't mesh very well with the overflowing_literals lint.

Here's a simplified example:

let good_signed = 0b_1111_1111i8;
let bad_signed = 0b_111_111_111i8;
let good_unsigned = 0b_1111_1111u8;
let bad_unsigned = 0b_111_111_111u8;

Right now, with the overflowing_literals lint, good_signed, bad_signed, and bad_unsigned will all be marked as overflowing. To avoid good_signed being marked as invalid, I had code that I blanket marked as #[allow(overflowing_literals)], but this caused me to miss literals like bad_signed and bad_unsigned, which overflow regardless of their sign.

I'm not sure what the best strategy here is. I see two possible solutions:

  1. The overflowing_literals lint is split into a separate lint alongside overflowing_literals lint, where this new lint warns on integer literals that would not overflow if they were unsigned, but do overflow because they're signed. This can be selectively allowed in code while still being able to keep the overflowing_literals lint on.
  2. Instead of splitting the lint, no lint is thrown at all in the case where a signed integer overflows to a negative number because it's written in two's complement form. Or, alternatively, the lint is only triggered if the integer is written in base-10, but not if it's written in hexadecimal, octal, or binary.

Either way, the current configuration of the lint seems overly restrictive.

After searching, I decided to file a separate issue instead of including this analysis in #53628. If this is seen as too similar to the existing ticket, I'm fine with also closing this in favour of that one.

@clarfonthey clarfonthey added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 12, 2022
@AaronKutch
Copy link
Contributor

AaronKutch commented Feb 23, 2023

edit: see #108383 instead

@scottmcm
Copy link
Member

scottmcm commented Feb 24, 2023

The big complication to me here is that context can be really important for whether the overflow is "important".

For example, what does this do?

fn main() {
    for i in 100..200 {
        foo(i);
        dbg!(i);
    }
}

It might be nothing, depending on non-local stuff: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4f71666663951eb0bcfa3accb2593a5d.

And I'm not sure that signed-vs-unsigned is quite the right pivot here. After all, 0xFFF_i8 feels like it should undeniably be deny despite being signed, and there are lots of people who wish they could do let x: u16 = -1;.

A first stab at trying to tease out the various categories:

  1. If the literal contains a bit that doesn't affect the eventual value, like 0x7FFFFFFFF_i32 or 300_u8 or 100_000-inferred-as-i16. This feels like an obvious deny, and potentially a candidate to make a hard error over an edition boundary. (Maybe for usize it'd stay deny, though? 🤷)
  2. If an unannotated literal doesn't fit in its inferred type, but would fit in the other (signed/unsigned) version of the same width, like the 100..200 example above, or maybe could include the let x: u16 = -1; case (though I never remember the rules for whether - is part of the literal or not).
  3. If an annotated literal doesn't fit in its annotated type, but would fit in the other (signed/unsigned) version of the same width, like your 0x8080_i16 examples. Maybe there are versions of this one where it makes sense to not be deny, especially if it's using an exhaustive hex/binary format and thus is somehow "clear" what it's doing?

I'll nominate to see if lang has any suggestions on whether this should be split up at all, or if so how. (Coming up again because of #108269.)

@scottmcm scottmcm added the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Feb 24, 2023
@AaronKutch
Copy link
Contributor

AaronKutch commented Feb 24, 2023

My preference is that, in a future edition, all overflowing literals are unconditionally hard errors that can't be changed by some lint. This is how it should have always been, its just some sloppy left over from other languages. usize/isize is kind of a weird case, but I think it should error on building for certain architectures, because it indicates something in the code is going to get cut off

@workingjubilee
Copy link
Member

workingjubilee commented Feb 26, 2023

If an unannotated literal doesn't fit in its inferred type, but would fit in the other (signed/unsigned) version of the same width, like the 100..200 example above, or maybe could include the let x: u16 = -1; case (though I never remember the rules for whether - is part of the literal or not).

As @BoxyUwU noted to me, we have to combine the hyphen-minus and the digits, at least when evaluating this lint, as otherwise we can't resolve this correctly:

let x: i8 = -128;

Given first 128, and then negation, it would first overflow. To the correct value, yes! And the negation would do nothing. But it would definitely trigger our linting otherwise.

@scottmcm
Copy link
Member

We discussed this briefly in some lang meetings. The temperature of the room was positive towards potentially tweaking things, but would be interested in having a concrete proposal to review before saying more.

@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. labels Mar 14, 2023
@clarfonthey
Copy link
Contributor Author

Just double-checking on what the process is here, is there somewhere where that proposal would best go besides Zulip or here? Not sure what lang-team does nowadays for this.

@workingjubilee
Copy link
Member

workingjubilee commented May 22, 2023

@clarfonthey I would recommend posting a concrete proposal here on the issue, forking the discussion itself off into a Zulip thread, and coming back here with a summary of any revisions.

I seem to remember lang had been experimenting with the "major change proposal" format, but I'm not sure if they've continued use of it into 2023: https://github.com/rust-lang/lang-team/issues?q=is%3Aissue+label%3Amajor-change

@scottmcm
Copy link
Member

Since lints aren't part of the strict stability promise, we can definitely use low-overhead processes here. Do whatever's easiest for now, and we can figure out how to officialize it as needed.

@clarfonthey
Copy link
Contributor Author

Sounds good to me! Mostly just curious what the best way would be to ensure that folks are actually able to chime in on the design, since I know that not everyone is following this specific thread. I'll probably just post a link on Zulip if/when I end up writing a proposal, assuming someone else doesn't beat me to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Status: Idea
Development

No branches or pull requests

4 participants