Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Amend style guide section for formatting where clauses in type aliases #114901
Changes from 2 commits
85254c9
f707b9c
2ff14b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, whilewhere
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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