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

Warn about C-style octal literals #131309

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GrigorenkoPV
Copy link
Contributor

@GrigorenkoPV GrigorenkoPV commented Oct 5, 2024

Uplifts clippy::zero_prefixed_literal as leading_zeros_in_decimal_literals, warn-by-default.


The leading_zeros_in_decimal_literals lint detects decimal integral literals with leading zeros.

Example

fn is_executable(unix_mode: u32) -> bool {
    unix_mode & 0111 != 0

Explanation

In some languages (including the infamous C language and most of its family), a leading zero marks an octal constant. In Rust however, a 0o prefix is used instead.
Thus, a leading zero can be confusing for both the writer and a reader.

In Rust:

fn main() {
    let a = 0123;
    println!("{}", a);
}

prints 123, while in C:

#include <stdio.h>

int main() {
    int a = 0123;
    printf("%d\n", a);
}

prints 83 (as 83 == 0o123 while 123 == 0o173).


Notes

Compared to the Clippy lint the only difference in behavior is that the suggestion to remove leading zeros from unambiguously decimal literals (those with 8's or 9's in them) is marked as machine-applicable.

So, the current approach thus is:

  • If a non-zero decimal literal starts with a 0, issue a warning.
  • Suggest to remove leading zeros.
    • If the literal cannot possibly be octal, mark the suggestion as machine-applicable.
    • If it can, mark the suggestion as maybe-incorrect and also maybe-incorrectly suggest to use 0o.

Alternative approaches to consider:

  1. Only lint ambiguous cases (do not warn about literals with 8's or 9's in them at all)
  2. Only lint ambiguous cases where it makes a difference (at least 2 significant digits, none of them are 8 or 9).

Closes #33448
r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 5, 2024
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 6, 2024
@bors

This comment was marked as resolved.

@GrigorenkoPV GrigorenkoPV changed the title suspicious_leading_zero lint for detecting C-style octals Warn on C-style octal literals Nov 23, 2024
@GrigorenkoPV GrigorenkoPV changed the title Warn on C-style octal literals Warn about C-style octal literals Nov 23, 2024
@GrigorenkoPV GrigorenkoPV force-pushed the suspicious-leading-zero branch from fe17bb5 to def1383 Compare November 23, 2024 22:50
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@GrigorenkoPV GrigorenkoPV force-pushed the suspicious-leading-zero branch from def1383 to f70a324 Compare December 3, 2024 23:34
@GrigorenkoPV
Copy link
Contributor Author

@rustbot ready

r? @Urgau

@GrigorenkoPV GrigorenkoPV marked this pull request as ready for review December 3, 2024 23:35
@rustbot
Copy link
Collaborator

rustbot commented Dec 3, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV GrigorenkoPV force-pushed the suspicious-leading-zero branch from f70a324 to e7881ba Compare December 4, 2024 13:19
@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV GrigorenkoPV force-pushed the suspicious-leading-zero branch from e7881ba to ea90417 Compare December 4, 2024 16:16
@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV GrigorenkoPV force-pushed the suspicious-leading-zero branch from ea90417 to 2bfd5cf Compare December 4, 2024 18:04
@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV GrigorenkoPV force-pushed the suspicious-leading-zero branch from 2bfd5cf to 81b09ce Compare December 4, 2024 20:54
@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2024

The Miri subtree was changed

cc @rust-lang/miri

@GrigorenkoPV GrigorenkoPV force-pushed the suspicious-leading-zero branch from 81b09ce to 24e10ba Compare December 5, 2024 13:08
@Urgau
Copy link
Member

Urgau commented Dec 5, 2024

This looks fine to me. Let's nominate it for T-lang fcp (which could unfortunatly take months, due to T-lang backlog).

I've taken the liberty to adjust the PR description to include the lint description as well, in order to clearly see what the lint does. Feel free to update it if you update the one in the PR.

@rustbot labels -S-waiting-on-review +S-waiting-on-team +I-lang-nominated

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 5, 2024
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2024
@Noratrieb
Copy link
Member

Adding easy decision as I expect this to be accepted without much discussion, I was honestly surprised to see that this didn't exist already in rustc.

@Noratrieb Noratrieb added the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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
None yet
Development

Successfully merging this pull request may close these issues.

Lint against leading 0 in integer literals
8 participants