Skip to content

Fix unused_unit suggestion for standalone unit expressions#16920

Open
cyphercodes wants to merge 1 commit intorust-lang:masterfrom
cyphercodes:fix-unused-unit-blank-line-16918
Open

Fix unused_unit suggestion for standalone unit expressions#16920
cyphercodes wants to merge 1 commit intorust-lang:masterfrom
cyphercodes:fix-unused-unit-blank-line-16918

Conversation

@cyphercodes
Copy link
Copy Markdown

@cyphercodes cyphercodes commented Apr 27, 2026

Fixes #16918.

This updates unused_unit to use a separate machine-applicable suggestion span for final unit expressions that are alone on their source line. The diagnostic still points at the (), but rustfix now deletes the whole standalone line so it does not leave a whitespace-only blank line behind.

Inline final () expressions keep the existing visible suggestion behavior.

changelog: [unused_unit]: avoid leaving blank lines when removing standalone final () expressions

Tests:

  • cargo fmt -- clippy_lints/src/unused_unit.rs
  • TESTNAME=unused_unit cargo uibless
  • TESTNAME=unused_unit cargo uitest

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 27, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 27, 2026

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: 7 candidates
  • 7 candidates expanded to 7 candidates
  • Random selection from Jarcho, dswij, llogiq

@github-actions
Copy link
Copy Markdown

Lintcheck changes for 4d5ff9c

Lint Added Removed Changed
clippy::unused_unit 0 0 4

This comment will be updated if you push new changes

@Gri-ffin
Copy link
Copy Markdown
Contributor

just wondering if this is clippy doing rustfmt's job? the workflow is cargo clippy --fix then cargo fmt,rustfmt would already clean up the blank line. If we're fixing whitespace artifacts here, shouldn't we also fix

pub fn foo(x: usize) -> () {
    let _ = x;
    ()}

into

pub fn foo(x: usize) {
    let _ = x;
}

instead of

pub fn foo(x: usize) {
    let _ = x;
    }

? But at that point we're just reimplementing rustfmt. just an opinion but wouldn't it be better to leave formatting to rustfmt rather than clippy patching individual whitespace cases? otherwise the lines between clippy and rustfmt might blur?

@kraktus
Copy link
Copy Markdown
Contributor

kraktus commented May 1, 2026

I also think it's not clippy's job to do fmt.

@Souradip121
Copy link
Copy Markdown
Contributor

Worth flagging that needless_return solved this exact blank line problem back in 2022 (#9416, commit 8c50dfb) by extending the suggestion span over the leading whitespace. That already set the precedent for clippy handling whitespace artifacts left behind by its own removals, so this PR follows an established pattern rather than introducing one. Also worth keeping in mind that not every project chains cargo fmt after cargo clippy --fix, so the fix output should stand on its own.

@Gri-ffin
Copy link
Copy Markdown
Contributor

Gri-ffin commented May 2, 2026

my genuine concern is clippy falling into the formatting creep problem, take eslint for example, eslint originally wasn't planning to be a forrmatter, it started as a linter too, they got there through exactly this kind of accumulated precedent, and the cost of unwinding it was huge, years of debates, deprecated rules, eventually creating harder boundaries regarding formatting, once you say yes to cleaning up a new line, you create a consistency demand across all the other lints, eventually users will start to argue that since clippy already fixes blank lines, it should also fix other formatting issues, which can eventually conflict with a user's rustfmt.toml or worse, conflict with rustfmt itself.

@kraktus
Copy link
Copy Markdown
Contributor

kraktus commented May 3, 2026

A FOSS project isn’t a court, and a preciously accepted patch does not mean further patches will be accepted or set a trend regarding the scope of the project.

Besides as a maintainer myself I know you often accept changes that aren’t really in scope/needed but the contribution has already been done, you do want to keep the contributor involved, as it is often narrow in scope in itself it won’t jeopardize the project.

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.

unused_unit suggestion includes blank lines

6 participants