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.

Sign up for GitHub

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

Jump to bottom

Tracking Issue: Fix auto-applicable lint suggestions by using multipart suggestions #13099

Open
2 of 13 tasks
flip1995 opened this issue Jul 15, 2024 · 8 comments
Open
2 of 13 tasks

Tracking Issue: Fix auto-applicable lint suggestions by using multipart suggestions #13099

flip1995 opened this issue Jul 15, 2024 · 8 comments
Labels
C-tracking-issue Category: Tracking Issue E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy

Comments

@flip1995
Copy link
Member

flip1995 commented Jul 15, 2024 •
edited
Loading

Description

#13098 introduced @no-rustfix annotations, that can be removed by changing the suggestion to a multipart suggestion:

//@no-rustfix: need to change the suggestion to a multipart suggestion

The affected test files are:

  • ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs
  • ui/crashes/ice-3717.rs
  • ui/derivable_impls.rs
  • ui/index_refutable_slice/if_let_slice_binding.rs
  • ui/index_refutable_slice/slice_indexing_in_macro.rs
  • ui/let_unit.rs
  • ui/manual_assert.rs
  • ui/manual_async_fn.rs
  • ui/manual_split_once.rs
  • ui/match_same_arms2.rs
  • ui/significant_drop_tightening.rs
  • ui/unnecessary_iter_cloned.rs
  • ui/unnecessary_to_owned.rs

To fix some of those:

  1. Pick one test and leave a comment about it
  2. Remove the @no-rustfix annotation
  3. Change the lint suggestion building to use multipart_suggestion over the current implementation
  4. write a comment here or mention this issue in the PR so that it gets marked as resolved.

Version

No response

Additional Labels

No response
@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-tracking-issue Category: Tracking Issue labels Jul 15, 2024
@flip1995 flip1995 mentioned this issue Jul 15, 2024
Merged
@ibilalkayy ibilalkayy mentioned this issue Aug 4, 2024
Closed
@ibilalkayy
Copy link

Hey @flip1995, I just sent a PR to solve a problem that you mentioned in this issue. Thank you!

kyoto7250 added a commit to kyoto7250/rust-clippy that referenced this issue Aug 7, 2024
@kyoto7250
add a test for ice-3717.rs
5e25e7c 
rust-lang#13099
@kyoto7250 kyoto7250 mentioned this issue Aug 7, 2024
Merged
bors added a commit that referenced this issue Aug 12, 2024
@bors
Auto merge of #13230 - kyoto7250:fix_issue_13099, r=flip1995
a616ca7 
Add a test for ice-3717.rs

this PR is a part of #13099.

Based on the changes introduced in #13098 for introduce ui_test, we will update the uitest output.
This is a fix for `ice-3717.rs`.

Although fixes have already been made in #13216, it seems that he is a first-time contributor.
I thought it might be better for him to refer to my PR, so I created it accordingly.

Since this is my first contribution in a while, please let me know if there are any issues or required changes.

changelog:
None

r! `@flip1995`
bors added a commit that referenced this issue Aug 12, 2024
@bors
Auto merge of #13230 - kyoto7250:fix_issue_13099, r=flip1995
8827107 
Add a test for ice-3717.rs

this PR is a part of #13099.

Based on the changes introduced in #13098 for introduce ui_test, we will update the uitest output.
This is a fix for `ice-3717.rs`.

Although fixes have already been made in #13216, it seems that he is a first-time contributor.
I thought it might be better for him to refer to my PR, so I created it accordingly.

Since this is my first contribution in a while, please let me know if there are any issues or required changes.

changelog: none

r! `@flip1995`
@flip1995
Copy link
Member Author

Copying a comment by @kyoto7250, that explains the tasks here a bit better than I have in the issue: #13216 (comment)

I [kyoto7250] have created a PR that fixes one of the files in this issue. #13230

In PR #13098, all tests that were expected to split into multipart files have been deleted and set to be skipped using @no-rustfix. Therefore, simply removing the annotation is not sufficient.

You remove @no-rustfix and run cargo uitest locally, the tests should fail. For more details, please refer to the following documentation:

https://github.com/rust-lang/rust-clippy/blob/master/book/src/development/writing_tests.md

In my PR, multiple files were not generated, but I believe that in this issue, it is expected that multiple files will be generated by the multipart suggestion.

@scottgerring scottgerring mentioned this issue Nov 22, 2024
Merged
@scottgerring
Copy link
Contributor

I pushed a change to address this for ui/derivable_impls.rs - if it looks sane, i'll polish up a few of the others too!

@scottgerring scottgerring mentioned this issue Nov 25, 2024
Open
@scottgerring
Copy link
Contributor

I pushed a change for ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs above too.

@scottgerring
Copy link
Contributor

Hey @flip1995 , if you have a chance, do the PRs i've pushed look good to you? I'm happy to do more of these but it looks like the PR approval lag is high and I am hesitant to continue until i've got a positive signal!

github-merge-queue bot pushed a commit that referenced this issue Nov 27, 2024
@flip1995
Fix: Use multipart_suggestion for derivable_impls (#13717)
9b0597d 
This should address #13099 for the `derivable_impls` test. As I've not
contributed to clippy before, I'd like to make sure i'm on the right
track before doing more :)

changelog: [`derivable_impls`]: Use multipart_suggestion to aggregate
feedback
@flip1995
Copy link
Member Author

Sorry for not replying. I'm also pretty backed up right now. I just found some time for giving your PRs a look. They LGTM (and I already merged one 🎉 ).

@scottgerring
Copy link
Contributor

No worries - thanks for finding the time! I'll knock a few more off in the coming days 💪

@scottgerring
Copy link
Contributor

I've started on ui/let_unit.rs but it's a bit fiddlier - i'll push a PR for this early next week.

@scottgerring scottgerring mentioned this issue Nov 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-tracking-issue Category: Tracking Issue E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@scottgerring @flip1995 @ibilalkayy