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

new lint: init-numbered-fields #8170

Merged
merged 1 commit into from
Dec 27, 2021
Merged

new lint: init-numbered-fields #8170

merged 1 commit into from
Dec 27, 2021

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Dec 25, 2021 •
edited
Loading

This fixes #7985.

r? @xFrednet

changelog: new lint: [init_numbered_fields]

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 25, 2021
@llogiq llogiq force-pushed the numbered-fields branch 2 times, most recently from 9940d5f to d6656c3 Compare December 25, 2021 17:45
@alercah
Copy link

alercah commented Dec 25, 2021

🎉

Will this be suppressed in macros by default? I think that macros might be a legitimate use of numbered fields to let them treat tuples like other structs.

@llogiq
Copy link
Contributor Author

llogiq commented Dec 25, 2021 •
edited
Loading

Good point, I've added a macro check.

xFrednet
xFrednet reviewed Dec 26, 2021
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

TIL that you can construct tuples like structs ^^. The implementation looks good to me, it would be good if you could add one more test to see if the suggestion is correct for out of order fields 🙃. Then it's ready to be merged 👍

clippy_lints/src/numbered_fields.rs Outdated Show resolved Hide resolved
tests/ui/numbered_fields.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Dec 26, 2021 •
edited
Loading

Do we maybe want to change the lint name to something more precise? When I first read it, I thought this would be linting struct definitions like struct A {1: u32, 2: u32} and not the struct like initialization for tuples. I like the name struct-init-for-tuples or struct-like-init-for-tuples

@llogiq
Copy link
Contributor Author

llogiq commented Dec 26, 2021

Numbers are not valid field names in struct definitions, so I don't think we need to explicitly rule out that case. And I'd like to keep it short. If we were to be exact, it would need to be called struct-init-for-tuple-struct or something like it.

@xFrednet
Copy link
Member

If we want to keep it short then I have one more suggestion what about init_numbered_fields? 🙃

@llogiq llogiq changed the title new lint: numbered-fields new lint: init-numbered-fields Dec 26, 2021
@llogiq
Copy link
Contributor Author

llogiq commented Dec 26, 2021

Ok, renamed. I kept the test's name though. Is that OK with you?

xFrednet
xFrednet approved these changes Dec 26, 2021
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Ok, renamed. I kept the test's name though. Is that OK with you?

Sure, thank you for renaming the lint, I like this name more!

@xFrednet
Copy link
Member

And thank you for the lint implementation! 👍

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 26, 2021

📌 Commit 3ebd2bc has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented Dec 26, 2021

⌛ Testing commit 3ebd2bc with merge 0bc8680...

bors added a commit that referenced this pull request Dec 26, 2021
@bors
Auto merge of #8170 - rust-lang:numbered-fields, r=xFrednet
0bc8680 
new lint: `init-numbered-fields`

This fixes #7985.

r? `@xFrednet`

---

changelog: new lint: [`init_numbered_fields`]
@bors
Copy link
Collaborator

bors commented Dec 26, 2021

💥 Test timed out

@xFrednet
Copy link
Member

@bors retry

@bors
Copy link
Collaborator

bors commented Dec 26, 2021

⌛ Testing commit 3ebd2bc with merge dd19724...

bors added a commit that referenced this pull request Dec 26, 2021
@bors
Auto merge of #8170 - rust-lang:numbered-fields, r=xFrednet
dd19724 
new lint: `init-numbered-fields`

This fixes #7985.

r? `@xFrednet`

---

changelog: new lint: [`init_numbered_fields`]
@bors
Copy link
Collaborator

bors commented Dec 27, 2021

💥 Test timed out

@llogiq
Copy link
Contributor Author

llogiq commented Dec 27, 2021

That is weird. I didn't have any problems when running the tests locally.

@xFrednet
Copy link
Member

Bors is acting up a bit today, the last PR merged in rust-lang/rust was also yesterday. I'm guessing we can try another retry

@bors retry

@bors
Copy link
Collaborator

bors commented Dec 27, 2021

⌛ Testing commit 3ebd2bc with merge 621944a...

bors added a commit that referenced this pull request Dec 27, 2021
@bors
Auto merge of #8170 - rust-lang:numbered-fields, r=xFrednet
621944a 
new lint: `init-numbered-fields`

This fixes #7985.

r? `@xFrednet`

---

changelog: new lint: [`init_numbered_fields`]
@bors
Copy link
Collaborator

bors commented Dec 27, 2021

💔 Test failed - checks-action_remark_test

@matthiaskrgr
Copy link
Member

@bors retry

@bors
Copy link
Collaborator

bors commented Dec 27, 2021

⌛ Testing commit 3ebd2bc with merge adba132...

@bors
Copy link
Collaborator

bors commented Dec 27, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing adba132 to master...

@bors bors merged commit adba132 into master Dec 27, 2021
@llogiq llogiq deleted the numbered-fields branch December 27, 2021 21:31
@camsteffen camsteffen mentioned this pull request Jan 4, 2022
Merged
bors added a commit that referenced this pull request Jan 5, 2022
@bors
Auto merge of #8223 - camsteffen:remove-in-macro, r=llogiq
d5dcda2 
Remove in_macro from clippy_utils

changelog: none

Previously done in #7897 but reverted in #8170. I'd like to keep `in_macro` out of utils because if a span is from expansion in any way (desugaring or macro), we should not proceed without understanding the nature of the expansion IMO.

r? `@llogiq`
@flihp flihp mentioned this pull request Mar 18, 2022
Merged
@nyospe nyospe mentioned this pull request Apr 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lint suggestion: explicit construction of tuple struct by named fields
6 participants
@llogiq @alercah @xFrednet @bors @matthiaskrgr @rust-highfive