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

Only emit useless_vec suggestion if the macro does not contain code comments #13911

Merged
merged 2 commits into from
Jan 4, 2025

Conversation

GuillaumeGomez
Copy link
Member

Fixes #13692.

If the vec! macro call contains comments, we should not provide suggestions and let users handle it however they see fit.

changelog: Only emit useless_vec suggestion if the macro does not contain code comments

@rustbot
Copy link
Collaborator

rustbot commented Dec 30, 2024

r? @xFrednet

rustbot has assigned @xFrednet.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 30, 2024
LL | |
LL | | 1, 2, // i'm here to stay
LL | | 3, 4, // but this one going away ;-;
LL | | ]; // that is life anyways
Copy link
Member

Choose a reason for hiding this comment

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

IMO the help message/suggestion was a fairly important part of the lint because now it's not 100% clear what the lint wants the user to do from the diagnostic alone (use an array instead).

What about just changing the applicability to something else other than MachineApplicable so that --fix won't autofix it but is still shown to the user as a hint to what the fix roughly looks like and they can apply it manually by preserving the comment?

(I guess ideally the suggestion could just include the last comment directly and keep it as MachineApplicable since all other comments except for the one after the last element are already correctly handled, but I can only think of hacky ways to do it like extending the snippet's span to the full vec![] invocation minus 1 (just behind the ]), and am not sure if that will always work so 🤷)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah makes sense. Gonna change the applicability instead.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @y21 that it's better to only change the applicability, like you did now. (Thank you for the good suggestion!)

I would be in favor of expanding the snippet span instead. I don't really see when this should fail. Modulo macro expansion, which will probably make this a lot harder ^^.

I checked the code and don't see a super obvious way to hack it in. I don't think it's worth it to spent too much time on this, since changing the applicability already prevents automatic deletion of the comments.

So I'd say we can merge this once the comment (from my other review comment) has been updated :D

clippy_lints/src/vec.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

@rustbot ready

Comment on lines 137 to 139
// If the `vec!` macro contains comment, better not emit any suggestion as it
// would remove them. Let's just lint that the `vec!` is useless and let the user
// decides what they wanna do.
Copy link
Member

Choose a reason for hiding this comment

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

This comment is outdated:

Suggested change
// If the `vec!` macro contains comment, better not emit any suggestion as it
// would remove them. Let's just lint that the `vec!` is useless and let the user
// decides what they wanna do.
// If the `vec!` macro contains comment, better not make the suggestion machine applicable as it
// would remove them.

LL | |
LL | | 1, 2, // i'm here to stay
LL | | 3, 4, // but this one going away ;-;
LL | | ]; // that is life anyways
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @y21 that it's better to only change the applicability, like you did now. (Thank you for the good suggestion!)

I would be in favor of expanding the snippet span instead. I don't really see when this should fail. Modulo macro expansion, which will probably make this a lot harder ^^.

I checked the code and don't see a super obvious way to hack it in. I don't think it's worth it to spent too much time on this, since changing the applicability already prevents automatic deletion of the comments.

So I'd say we can merge this once the comment (from my other review comment) has been updated :D

@GuillaumeGomez
Copy link
Member Author

Updated comment.

@rustbot ready

Merged via the queue into rust-lang:master with commit ad69c65 Jan 4, 2025
9 checks passed
@GuillaumeGomez GuillaumeGomez deleted the useless_vec-fix branch January 6, 2025 16:31
github-merge-queue bot pushed a commit that referenced this pull request Jan 7, 2025
…mments (#13940)

Fixes #8528.

Similar to #13911, if there are code comments, we don't want to remove
them automatically.

changelog: Don't emit machine applicable `map_flatten` lint if there are
code comments

r? @xFrednet
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.

clippy --fix deleting informational comments
4 participants