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 non_camel_case_types for screaming single-words #116389

Closed
wants to merge 7 commits into from

Conversation

sjwang05
Copy link
Contributor

@sjwang05 sjwang05 commented Oct 3, 2023

Fixes #116336

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

@rust-log-analyzer

This comment has been minimized.

library/std/src/sys/personality/dwarf/eh.rs Outdated Show resolved Hide resolved
library/std/src/path.rs Show resolved Hide resolved
library/core/src/intrinsics.rs Outdated Show resolved Hide resolved
library/alloc/src/collections/btree/set_val.rs Outdated Show resolved Hide resolved
compiler/stable_mir/src/lib.rs Outdated Show resolved Hide resolved
library/core/src/iter/mod.rs Show resolved Hide resolved
compiler/rustc_lint/src/nonstandard_style.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/nonstandard_style.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

This does look like an improvement to me.
I'm just not sure whether we can make this change at this point though. after treating ABC as a standard style for many years.
@rustbot author

@rustbot rustbot 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 11, 2023
@sjwang05 sjwang05 force-pushed the non-camel-case-types-fix branch from 9d04f23 to c687d70 Compare October 12, 2023 20:55
@rustbot
Copy link
Collaborator

rustbot commented Oct 12, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

@rust-log-analyzer

This comment has been minimized.

@sjwang05 sjwang05 force-pushed the non-camel-case-types-fix branch from c687d70 to aff01cf Compare October 12, 2023 21:09
@rust-log-analyzer

This comment has been minimized.

@sjwang05 sjwang05 force-pushed the non-camel-case-types-fix branch from aff01cf to 7dde3ef Compare October 12, 2023 21:14
@rust-log-analyzer

This comment has been minimized.

@sjwang05 sjwang05 force-pushed the non-camel-case-types-fix branch from 7dde3ef to 779e041 Compare October 12, 2023 22:24
@rustbot rustbot added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Oct 12, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 12, 2023

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@sjwang05
Copy link
Contributor Author

Thanks for all the feedback!

I'm just not sure whether we can make this change at this point though. after treating ABC as a standard style for many years.

I can definitely see that--I think I've fixed most of the violations I could find, though it feels like new ones keep popping up whenever I do 😅 . If you (or anyone else) see fit, I'd be more than willing to close this PR because of this.

@rustbot review

@petrochenkov
Copy link
Contributor

I'm going to nominate this for the lang team.

A relaxed version of this PR could be bumping the length limit from 3 letters to 4.
E.g. ABC would still be considered camel case, but ABCD/ABCDF/etc not.

@petrochenkov petrochenkov added I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2023
@bors
Copy link
Contributor

bors commented Oct 17, 2023

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

@m-ou-se m-ou-se removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 18, 2023
@scottmcm
Copy link
Member

scottmcm commented Nov 8, 2023

Some prior art: C#'s rules require upper for two-letter acronyms, like System.IO but not for longer ones, like System.Xml.

(No idea if that's good or not, but it came to mind.)

@scottmcm
Copy link
Member

scottmcm commented Nov 8, 2023

We discussed this in the lang triage meeting today.

While many felt that most of the changes in the PR "felt more Rust-y", there was lots of concern about how much churn this might be pushing on people, based on all the edits needed in the PR.

Can you say more about the motivation here? How essential is this?


Personally, the note from #116389 (comment) about maybe moving the threshold for complaining about it seems potentially interesting as a way to have this handle the most egregious cases without being so noisy.

@sjwang05
Copy link
Contributor Author

Can you say more about the motivation here? How essential is this?

I'd say the main motivation here (as mentioned in the original issue) is just to be more compliant with RFC430, though I wouldn't say it's essential, especially for 3-letter screaming words.

Personally, the note from #116389 (comment) about maybe moving the threshold for complaining about it seems potentially interesting as a way to have this handle the most egregious cases without being so noisy.

I agree--I think raising the limit to 4 upper-case characters would be a good compromise, though I'll leave the final call on what to do up to you all.

@traviscross
Copy link
Contributor

We discussed this in the T-lang meeting today alongside #60570. The feeling was that simple heuristics aren't powerful enough to reliably check whether or not some string is following the camel case style since, e.g., a series of upper-case letters may be a series of single letter words or may be an initialism (rather than acronym) that it may be more appropriate to leave capitalized. As @joshtriplett explains over in #60570:

I don't think we should encourage INTEGER_1, but I do think it's completely valid to write arbitrarily-long strings of capital letters in "camel case". If someone writes IFAClient (example made up on the spot), we have no way of knowing if that's "IFA Client" (in which case officially by our rules you're "supposed" to write IfaClient), or "I F A Client" (e.g. Iterator of Futures of Async-wrapped Client). And since we can't distinguish those, we should be more permissive.

And as @scottmcm noted:

rb"foo" being an RBString and not getting linted seems entirely fine.

People also felt it might just be too opinionated for rustc to enforce Http and Html over HTTP and HTML, and that if desired, these may be better as clippy lints. People were also concerned about unnecessary churn here.

With this context, a proposal for FCP close will be forthcoming.

@tmandry tmandry added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 15, 2023
@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Nov 15, 2023
@tmandry
Copy link
Member

tmandry commented Nov 15, 2023

On the basis of the above comment,

@rfcbot fcp close

@rfcbot
Copy link

rfcbot commented Nov 15, 2023

Team member @tmandry has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Nov 15, 2023
@traviscross traviscross removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Nov 15, 2023
@petrochenkov petrochenkov added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Nov 15, 2023
@scottmcm
Copy link
Member

@rfcbot reviewed

But as I said earlier (#116389 (comment)), I might still be interested in different tweaks at different points in the "how confident can we be" spectrum. I don't have enough data to know where that would be, but at some point I think the odds that IOMGWTFBBQWidget is actually legitimate camel case are low enough that I'd be fine linting against it, even if technically it could have false positives. (That said, the odds of that catching anything meaningful might also be low enough that doing work for it wouldn't be worth bothering either. I don't know.)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 10, 2024
@rfcbot
Copy link

rfcbot commented Jan 10, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 20, 2024
@rfcbot
Copy link

rfcbot commented Jan 20, 2024

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

non_camel_case_types does not trigger for screaming single-word