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
5 of 13 tasks
flip1995 opened this issue Jul 15, 2024 · 12 comments
Open
5 of 13 tasks

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

flip1995 opened this issue Jul 15, 2024 · 12 comments
Assignees
@scottgerring
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 by y21
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
Merged
@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
@scottgerring
Copy link
Contributor

Hey @flip1995 , I reckon i've got my head around these. If you're happy for me to pick up the rest, feel free to assign the issue to me!
Would it be easier to continue doing PR-per-lint or should I bundle them up?

@flip1995
Copy link
Member Author

flip1995 commented Dec 4, 2024

You can do PR-per lint, that's easier to review. I'll try to get to reviewing them as fast as I can, but that is currently quite slow. Sorry about that 😐

@flip1995
Copy link
Member Author

flip1995 commented Dec 4, 2024

FYI: You can self-assign issues by writing a comment with

@rustbot claim

in it.

@scottgerring
Copy link
Contributor

@rustbot claim

This was referenced Dec 6, 2024
Open
Merged
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 6, 2024
@Jarcho
chore: use multipart_suggestions for str_splitn (#13789)
5198941 
This addresses #13099 for the manual_split_once test, using the
str_splitn lint.

changelog: [str_splitn]: Updated str_splitn to use multipart_suggestions
where appropriate
github-merge-queue bot pushed a commit that referenced this issue Dec 8, 2024
@llogiq
chore: use multipart_suggestions for manual_async_fn (#13788)
c4aeb32 
This addresses #13099 for the manual_async_fn test.

changelog: [manual_async_fn]: Updated manual_async_fn to use
multipart_suggestions where appropriate
@scottgerring scottgerring mentioned this issue Dec 9, 2024
Open
github-merge-queue bot pushed a commit that referenced this issue Dec 10, 2024
@y21
Fix: fixed multipart_suggestion in index_refutable_slice uitest (#13727)
2a28347 
This should address #13099 for the derivable_impls test. As this
combines everything into a single multipart_suggestion, the feedback
message is a little less "targeted" than it was before, but now it
provides a complete`--fix`able suggestion - e.g.:

```
error: this binding can be a slice pattern to avoid indexing
  --> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:5:17
   |
LL |     if let Some(slice) = slice {
   |                 ^^^^^
   |
note: the lint level is defined here
  --> tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs:1:9
   |
LL | #![deny(clippy::index_refutable_slice)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: replace the binding and indexed access with a slice pattern
   |
LL ~     if let Some([_, _, _, _, _, _, _, slice_7, ..]) = slice {
LL |
LL |         // This would usually not be linted but is included now due to the
LL |         // index limit in the config file
LL ~         println!("{}", slice_7);
   |
```

changelog: [index_refutable_slice]: Fixed multipart_suggestions to
provide correct rustfix-able lint
@scottgerring scottgerring mentioned this issue Dec 13, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Dec 14, 2024
@xFrednet
chore: use multipart_suggestion in significant_drop_tightening lint (#…
5ac1805 
…13823)

This addresses #13099 for
the significant_drop_tightening lint.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@scottgerring scottgerring

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