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

Implement lint against ambiguous negative literals #121364

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Feb 20, 2024

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

-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.

Old proposed lint

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

-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.


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

Fixes #117155
@rustbot labels +I-lang-nominated
cc @scottmcm
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 Feb 20, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 20, 2024
@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as outdated.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

Let's remove the nomination here in favor of the one on #117161.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 6, 2024
@traviscross
Copy link
Contributor

@rustbot labels +I-lang-nominated

Thanks to discussion with @Urgau, I now notice that this is in fact proposing a subset of #117161 and so should be separately considered (though still probably should be discussed in the same breath). Let's renominate on that basis.

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 20, 2024
@scottmcm
Copy link
Member

I'm (unsurprisingly) a fan of having a lint like this.

For triage discussion: do we want it called unary like this, or maybe ambiguous_literal_precedence?

@apiraino apiraino added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2024
@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 10, 2024
@scottmcm
Copy link
Member

We discussed this in the lang triage meeting today.

There was some unwillingness to phrase this in terms of unary operators, in the sense that there's lots of potentially-ambiguous things -- *foo.bar for example -- that we wouldn't necessarily want to lint on, expecting instead that people know those precedences.

There was broad agreement that the confusing case of negation with literals is particularly footgunny, however.

So the proposed decision is the following:

Deny-by-default lint ambiguous_negative_literal for cases that are confusing between a negative literal and a negation that's not part of the literal.

@rfcbot fcp merge

I'm checking tyler and josh's boxes per their in-meeting request.

(I think that's already the implemented semantic, just a different name?)

@rfcbot
Copy link

rfcbot commented Jul 10, 2024

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 10, 2024
@traviscross
Copy link
Contributor

@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jul 10, 2024
@rfcbot
Copy link

rfcbot commented Jul 10, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@traviscross traviscross removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 10, 2024
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 20, 2024
@rfcbot
Copy link

rfcbot commented Jul 20, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 24, 2024
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 25, 2024

📌 Commit c5e1a12 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121364 (Implement lint against ambiguous negative literals)
 - rust-lang#127300 (Fix connect timeout for non-linux targets, read readiness of socket connection, Read readiness to detect errors. `Fixes rust-lang#127018`)
 - rust-lang#128138 (`#[naked]`: use an allowlist for allowed options on `asm!` in naked functions)
 - rust-lang#128158 (std: unsafe-wrap personality::gcc)
 - rust-lang#128171 (Make sure that args are compatible in `resolve_associated_item`)
 - rust-lang#128172 (Don't ICE if HIR and middle types disagree in borrowck error reporting)
 - rust-lang#128173 (Remove crashes for misuses of intrinsics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ae71900 into rust-lang:master Jul 25, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 25, 2024
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
@BurntSushi
Copy link
Member

This completely broke Jiff once the change landed. Because it specifically supports negating spans like -1.hour(). In the case of Jiff, there is no ambiguity because -1.hour() and (-1).hour() and -(1.hour()) are all precisely equivalent.

BurntSushi added a commit to BurntSushi/jiff that referenced this pull request Jul 27, 2024
This was bad timing. The lang team just stabilized (in nightly) a new
deny-by-default lint, named `ambiguous_negative_literals`, which
triggers an error for things like `-1.hour()`. While such things can be
confusingly ambiguous in some cases, in Jiff, `-1.hour()`, `(-1).hour()`
and `-(1.hour())` are all, very intentionally, precisely equivalent.

For now we just `allow` the lint. If the lint stays, we'll likely want
to recommend allowing it in the Jiff docs.

See: rust-lang/rust#121364
BurntSushi added a commit to BurntSushi/jiff that referenced this pull request Jul 27, 2024
This was bad timing. The lang team just stabilized (in nightly) a new
deny-by-default lint, named `ambiguous_negative_literals`, which
triggers an error for things like `-1.hour()`. While such things can be
confusingly ambiguous in some cases, in Jiff, `-1.hour()`, `(-1).hour()`
and `-(1.hour())` are all, very intentionally, precisely equivalent.

For now we just `allow` the lint. If the lint stays, we'll likely want
to recommend allowing it in the Jiff docs.

See: rust-lang/rust#121364
BurntSushi added a commit to BurntSushi/jiff that referenced this pull request Jul 27, 2024
This was bad timing. The lang team just stabilized (in nightly) a new
deny-by-default lint, named `ambiguous_negative_literals`, which
triggers an error for things like `-1.hour()`. While such things can be
confusingly ambiguous in some cases, in Jiff, `-1.hour()`, `(-1).hour()`
and `-(1.hour())` are all, very intentionally, precisely equivalent.

For now we just `allow` the lint. If the lint stays, we'll likely want
to recommend allowing it in the Jiff docs.

See: rust-lang/rust#121364
BurntSushi added a commit to BurntSushi/jiff that referenced this pull request Jul 27, 2024
This was bad timing. The lang team just stabilized (in nightly) a new
deny-by-default lint, named `ambiguous_negative_literals`, which
triggers an error for things like `-1.hour()`. While such things can be
confusingly ambiguous in some cases, in Jiff, `-1.hour()`, `(-1).hour()`
and `-(1.hour())` are all, very intentionally, precisely equivalent.

For now we just `allow` the lint. If the lint stays, we'll likely want
to recommend allowing it in the Jiff docs.

See: rust-lang/rust#121364
@Urgau
Copy link
Member Author

Urgau commented Aug 1, 2024

For info: T-lang decided to temporarily switch the lint to allow-by-default (done in #128449) until they have a bigger discussion about it.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Aug 8, 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
@Urgau Urgau mentioned this pull request Oct 2, 2024
@Urgau Urgau added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

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