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

Adds a new lint that checks if there is a semicolon on the last block statement if it returns nothing #6681

Merged
merged 8 commits into from Feb 7, 2021

Conversation

1c3t3a
Copy link
Contributor

@1c3t3a 1c3t3a commented Feb 5, 2021

changelog: Added a new lint: SEMICOLON_IF_NOTHING_RETURNED
fixes #6467
Adds the SEMICOLON_IF_NOTHING_RETURNED lint and therefore closes #6467.

@rust-highfive
Copy link

r? @phansch

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 5, 2021
@1c3t3a
Copy link
Contributor Author

1c3t3a commented Feb 5, 2021

The dogfood tests are currently failing, because the lint fires in several occasions in the clippy codebase. Should we maybe change the lines in the codebase? Otherwise pedantic is maybe the wrong lint level. The lint also fires on for loops as I saw, which is not desired behavior.

@1c3t3a 1c3t3a marked this pull request as draft February 5, 2021 20:19
@xFrednet
Copy link
Member

xFrednet commented Feb 6, 2021

pedantic should be the correct lint group. The lint is triggered on Clippy because dogfood enables clippy::pedantic on the Clippy code itself.

It can happen that a new lint triggers on Clippy code. The lint implementer usually just updates the code to conform with the new lint. It's basically a self improving machine where each lint also improves Clippy itself. Your observation with if_chain! would for instance be too much and the suggested fix would look weird. @_flip1995 already mentioned a potential solution on Zulip (Sorry for splitting the discussion into two channels. I think we can keep being on Zulip, but I wanted to answer the questions you had here 🙃 ).


I suggest that you first try his suggestion with checking the snippet's last character for an }. Then take a look at this false positive where Clippy lints on if expressions:

44 |                 for &ext in &["stdout", "stderr", "fixed"] {
   |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add `;`: `&["stdout", "stderr", "fixed"];`

Afterwards there should only be a few reports left. Examples like this following one are exactly what the lint is made for and can be fixed by adding a ; at the end. (If the amount is reasonable which it should be).

 error: add `;` to terminate block
  --> clippy_lints/src/verbose_file_reads.rs:48:13
   |
48 | /             span_lint_and_help(
49 | |                 cx,
50 | |                 VERBOSE_FILE_READS,
51 | |                 expr.span,
...  |
54 | |                 "consider using `fs::read_to_string` instead",
55 | |             )
   | |_____________^

One more side note: This new lint should be included in the Change log currently you have changelog: none in your PR description. Maybe change it so something like:

changelog: Added a new lint: `SEMICOLON_IF_NOTHING_RETURNED`

This makes it easier for the one writing the change log for the next release 🙃

clippy_lints/src/eq_op.rs Outdated Show resolved Hide resolved
clippy_lints/src/semicolon_if_nothing_returned.rs Outdated Show resolved Hide resolved
clippy_lints/src/semicolon_if_nothing_returned.rs Outdated Show resolved Hide resolved
clippy_lints/src/semicolon_if_nothing_returned.rs Outdated Show resolved Hide resolved
@1c3t3a 1c3t3a marked this pull request as ready for review February 6, 2021 20:24
@1c3t3a
Copy link
Contributor Author

1c3t3a commented Feb 6, 2021

Oke, so the lint is now working. In an earlier version the lint was (falsely) triggered on those lines, but after resolving the snippet in another way, this did not come while testing. Thanks again to @xFrednet and @flip1995 for the great support!

@phansch
Copy link
Member

phansch commented Feb 7, 2021

LGTM, thanks!

@bors r=xFrednet,flip1995,phansch

@bors
Copy link
Collaborator

bors commented Feb 7, 2021

📌 Commit 6b4789d has been approved by xFrednet,flip1995,phansch

@bors
Copy link
Collaborator

bors commented Feb 7, 2021

⌛ Testing commit 6b4789d with merge 001185d...

@bors
Copy link
Collaborator

bors commented Feb 7, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet,flip1995,phansch
Pushing 001185d to master...

@bors bors merged commit 001185d into rust-lang:master Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint: Have a semicolon on the last block statement if it returns nothing
6 participants