Skip to content

make path_statements lint machine applicable for statements with no effect #140830

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

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

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented May 8, 2025

The motivation for this change is to make it easier to clean up code generated by c2rust, which tends to include a lot of unnecessary path statements.

@rustbot
Copy link
Collaborator

rustbot commented May 8, 2025

r? @nnethercote

rustbot has assigned @nnethercote.
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

@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 May 8, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: since this is machine-applicable, can you add a test case for the suggestion where the stmt span constitutes of macro_call!();, where macro_call!()-the-expr-span might have a different syntax context?

(Unless this already exists, ofc)

@nnethercote
Copy link
Contributor

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2025
@yaahc yaahc force-pushed the machine-applicable-path-statments branch from 4096867 to ba2de32 Compare June 3, 2025 20:38
@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2025

⚠️ Warning ⚠️

@yaahc yaahc force-pushed the machine-applicable-path-statments branch from ba2de32 to 6ababd0 Compare June 3, 2025 20:48
};
}

foo!(x);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jieyouxu is this what you meant? Right now this is emitting the lint. Is the idea that this lint shouldn't be emitted the lint in this case or that it shouldn't be machine applicable?

Copy link
Member

@jieyouxu jieyouxu Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I somehow completely lost this notification, sorry about that. Now that I read my own comment #140830 (review) back, I think what I had in mind at the time was stuff like having a path statement that comes from a macro expansion from another crate, where I think I thought that a machine-applicable suggestion in that case wouldn't be very actionable.

... But that can be a follow-up or further adjusted as suitable, and does not need to block this PR at all.

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu-llvm-19-1 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
failures:

---- [ui] tests/ui/lint/warn-path-statement.rs stdout ----

error: /checkout/tests/ui/lint/warn-path-statement.rs:20: unexpected ERROR: '20:13: 20:16: path statement with no effect [path_statements]'

error: 1 unexpected errors found, 0 expected errors not found
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/aarch64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/lint/warn-path-statement.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/aarch64-unknown-linux-gnu/stage2" "--target=aarch64-unknown-linux-gnu" "--check-cfg" "cfg(test,FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/aarch64-unknown-linux-gnu/test/ui/lint/warn-path-statement" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/aarch64-unknown-linux-gnu/native/rust-test-helpers" "-D" "path-statements"

--- unexpected errors (from JSON output) ---
ERRORline  20: 20:13: 20:16: path statement with no effect [path_statements]
---

thread '[ui] tests/ui/lint/warn-path-statement.rs' panicked at src/tools/compiletest/src/runtest.rs:790:13:
errors differ from expected
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@yaahc
Copy link
Member Author

yaahc commented Jun 16, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 16, 2025
@nnethercote
Copy link
Contributor

r? @jieyouxu

@rustbot
Copy link
Collaborator

rustbot commented Jun 17, 2025

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

@nnethercote nnethercote removed their assignment Jun 17, 2025

macro_rules! foo {
($e:expr) => {
$e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I think you'll need a

//~^ ERROR: path statement with no effect

annotation here. I guess partly what I had in mind re. macros was also this case, where the suggestion is tied to removing $e;, though arguably maybe it's the invocation foo!(x); that should be removed.

Again, this is not blocking IMO.

@jieyouxu
Copy link
Member

Sorry about losing the notification, I just had some thoughts on the suggestion which are not blocking. Feel free to r=me after adding the missing error annotation.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

5 participants