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

Fix as many clippy problems as possible #31500

Open
mrobinson opened this issue Mar 4, 2024 · 10 comments · Fixed by #31508, #31512, #31549, #31551 or #31548
Open

Fix as many clippy problems as possible #31500

mrobinson opened this issue Mar 4, 2024 · 10 comments · Fixed by #31508, #31512, #31549, #31551 or #31548
Labels
B-newcomer Newcomer-friendly issues. E-less-complex Straightforward. Recommended for a new contributor.

Comments

@mrobinson
Copy link
Member

mrobinson commented Mar 4, 2024

Steps:

  1. Get Servo building and running locally following project documentation at http://github.com/servo/servo.
  2. Run ./mach cargo-clippy
  3. Fix an issue by understanding what the error means and correcting the issue. For instance
warning: redundant field names in struct initialization
   --> components/remutex/lib.rs:168:13
    |
168 |             data: data,
    |             ^^^^^^^^^^ help: replace it with: `data`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names
    = note: `#[warn(clippy::redundant_field_names)]` on by default
In this case `data: data` can just be replaced with `data`.
  1. Open an pull request with your fix, filling out the template, and giving it a full name like the following "clippy: Fix a redundant field warning in components/remutex/lib.rs.
  2. Include a link to this issue in your pull request description body.

As this issue is large enough to be worked on in parallel, please don't ask to assign it to yourself. If you want to post fixes for a particular section of code, just check the PRs linked here to make sure that you are fixing different issues.

@mrobinson mrobinson added the E-less-complex Straightforward. Recommended for a new contributor. label Mar 4, 2024
@servo-highfive servo-highfive added the C-assigned There is someone working on resolving the issue label Mar 5, 2024
@servo servo deleted a comment from servo-highfive Mar 5, 2024
@servo servo deleted a comment from pranayrauni Mar 5, 2024
@servo servo deleted a comment from servo-highfive Mar 5, 2024
@mrobinson mrobinson removed the C-assigned There is someone working on resolving the issue label Mar 5, 2024
@Payne680
Copy link

Payne680 commented Mar 5, 2024

@mrobinson Please I will love to work on this task but can you give some resources on how to successfully run the project locally ?

@Loirooriol
Copy link
Contributor

BTW, some of the clippy warnings can be fixed automatically, e.g.

CARGO_BUILD_RUSTC=rustc cargo clippy --fix -p layout_2020 --broken-code

will apply some fixes automatically in the layout_2020 crate (and its tests). But some warnings need to be addressed manually.

Note that this can mess up the formatting, so remember to run ./mach fmt afterwards.

@mrobinson
Copy link
Member Author

BTW, some of the clippy warnings can be fixed automatically, e.g.

CARGO_BUILD_RUSTC=rustc cargo clippy --fix -p layout_2020 --broken-code

will apply some fixes automatically in the layout_2020 crate (and its tests). But some warnings need to be addressed manually.

Note that this can mess up the formatting, so remember to run ./mach fmt afterwards.

I do not recommend using clippy --fix as a new contributor actually. Sometimes it can change the logical meaning of the code that it changes, so it is better to do the fix manually. It's also a good way to start understanding a bit of the Servo code.

@eerii
Copy link
Contributor

eerii commented Mar 7, 2024

I'm going to copy this here since now the big pr is split into multiple ones, and it can be helpful so people looking to fix new warnings can know where to continue:

The prs above are fixes for most warnings until net is compiled, excluding:

  • result_unit_err: use a custom Error type instead of Result<_, ()>
  • missing_safety_doc: unsafe function's docs miss # Safety section

They also hopefully remove all clippy errors until script is compiled, so more warnings can be caught.

The next steps to tackle would be:

  • Fix the remaining net warnings
  • Create proper Error types for the first warnings
  • Write safety documentation (probably best to leave it for someone that is familiar with the unsafe code in the project)
  • Tackle script, which has 27358 ~2500 warnings and 433 10 errors

Edit: Update work that was already completed

@mrobinson mrobinson reopened this Apr 12, 2024
@sagudev sagudev reopened this Apr 18, 2024
@Loirooriol Loirooriol reopened this Apr 19, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 22, 2024
As seems #31500 still remain opened here's the next partial fix.

Fixed list: `unused_mut`, `clippy::needless_borrow`,
`clippy::match_ref_pats`, `clippy::borrow_deref_ref`, `clippy::ptr_eq`,
`clippy::unnecessary_cast`, `clippy::derivable_impls`,
`clippy::collapsible_match`, `clippy::extra_unused_lifetimes`,
`clippy::map_clone`, `clippy::manual_filter`.


- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes are part of #31500.
- [x] These changes do not require tests because are only cosmetic.
@jahielkomu
Copy link
Contributor

Hello everyone.
Is it just me or is ./mach cargo-clippy actually producing an error?

@sagudev
Copy link
Member

sagudev commented May 2, 2024

What error?

@jahielkomu
Copy link
Contributor

What error?

Thanks for asking @sagudev.
I have pasted what I get in this pastebin, https://paste.mozilla.org/Ea3ohjZt

It might be an isolated incident on my end.
Kindly advise how to go about it.

@sagudev
Copy link
Member

sagudev commented May 2, 2024

This is due to #32208, you might need to do mach clean and mach bootstrap again (also make sure to have stable internet connection).

@jahielkomu
Copy link
Contributor

This is due to #32208, you might need to do mach clean and mach bootstrap again (also make sure to have stable internet connection).

Alright, thanks again.
Let me do that.

aBit19 pushed a commit to aBit19/servo that referenced this issue May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment