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

[WIP] Register redundant_field_names and non_expressive_names as early passes #5518

Closed
wants to merge 3 commits into from
Closed

[WIP] Register redundant_field_names and non_expressive_names as early passes #5518

wants to merge 3 commits into from

Conversation

flip1995
Copy link
Member

@flip1995 flip1995 commented Apr 23, 2020 •
edited
Loading

Fixes #5356
Fixes #5521

We should move all pre_expansion_passes to early_passes, since they were soft deprecated in the rust compiler IIUC.

changelog: none

@flip1995 flip1995 requested a review from phansch April 23, 2020 22:16
@flip1995
Copy link
Member Author

No tests, since we can't really test libraries. I tested it locally and this fixed it (tRust Me™)

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Apr 23, 2020
@flip1995 flip1995 mentioned this pull request Apr 24, 2020
Closed
@phansch phansch added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Apr 25, 2020
@flip1995
Copy link
Member Author

@bors r=phansch

@bors
Copy link
Contributor

bors commented Apr 25, 2020

📌 Commit 61b0657 has been approved by phansch

@bors
Copy link
Contributor

bors commented Apr 25, 2020

🌲 The tree is currently closed for pull requests below priority 1, this pull request will be tested once the tree is reopened

flip1995 added a commit to flip1995/rust-clippy that referenced this pull request Apr 25, 2020
@flip1995
Rollup merge of rust-lang#5518 - flip1995:less_pre_expansion, r=phansch
c532d66 
Register redundant_field_names and non_expressive_names as early passes

Fixes rust-lang#5356
Fixes rust-lang#5521

We should move all pre_expansion_passes to early_passes, since they were soft deprecated in the rust compiler IIUC.

changelog: none
@flip1995 flip1995 mentioned this pull request Apr 25, 2020
Closed
@flip1995
Copy link
Member Author

@bors r-

@flip1995
Copy link
Member Author

I will get back to this ASAP.

@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-bors Status: The marked PR was approved and is only waiting bors labels Apr 25, 2020
@bors
Copy link
Contributor

bors commented Apr 27, 2020

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

@SwishSwushPow
Copy link

I am not very familiar with the GitHub PR process, but this hasn't been merged yet, has it? It is an issue I would love to see fixed and it seems the code to do this is already there? :)

@flip1995
Copy link
Member Author

The merged failed in a rollup. I haven't got back to this PR to check what causes these failures. If you have some time, you can take over here. Otherwise I try to complete this ASAP. (or ASAM, as soon as motivated)

@SwishSwushPow
Copy link

SwishSwushPow commented May 12, 2020 •
edited
Loading

So I just resolved a merge conflict with the master branch and that should be all. I just don't know how to proceed at this point. Do we update the branch on your fork and merge from there or should I open a new PR?

Edit: I've opened a new PR #5589 where I resolved the merge conflict.

@SwishSwushPow SwishSwushPow mentioned this pull request May 12, 2020
Closed
@flip1995 flip1995 force-pushed the less_pre_expansion branch from 61b0657 to ac37507 Compare May 12, 2020 14:27
@flip1995
Copy link
Member Author

Blocked on #5587.

@flip1995 flip1995 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 from the author. (Use `@rustbot ready` to update this status) labels May 12, 2020
@flip1995 flip1995 changed the title Register redundant_field_names and non_expressive_names as early passes [WIP] Register redundant_field_names and non_expressive_names as early passes May 12, 2020
@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties S-waiting-on-bors Status: The marked PR was approved and is only waiting bors labels May 12, 2020
@flip1995
Copy link
Member Author

Still failing... I think the similar_names lint requires a rewrite.

@flip1995 flip1995 force-pushed the less_pre_expansion branch from 4cd97a9 to cedd6d7 Compare May 12, 2020 14:56
@flip1995 flip1995 force-pushed the less_pre_expansion branch from cedd6d7 to ea6b330 Compare May 13, 2020 15:22
@flip1995
Copy link
Member Author

Quick update: since one lint would need a rewrite, I plan to split this up in two PRs. I don't know when I will get to this though, since I want to prioritize to review PRs by other contributors for now. If someone wants to pick this up, feel free to do so.

@ebroto
Copy link
Member

