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

Amend style guide section for formatting where clauses in type aliases #114901

Merged
merged 3 commits into from Sep 27, 2023

Conversation

compiler-errors
Copy link
Member

This PR has two parts:

  1. Amend wording about breaking before or after the =, which is a style guide bugfix to align it with current rustfmt behavior.
  2. Explain how to format trailing (Proposal: Change syntax of where clauses on type aliases #89122) where clauses, which are preferred in both GATs (Change location of where clause on GATs #90076) and type aliases (Unlock trailing where-clauses for lazy type aliases #114662).

r? @joshtriplett

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

rustbot commented Aug 16, 2023

Some changes occurred in src/doc/style-guide

cc @rust-lang/style

@compiler-errors compiler-errors changed the title Amend style guide section for formatting where clauses Amend style guide section for formatting where clauses in type aliases Aug 16, 2023
```

Where possible avoid `where` clauses and keep type constraints inline. Where
that is not possible split the line before and after the `where` clause (and
split the `where` clause as normal), e.g.,
that is not possible, prefer a trailing `where` clause over one that precedes
Copy link
Member

Choose a reason for hiding this comment

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

Part of what was also done in #113380 was removing some (but not all) usages of "prefer".

My interpretation of this is that this one is indeed intentional, targeted more at the human developer, aware that the clause can go in either/both position, and trying to encourage the latter because we feel like the style guide has to provide rules for both positions?

(edits: repeated typos, oh my)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should use "prefer" in any case where we expect a tool to do any kind of enforcement. But in this case, I don't think we expect a tool to convert between the two where positions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tools should not convert. Prefix WC is an error in the positions where it matters, so maybe we don't even need to tell them about preference at all. Happy to reword or remove.

Copy link
Member

Choose a reason for hiding this comment

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

Tools, including rustfmt, certainly could and it's probably worth noting that during some of the earlier rfc and implementation discussions that was explicitly suggested/requested.

I know there's differing philosophies around the should part of the question though

Copy link
Member Author

Choose a reason for hiding this comment

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

Well given that I'm probably in the position of having the least context and caring the least about this specific wording, one of y'all are free to suggest a rewording here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think (A) is the best way to go here, or to just remove the wording altogether and leave it up to the language to enforce what to do with where clause placement. The guide could perhaps just explain what to do in the cases of prefix, suffix, and prefix+suffix where clauses and take no opinions.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I don't have objections to either (A) or (C) in isolation; however, I'd object to doing (C) if we're not waiting for a new style edition, because that would produce more churn.

As for (B), I think the style guide should be providing documentation for how to format any language construct that exists in the language. If something hasn't been removed from the language, we should document how to handle it (even if the handling is (C), "convert it to something else"). We shouldn't leave things undocumented/implied.

Copy link
Member

Choose a reason for hiding this comment

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

OK, there's a bigger overriding consideration here. @compiler-errors just pointed out to me that the two locations have a semantic difference: where clauses before the = aren't enforced, while where clauses in the later position (which for item-level type aliases are currently only allowed in nightly) are enforced. So we should never move one to the other as a matter of mere style.

That means we should just describe both positions, and state that since there's a semantic difference between them, the Rust style does not express a preference or suggest moving constraints from one position to the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update, which should resolve this convo. Hopefully after that we can kick off an FCP?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, agreed with moving ahead

As for (B), I think the style guide should be providing documentation for how to format any language construct that exists in the language

I know this is more a framing/way of thinking moving forward, but I wanted to add before I forget that one potential area of nuance to this is to what degree, if any, the guide must account for formatters that operate prior to the AST validation stage (as rustfmt does), and things that aren't technically valid/exist in the language but which do pass the initial parsing phase

@compiler-errors
Copy link
Member Author

I'm just gonna go ahead and start the FCP here.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 27, 2023

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 27, 2023
@rfcbot
Copy link

rfcbot commented Aug 27, 2023

🔔 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 Aug 27, 2023
@calebcartwright
Copy link
Member

@rfcbot concern require all checkboxes

(we require a ✔️ from all team members so allow Jane time to review)

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

dtolnay commented Sep 16, 2023

Jane reviewed last week. @calebcartwright could you resolve the concern?

@calebcartwright
Copy link
Member

@rfcbot resolved require all checkboxes

@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 Sep 16, 2023
@rfcbot
Copy link

rfcbot commented Sep 16, 2023

🔔 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 Sep 26, 2023
@rfcbot
Copy link

rfcbot commented Sep 26, 2023

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.

@calebcartwright
Copy link
Member

@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Sep 27, 2023

📌 Commit 2ff14b0 has been approved by calebcartwright

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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 27, 2023
@bors
Copy link
Contributor

bors commented Sep 27, 2023

⌛ Testing commit 2ff14b0 with merge 7b4b1b0...

@bors
Copy link
Contributor

bors commented Sep 27, 2023

☀️ Test successful - checks-actions
Approved by: calebcartwright
Pushing 7b4b1b0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 27, 2023
@bors bors merged commit 7b4b1b0 into rust-lang:master Sep 27, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 27, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7b4b1b0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.1% [-3.5%, -2.8%] 3
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 630.239s -> 632.097s (0.29%)
Artifact size: 317.29 MiB -> 317.30 MiB (0.00%)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 28, 2023
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. merged-by-bors This PR was explicitly merged by bors. 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.

None yet

9 participants