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

various clippy fixes #121543

Merged
merged 5 commits into from
Mar 20, 2024
Merged

various clippy fixes #121543

merged 5 commits into from
Mar 20, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Feb 24, 2024

We need to keep the order of the given clippy lint rules before passing them.
Since clap doesn't offer any useful interface for this purpose out of the box,
we have to handle it manually.

Additionally, this PR makes -D rules work as expected. Previously, lint rules were limited to -W. By enabling -D, clippy began to complain numerous lines in the tree, all of which have been resolved in this PR as well.

Fixes #121481
cc @matthiaskrgr

@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2024

r? @clubby789

rustbot has assigned @clubby789.
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
Copy link
Collaborator

rustbot commented Feb 24, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 24, 2024
@matthiaskrgr
Copy link
Member

Looks good so far :) thanks!
Side note: it seems that -D and -F do not have any effect, probably due to --cap-lints warn which we also pass?

Comment on lines 69 to 70
args.extend(ignored_lints.iter().map(|lint| format!("-Aclippy::{}", lint)));
let mut clippy_lint_levels: Vec<String> = Vec::new();
allow.iter().for_each(|v| clippy_lint_levels.push(format!("-A{}", v)));
deny.iter().for_each(|v| clippy_lint_levels.push(format!("-D{}", v)));
warn.iter().for_each(|v| clippy_lint_levels.push(format!("-W{}", v)));
forbid.iter().for_each(|v| clippy_lint_levels.push(format!("-F{}", v)));
args.extend(clippy_lint_levels);
args.extend(get_clippy_rules_in_order(&all_args, allow, deny, warn, forbid));
Copy link
Member

Choose a reason for hiding this comment

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

like this, the manual lints passed to x.py clippy will always override the ignored lints.

I'm not sure if we have any other options than either make x.py always override the ignored lints or make the ignored lints always override the manual ones, so the way it is currently seems to be the less painful option..?

Copy link
Member Author

Choose a reason for hiding this comment

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

7745deb should be appropriate solution I think?

@onur-ozkan
Copy link
Member Author

Side note: it seems that -D and -F do not have any effect, probably due to --cap-lints warn which we also pass?

6e8042b should fix that.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@onur-ozkan onur-ozkan 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 Feb 24, 2024
@onur-ozkan
Copy link
Member Author

onur-ozkan commented Feb 24, 2024

Now clippy complains a lot of stuff because the -D rules have never been functional before. I will switch back this PR to S-waiting-on-review label once I resolve the lint errors in the tree.

@onur-ozkan onur-ozkan marked this pull request as draft February 24, 2024 14:35
@onur-ozkan onur-ozkan changed the title reorder clippy rules to their original order before passing them various clippy fixes Feb 24, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino, @ouz-a

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@onur-ozkan
Copy link
Member Author

@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 Mar 14, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@@ -671,11 +671,11 @@ pub fn read_target_uint(endianness: Endian, mut source: &[u8]) -> Result<u128, i
// So we do not read exactly 16 bytes into the u128, just the "payload".
let uint = match endianness {
Endian::Little => {
source.read(&mut buf)?;
let _ = source.read(&mut buf)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

read_exact here, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I already tried that and got failures in CI https://github.com/rust-lang/rust/actions/runs/8307751008/job/22737303335#step:27:1679. And that actually makes sense as trying to fill entire [0u8; std::mem::size_of::<u128>()] will fail for most cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, you need to use the same [16 - source.len()..] logic used in the Endian::Big branch (or even better, move that logic before the match endianess

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is to cover the logic for the "most significant byte" and we shouldn't do the same in Little branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah makes sense. Then you need to slice ..source.len() before using read_exact

Tho maybe we should just use one of the slice copying functions instead of using the Read trait

@bors
Copy link
Contributor

bors commented Mar 17, 2024

☔ The latest upstream changes (presumably #122637) made this pull request unmergeable. Please resolve the merge conflicts.

We need to keep the order of the given clippy lint rules before passing them.
Since clap doesn't offer any useful interface for this purpose out of the box,
we have to handle it manually.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Previously, when passing lint rules manually using `x clippy ..`, ignored lints would
override manual ones. This change corrects the order by passing ignored lints after the
manual ones.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@oli-obk
Copy link
Contributor

oli-obk commented Mar 19, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 19, 2024

📌 Commit 81d7d7a has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#121543 (various clippy fixes)
 - rust-lang#122540 (Do not use `?`-induced skewing of type inference in the compiler)
 - rust-lang#122730 (Expose `ucred::peer_cred` on QNX targets to enable dist builds)
 - rust-lang#122732 (Remove redundant coroutine captures note)
 - rust-lang#122739 (Add "put" as a confusable for insert on hash map/set)
 - rust-lang#122748 (Reduce `pub` usage in `rustc_session`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4f3050b into rust-lang:master Mar 20, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 20, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 20, 2024
Rollup merge of rust-lang#121543 - onur-ozkan:clippy-args, r=oli-obk

various clippy fixes

We need to keep the order of the given clippy lint rules before passing them.
Since clap doesn't offer any useful interface for this purpose out of the box,
we have to handle it manually.

Additionally, this PR makes `-D` rules work as expected. Previously, lint rules were limited to `-W`. By enabling `-D`, clippy began to complain numerous lines in the tree, all of which have been resolved in this PR as well.

Fixes rust-lang#121481
cc `@matthiaskrgr`
@onur-ozkan onur-ozkan deleted the clippy-args branch March 20, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x.py clippy argument passing broken, order no loger preserved
7 participants