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

style-guide: Format single associated type where clauses on the same line #119515

Merged

Conversation

joshtriplett
Copy link
Member

In particular, lifetime-generic associated types often have a
where Self: 'a bound, which we can format on the same line.

@joshtriplett joshtriplett added I-style-nominated Nominated for discussion during a style team meeting. T-style Relevant to the style team, which will review and decide on the PR/issue. labels Jan 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2024

r? @yaahc

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2024

Some changes occurred in src/doc/style-guide

cc @rust-lang/style

@joshtriplett joshtriplett force-pushed the style-guide-gat-where-clause-same-line branch from 5010ff3 to 6324736 Compare January 11, 2024 03:21
@bors
Copy link
Contributor

bors commented Jan 11, 2024

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

@joshtriplett
Copy link
Member Author

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jan 12, 2024

Team member @joshtriplett has proposed to merge 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!

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-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 12, 2024
@joshtriplett joshtriplett removed the I-style-nominated Nominated for discussion during a style team meeting. label Jan 17, 2024
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jan 24, 2024
@rfcbot
Copy link

rfcbot commented Jan 24, 2024

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 24, 2024
@pitaj
Copy link
Contributor

pitaj commented Jan 31, 2024

Just curious about the reasoning here

    fn count(self) -> usize
    where
        Self: Sized,
    {
        todo!()
    }

Why exclude functions with bodies, and why not allow Self: Sized to be on the same line as the where?

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 3, 2024
@rfcbot
Copy link

rfcbot commented Feb 3, 2024

The final comment period, with a disposition to merge, 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.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Feb 3, 2024
@compiler-errors
Copy link
Member

Please rebase, then r=me.

@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 Feb 5, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 8, 2024
@Dylan-DPC
Copy link
Member

@joshtriplett any updates on this? this is ready for a merge and has been approved but requires a rebase before that

…e line

In particular, lifetime-generic associated types often have a
`where Self: 'a` bound, which we can format on the same line.
@joshtriplett joshtriplett force-pushed the style-guide-gat-where-clause-same-line branch from a74515a to 163b1a6 Compare May 13, 2024 14:43
@joshtriplett
Copy link
Member Author

Rebased.

@joshtriplett
Copy link
Member Author

@bors merge r=compiler-errors

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2024

📌 Commit 163b1a6 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2024
fmease added a commit to fmease/rust that referenced this pull request May 13, 2024
…-clause-same-line, r=compiler-errors

style-guide: Format single associated type `where` clauses on the same line

In particular, lifetime-generic associated types often have a
`where Self: 'a` bound, which we can format on the same line.
@joshtriplett
Copy link
Member Author

joshtriplett commented May 13, 2024

@pitaj This was intended as a narrow fix for making GATs simpler to write and read. The rationale for excluding functions with bodies is the same as the rationale for excluding types that have an =, namely trying to space things out for readability.

Separate from that: Putting a function's where Single: Bound on a single line was a request made of the original Rust style as well. Part of the rationale against was that when you have a long and complex fn function(...) -> Type line, having a single-line where next to that can blend together and prove hard to read, whereas a blank line seems to help with that. (Your count example is a simple function with a simple where clause; consider a more complex function than count.) We could reconsider that, but I feel like that rationale still applies.

@pitaj
Copy link
Contributor

pitaj commented May 13, 2024

A more complex function signature would also be more likely to spill into the next line.

The argument seems to apply equally regardless of whether the function has a body or not.

@joshtriplett
Copy link
Member Author

The argument seems to apply equally regardless of whether the function has a body or not.

The argument I wrote above was in response to the idea of writing

    fn count(self) -> usize
    where Self: Sized
    {
        todo!()
    }

I'd make a different argument about why not to write:

    fn count(self) -> usize where Self: Sized
    {
        todo!()
    }

That argument would mostly be "that function already has a body, so it's already spread over several lines and has a blank line separating it from other functions, so the incremental value of putting where Self: Sized on the same line is low compared to the readability cost of doing so".

By contrast, if you have:

    fn count(self) -> usize where Self: Sized;
    fn recount(self) -> usize where Self: Sized;
    fn turn_purple(&mut self);

I think that's more readable than breaking each of the where clauses onto subsequent lines.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 13, 2024
…-clause-same-line, r=compiler-errors

style-guide: Format single associated type `where` clauses on the same line

In particular, lifetime-generic associated types often have a
`where Self: 'a` bound, which we can format on the same line.
bors added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#119515 (style-guide: Format single associated type `where` clauses on the same line)
 - rust-lang#119959 ([meta] Clarify prioritization alert)
 - rust-lang#123817 (Stabilize `seek_seek_relative`)
 - rust-lang#124532 (elaborate obligations in coherence)
 - rust-lang#125063 (Don't call `env::set_var` in `rustc_driver::install_ice_hook`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#119515 (style-guide: Format single associated type `where` clauses on the same line)
 - rust-lang#119959 ([meta] Clarify prioritization alert)
 - rust-lang#123817 (Stabilize `seek_seek_relative`)
 - rust-lang#125063 (Don't call `env::set_var` in `rustc_driver::install_ice_hook`)
 - rust-lang#125071 (Migrate rustdoc target spec json path)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4d1cce9 into rust-lang:master May 13, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2024
Rollup merge of rust-lang#119515 - joshtriplett:style-guide-gat-where-clause-same-line, r=compiler-errors

style-guide: Format single associated type `where` clauses on the same line

In particular, lifetime-generic associated types often have a
`where Self: 'a` bound, which we can format on the same line.
@joshtriplett joshtriplett deleted the style-guide-gat-where-clause-same-line branch May 14, 2024 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-style Relevant to the style team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants