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

Uplift clippy::precedence lint #117161

Closed
wants to merge 5 commits into from
Closed

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Oct 25, 2023

This PR aims at uplifting the clippy::precedence lint into rustc.

ambiguous_precedence

(warn-by-default)

The ambiguous_precedence lint checks for operations where precedence may be unclear and suggests adding parentheses.

Example

1 << 2 + 3; // equals 32, while `(1 << 2) + 3` equals 7
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1

Explanation

Unary operations take precedence on binary operations and method calls take precedence over unary precedence these precedence may be unexpected without parentheses.
Setting the precedence explicitly makes the code clearer and avoid potential bugs.


As implemented in this PR, the lint only lints on

  • a binary operation (<<, >>...) where either the left and/or right expression is a non-bitwise operation (*, +, /...)
  • or when a literal number is preceded by a - (minus operator) and followed by some method calls

Fixes #117155 (cc @RalfJung)
@rustbot labels +I-lang-nominated
r? compiler

@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 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@asquared31415
Copy link
Contributor

Should unary ! be linted on in the same way that unary - is?

fn main() {
    let x = !10_i32.abs();
    println!("{:#034b}", x);
    let x = (!10_i32).abs();
    println!("{:#034b}", x);
}
0b11111111111111111111111111110101
0b00000000000000000000000000001011

I'm not sure what most people's expectations are of !, I could see it being more clear that it's an expression as opposed to - which some people might think is part of the literal.

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 25, 2023
@traviscross
Copy link
Contributor

@Urgau:

Regarding the nomination, the question that will come up in the T-lang call is whether or not this is an appropriate lint for rustc. That is, this may be a reasonable lint in general, but for this to land in rustc, we need to explain why this one specifically makes sense to uplift. It'd be worth leaving a comment to discuss, e.g., other comparable lints we have in rustc and how this one fits in with those conceptually.

@rust-log-analyzer

This comment has been minimized.

@Urgau
Copy link
Member Author

Urgau commented Oct 26, 2023

Should unary ! be linted on in the same way that unary - is?

I'm not sure, I've implemented it locally, but almost every time it was triggered, I've had the impression that it was a false positive. So I don't think so, at least for the time being.

Regarding the nomination, the question that will come up in the T-lang call is whether or not this is an appropriate lint for rustc...

I don't have much to say, other than clippy's precedence lint would have catch the ambiguity in #117155 and that we don't currently have anything in rustc that resemble it currently, but we however have other lints about ambiguity: ambiguous_associated_items or ambiguous_glob_reexports.

@camsteffen
Copy link
Contributor

camsteffen commented Oct 26, 2023

This lint is a bit slippery to define because there is a sliding scale of how much a human might know the precedence rules. A lesser experienced programmer might prefer parenthesis to clarify precedence of || vs. && while a highly experienced programmer might prefer to never have parenthesis that are strictly not necessary.

I do think we can define it a bit more narrowly if we say that it is about "uncommon operators" such as | or <<. So I might name this lint uncommon_operator_precedence. Of course there is still some subjectivity about which operators are uncommon, but we can all probably agree that && and || are not uncommon.

@Urgau
Copy link
Member Author

Urgau commented Oct 26, 2023

To be clear this lint as implemented in this PR only lints on:

  • a binary operation (<<, >>...) where either the left and/or right expression is a non-binary non-bitwise operation (*, +, /...)
  • or when a literal number is preceded by a - (minus operator) and followed by some method calls

@camsteffen Aside from the lint name, this lint only lints on ambiguous case, I don't think &&, || or any other operator is ambiguous is his precedence. Linting on any other case is outside the scope of this PR.

@RalfJung
Copy link
Member

a binary operation (<<, >>...) where either the left and/or right expression is a non-binary operation (*, +, /...)

+ is a binary operator. "binary operator" means "operator with two arguments".

I had to read your statement 3 times until I realized you meant bitwise operator (as opposed to arithmetic operator).

@RalfJung
Copy link
Member

RalfJung commented Oct 27, 2023

So, for nomination...

My main case in point is that I wrote this code and then assumed that ln_gamma is just horribly imprecise because it gave results that are far away from its spec. The key expression is -0.5f32.gamma(). It never occurred to me that this might be parsed as -(0.5f32.gamma()) -- even though in the file I was doing this in there are plenty of (-1.0f32).method(), indicating that I ran into this in the past and then forgot again.

This is, of course, only anecdotal evidence. But it shows that even someone who is experienced in Rust, PL theory, and mathematics, can still be led completely astray by the parser behavior. There also is a clear way to disambiguate, so IMO this is a good case for a warn-by-default lint.

@RalfJung
Copy link
Member

This lint is a bit slippery to define because there is a sliding scale of how much a human might know the precedence rules.

Agreed. That's why it is a lint, not a hard error of any sort. We'll have to make judgment calls on which cases are sufficiently surprising that we want to lint about them by default. To me, -5.method() definitely passes that bar. a && b || c is near the edge -- I would generally reject such code during review and demand clarifying parentheses. a + b*c is fine since "multiplication binds stronger than addition" is extremely widely accepted.

@camsteffen
Copy link
Contributor

-5.method()

Hmm I take back my idea about "uncommon operators".

Maybe I'm being too pedantic but I don't like the name ambiguous_precedence because it never is actually ambiguous, only mistakenly understood by human readers... maybe unclear_precedence.

@RalfJung
Copy link
Member

Note that lint names should be in plural form, e.g. "unclear_precedences".

@traviscross
Copy link
Contributor

traviscross commented Oct 28, 2023

Maybe I'm being too pedantic but I don't like the name ambiguous_precedence because it never is actually ambiguous, only mistakenly understood by human readers... maybe unclear_precedence.

If the focus is on the human, perhaps potentially_suprising_precedences.

@blyxyas
Copy link
Member

blyxyas commented Oct 28, 2023

If Rust is a language meant to be written and reviewed by humans, I think the focus would always be on the human 😅.

imo potentially_surprising_precedences is too long, and the word "ambiguous" means that something has two meanings, which does not ever happen in the language as the syntax is deterministc. So, there's a +1 from me to unclear_precedences

@traviscross
Copy link
Contributor

The word unclear has the same problem as ambiguous. The precedence is clear. There's nothing unclear about it. It's just surprising to some humans. If length is a concern (and we otherwise wanted this lint), we could say just surprising_precedences. Or facetiously, we could say, huh_precedences or wow_precedences. Those would be shorter.

@RalfJung
Copy link
Member

"clear"/"unambiguous" is also often used to mean "obvious to the reader, does not require further context". By that standard, the precedence is not clear: I can't just show the code to someone and they will know what it means; they have to first learn the rules. After all we also talk about APIs being unclear when they use poorly chosen names or things like that, even though in a technical sense everything is well-defined and hence "clear".

@bors
Copy link
Contributor

bors commented Nov 16, 2023

☔ The latest upstream changes (presumably #117979) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 21, 2023

☔ The latest upstream changes (presumably #118134) made this pull request unmergeable. Please resolve the merge conflicts.

@scottmcm
Copy link
Member

scottmcm commented Nov 22, 2023

(Just speaking for me; hasn't come up in lang triage yet.)

Hmm, I feel like the two parts of this lint should be split since even though they're both about precedence, the mistakes that happen from them feel very different to me.


I'd actually like to go further for the first part, and make -1.foo() a deny-by-default lint, since it fits my heuristic of "well yes it's defined what it does but we know from experience that that exact pattern specifically is so commonly a mistake that it's worth being extra aggressive about" (like we do for 0_u8..300 or bindings in match arms that look like variants).

Basically, I think it's weird that -2 is a single literal in a macro matcher, but it's an operator and a separate literal in a method call here.

So I'd be a fan of a proposal like "we deny-by-default lint anywhere you have something that looks like a negative literal but isn't acting like a negative literal": -1.abs() being the canonical example of that, but I'd approve the general concept of lints for that without extra oversight wherever it fits that category. (Like if we end up with postfix match, I'd want a lint on -1.match { … } as well, even though it's not a call.)


For the second category, I'm really torn. I've worked places that required parens for (x*x) + (y*y), which I hated, but if I was dictator of the universe would require far more parens that the vast majority of languages do -- even for (A ∧ B) ∨ C. I've heard both "of course << is like multiplication" and "of course you shouldn't be mixing bitwise with non-bitwise", so I just don't know how to pick a principled middle-ground for a rustc lint.

rustc will already suppress the unnecessary_parens lint for technically-unnecessary parens when mixing operators, so people can over-paren if they want, and I wonder if that might be enough.

Perhaps it's fine to leave "here's the technically-unnecessary ones you should write anyway" to things like rustfmt and clippy and rust-analyzer, rather than a rustc lint.

(I could probably be convinced in different directions here, though.)

@Noratrieb Noratrieb added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2023
@scottmcm
Copy link
Member

As another demonstration of why I think the negative-literal case is so important to lint about, notice how using literal vs tt makes a huge difference here, in a way that I'd say isn't intuitive:

#![allow(warnings)]
macro_rules! foo {
    ( $a:literal $($t:tt)* ) => { ($a $($t)+) };
}

fn main() {
    dbg!(foo!( -1.0_f32.cos() ));
    dbg!(    ( -1.0_f32.cos() ));
}

https://rust.godbolt.org/z/MxM95zPr5

[/app/example.rs:7:5] foo!(-1.0_f32.cos()) = 0.5403023
[/app/example.rs:8:5] (-1.0_f32.cos()) = -0.5403023

@Urgau
Copy link
Member Author

Urgau commented Jul 10, 2024

The negative literal case is being FCP-ed in #121364 (comment).

The "binary operation" case is much less useful.
Therefore I'm closing this PR.

@rustbot label -I-lang-nominated

@Urgau Urgau closed this Jul 10, 2024
@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 10, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 25, 2024
…-errors

Implement lint against ambiguous negative literals

This PR implements a lint against ambiguous negative literals with a literal and method calls right after it.

## `ambiguous_negative_literals`

(deny-by-default)

The `ambiguous_negative_literals` lint checks for cases that are confusing between a negative literal and a negation that's not part of the literal.

### Example

```rust,compile_fail
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### Explanation

Method calls take precedence over unary precedence. Setting the precedence explicitly makes the code clearer and avoid potential bugs.

<details>
<summary>Old proposed lint</summary>

## `ambiguous_unary_precedence`

(deny-by-default)

The `ambiguous_unary_precedence` lint checks for use the negative unary operator with a literal and method calls.

### Example

```rust
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### Explanation

Unary operations take precedence on binary operations and method calls take precedence over unary precedence. Setting the precedence explicitly makes the code clearer and avoid potential bugs.

</details>

-----

Note: This is a strip down version of rust-lang#117161, without the binary op precedence.

Fixes rust-lang#117155
`@rustbot` labels +I-lang-nominated
cc `@scottmcm`
r? compiler
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2024
Rollup merge of rust-lang#121364 - Urgau:unary_precedence, r=compiler-errors

Implement lint against ambiguous negative literals

This PR implements a lint against ambiguous negative literals with a literal and method calls right after it.

## `ambiguous_negative_literals`

(deny-by-default)

The `ambiguous_negative_literals` lint checks for cases that are confusing between a negative literal and a negation that's not part of the literal.

### Example

```rust,compile_fail
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### Explanation

Method calls take precedence over unary precedence. Setting the precedence explicitly makes the code clearer and avoid potential bugs.

<details>
<summary>Old proposed lint</summary>

## `ambiguous_unary_precedence`

(deny-by-default)

The `ambiguous_unary_precedence` lint checks for use the negative unary operator with a literal and method calls.

### Example

```rust
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### Explanation

Unary operations take precedence on binary operations and method calls take precedence over unary precedence. Setting the precedence explicitly makes the code clearer and avoid potential bugs.

</details>

-----

Note: This is a strip down version of rust-lang#117161, without the binary op precedence.

Fixes rust-lang#117155
`@rustbot` labels +I-lang-nominated
cc `@scottmcm`
r? compiler
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Aug 8, 2024
Implement lint against ambiguous negative literals

This PR implements a lint against ambiguous negative literals with a literal and method calls right after it.

## `ambiguous_negative_literals`

(deny-by-default)

The `ambiguous_negative_literals` lint checks for cases that are confusing between a negative literal and a negation that's not part of the literal.

### Example

```rust,compile_fail
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### Explanation

Method calls take precedence over unary precedence. Setting the precedence explicitly makes the code clearer and avoid potential bugs.

<details>
<summary>Old proposed lint</summary>

## `ambiguous_unary_precedence`

(deny-by-default)

The `ambiguous_unary_precedence` lint checks for use the negative unary operator with a literal and method calls.

### Example

```rust
-1i32.abs(); // equals -1, while `(-1i32).abs()` equals 1
```

### Explanation

Unary operations take precedence on binary operations and method calls take precedence over unary precedence. Setting the precedence explicitly makes the code clearer and avoid potential bugs.

</details>

-----

Note: This is a strip down version of rust-lang/rust#117161, without the binary op precedence.

Fixes rust-lang/rust#117155
`@rustbot` labels +I-lang-nominated
cc `@scottmcm`
r? compiler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"-NUM.method()" is parsed in surprising ways