ebroto commented May 26, 2020

If someone wants to pick this up, feel free to do so.

I can work on this as there seems to be some demand (I'm assuming @phansch is assigned as a reviewer, if you're working on it it's all yours :)

We should move all pre_expansion_passes to early_passes, since they were soft deprecated in the rust compiler IIUC.

Argh, I was not aware of this and just created a new pre-expansion pass in #5643 ... Maybe it's not the best idea to merge it, I will leave a comment there to discuss.

@flip1995 do you have a link to the issue in the rust repo by any chance? I searched but found rust-lang/rust#69838 which just speaks about it IIUC

@ebroto ebroto mentioned this pull request May 26, 2020
Closed
@flip1995
Copy link
Member Author

@ebroto Great, thanks for taking over!

There is not a real issue, since the existence of the pre-expansion passes is just technical debt. Here's a link to the comment talking about this of the PR you linked: rust-lang/rust#69838 (comment)

@ebroto ebroto mentioned this pull request May 26, 2020
Merged
@flip1995 flip1995 closed this May 26, 2020
@flip1995 flip1995 deleted the less_pre_expansion branch May 26, 2020 22:56
@ebroto ebroto mentioned this pull request Oct 13, 2020
Merged
@xFrednet xFrednet mentioned this pull request May 31, 2021
Closed
@giraffate giraffate mentioned this pull request Feb 15, 2022
Merged
@xFrednet xFrednet mentioned this pull request Feb 26, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 26, 2022
@matthiaskrgr
Rollup merge of rust-lang#94397 - xFrednet:69838-deprecate-pre-expans…
18f9326 
…ion, r=cjgillot

Document that pre-expansion lint passes are softly deprecated

The pre-expansion lint pass has been softly deprecated since rust-lang#69838. Every once in a while I see someone mention it as a possibility, only get the feedback that it's deprecated. This PR officially documents that the method is soft deprecated to have a single point of truth for it.

That's it. Have a great rest of the day 🙃

---

* See [rust#69838](rust-lang#69838)
* See [rust-clippy#5518](rust-lang/rust-clippy#5518)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 27, 2022
@Dylan-DPC
Rollup merge of rust-lang#94397 - xFrednet:69838-deprecate-pre-expans…
a87dc77 
…ion, r=cjgillot

Document that pre-expansion lint passes are softly deprecated

The pre-expansion lint pass has been softly deprecated since rust-lang#69838. Every once in a while I see someone mention it as a possibility, only get the feedback that it's deprecated. This PR officially documents that the method is soft deprecated to have a single point of truth for it.

That's it. Have a great rest of the day 🙃

---

* See [rust#69838](rust-lang#69838)
* See [rust-clippy#5518](rust-lang/rust-clippy#5518)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2022
@matthiaskrgr
Rollup merge of rust-lang#94397 - xFrednet:69838-deprecate-pre-expans…
edc8b73 
…ion, r=cjgillot

Document that pre-expansion lint passes are softly deprecated

The pre-expansion lint pass has been softly deprecated since rust-lang#69838. Every once in a while I see someone mention it as a possibility, only get the feedback that it's deprecated. This PR officially documents that the method is soft deprecated to have a single point of truth for it.

That's it. Have a great rest of the day 🙃

---

* See [rust#69838](rust-lang#69838)
* See [rust-clippy#5518](rust-lang/rust-clippy#5518)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 27, 2022
@matthiaskrgr
Rollup merge of rust-lang#94397 - xFrednet:69838-deprecate-pre-expans…
9373d8b 
…ion, r=cjgillot

Document that pre-expansion lint passes are softly deprecated

The pre-expansion lint pass has been softly deprecated since rust-lang#69838. Every once in a while I see someone mention it as a possibility, only get the feedback that it's deprecated. This PR officially documents that the method is soft deprecated to have a single point of truth for it.

That's it. Have a great rest of the day 🙃

---

* See [rust#69838](rust-lang#69838)
* See [rust-clippy#5518](rust-lang/rust-clippy#5518)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phansch phansch phansch approved these changes

Assignees

@phansch phansch

Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Milestone
No milestone
5 participants
@flip1995 @bors @SwishSwushPow @ebroto @phansch