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

remove mutability of fn args that don't need it #114996

Closed

Conversation

matthiaskrgr
Copy link
Member

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2023

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

@jackh726
Copy link
Contributor

This touches a lot more of the codebase than I am comfortable reviewing, I think. It may be a seemingly innocuous change, but there may be cases that the mutability is from past or future change that requires it.

As always with these kind of "change the world" types of changes, I'd rather see some kind of lint that helps to automatically catch these, rather than a one-off change.

@matthiaskrgr
Copy link
Member Author

This is a clippy lint, but we don't run clippy on ci here.

@matthiaskrgr
Copy link
Member Author

I'm not sure if it is a good idea to run clippy on rustc in ci.

Pros:
more coverage for clippy, I have actually a PR up that adds a "run clippy on rustc" to the clippy ci. Rustc has a ton of sophisticated code and this sometimes causes clippy to hang or crash (sometimes caused by changes in clippy, sometimes caused by changes in rustc) and helps catch these bugs early on

you can deny clippy lints in rustc to your hearts content

cons: adding clippy to rustc CI also means, if your rustc change accidentally causes clippy to report FPs on rustc, you will have to fix these as well (inside clippy), to get "green ci" again, which might be a pain (a pro for the clippy team of course :P)

this might add around 5-10 minutes of runtime to rustc ci for the job that also has to run clippy now

right now (there was a pr to change that) you are only guaranteed to run master-clippy on rustc master (versions must match), so you'd have

  • build rustc
  • build clippy
  • run that clippy on rustc again,
    otherwise you might get a ton of compiler errors.

people might start sprinkling allow(clippy::...) around in the rustc code base

@jackh726
Copy link
Contributor

I think approaching this as a "run clippy on rustc" point of view is not a bad approach, and better than this one-off change. For that, an MCP would be good.

For this PR, like I said, I personally don't know that I want to r+ this. You can reroll and see if whoever next disagrees with me, but shrug.

@bors
Copy link
Contributor

bors commented Sep 18, 2023

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

@jackh726 jackh726 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 Oct 4, 2023
@jackh726
Copy link
Contributor

I'm going to reassign to another compiler member, because of my thoughts here. Basically, this type of refactoring makes more sense as tool-driven, where there is both a clear criteria for the change and there will be little chance of accidental regressions over time.

r? compiler

@rustbot rustbot assigned TaKO8Ki and unassigned jackh726 Dec 23, 2023
@Dylan-DPC
Copy link
Member

@matthiaskrgr what's the status of this? thanks

@Dylan-DPC Dylan-DPC closed this Mar 29, 2024
@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants