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

Initial work on formatting headers #5847

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

fee1-dead
Copy link
Member

This PR should be one of a series of PRs that formats headers. An item's header can include its keywords, its name, and its visibility. With generic header formatting facility, this would solve most, if not all issues with comments being deleted between keywords. In addition, this intends to replace the rewrite_* functions in utils.rs.

This PR is intentionally minimal, as I am not completely sure if this is desirable.

@calebcartwright
Copy link
Member

Thanks for your continued efforts on improving rustfmt! I haven't looked closely at the code changes but did want to note that I think this is ultimately the type of direction we should take.

I'm not sure about the timing/how long it may take to get through this though as I'd like to see if the Style Team would want to weigh in on how certain constructs should be formatted in the presence of comments. The existing Style Guide at times has varying prescriptions depending on whether there's a comment, but in other cases simply says "don't put comments here" which leaves things a bit ambiguous for Rustfmt when it, inevitably, finds comments in such places

src/header.rs Outdated Show resolved Hide resolved
src/header.rs Outdated Show resolved Hide resolved
@ytmimi
Copy link
Contributor

ytmimi commented Sep 1, 2023

Also, I've recently revisited #5708, and I feel that centralizing this logic, would definitely help simplify that fix. I'm in agreement with Caleb on supporting this change!

@fee1-dead fee1-dead force-pushed the format-header branch 2 times, most recently from bff5851 to 030fcff Compare September 1, 2023 14:27
@ytmimi ytmimi added blocked Blocked on rustc, an RFC, etc. and removed pr-not-reviewed labels Sep 1, 2023
@ytmimi
Copy link
Contributor

ytmimi commented Sep 1, 2023

Going to label this one as blocked for now until there's additional clarification from t-style, but I've reviewed the code and would feel comfortable merging this as is.

@fee1-dead
Copy link
Member Author

Going to label this one as blocked for now until there's additional clarification from t-style, but I've reviewed the code and would feel comfortable merging this as is.

Per rust-lang/style-team#191 (comment) I think this is ready to merge?

@ytmimi
Copy link
Contributor

ytmimi commented Mar 12, 2024

@fee1-dead thanks for staying on top of this! I'll give this another look later this week. I don't think my thoughts have change, but I want to go over it one more time before merging.

@ytmimi ytmimi added pr-follow-up-review-pending and removed blocked Blocked on rustc, an RFC, etc. labels Mar 12, 2024
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

After taking another pass at this I have two minor comments. Take a look when you get a chance.

src/header.rs Outdated Show resolved Hide resolved
src/header.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

Thanks for making those minor changes. I feel pretty good about this new header rewrite logic!

@ytmimi ytmimi added pr-ready-to-merge release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Mar 13, 2024
@ytmimi
Copy link
Contributor

ytmimi commented Mar 13, 2024

Just want to run the Diff-Check before merging.

Edit: Job passed ✅

@ytmimi ytmimi merged commit dd301b0 into rust-lang:master Mar 13, 2024
27 checks passed
@fee1-dead fee1-dead deleted the format-header branch March 13, 2024 07:49
@calebcartwright
Copy link
Member

I've not looked at this in detail, but based on our style team discussion last week I'm not sure we should have merged this.

Is this applying formatting decision in the presence of all/certain comment types within the header? Or is it simply maintaining the existing content if comments are present?

If the former, then we need to revisit and potentially revert, as the specifics of what to do are still pending (rust-lang/rust#121762)

@ytmimi
Copy link
Contributor

ytmimi commented Mar 13, 2024

@calebcartwright From my review this preserves the comments that are already there, though some re-alignment of comments may be applied, and in the case of single-line block comments the header parts will be placed on one line. @fee1-dead only applied this updated logic to macro definitions, and it should be easy to revert if these changes are undesirable.

No changes:

macro_rules! // a
// b
// c
// d
my_macro {
    () => {};
}

macro_rules! /* e
f
g
h */
my_macro {
    () => {};
}

macro_rules! /* i
* j
* k
* l
*/
my_macro {
    () => {};
}

Comments are re-aligned:

input:

macro_rules!// a
  // b
    // c
 // d
my_macro {
    () => {};
}

output:

macro_rules! // a
// b
// c
// d
my_macro {
    () => {};
}

Single-line block comments cause header parts to be placed on a single line:

Input

macro_rules! /* a block comment */
my_macro {
    () => {};
}

macro_rules! /* a really really really really really really really really really really long comment */
my_macro {
    () => {};
}

Output

macro_rules! /* a block comment */ my_macro {
    () => {};
}

macro_rules! /* a really really really really really really really really really really long comment */ my_macro {
    () => {};
}

@calebcartwright
Copy link
Member

Yes, I'd like to see this reverted until we've discussed our strategy on this front more holistically, because once we have rustfmt start doing something, we can't change it (at least not outside a style edition boundary).

I was asking about whether this was doing the same thing we do for expressions (e.g. if comment lost then use original contents), but I can see now that it's not, we're applying formatting here.

To be clear, I'm not saying the formatting we're applying is necessarily wrong, just that I think it's premature to be applying formatting logic with comments.

All that's been determined from a style perspective is (1) the style guide text should not be interpreted by formatters as a mandate to remove certain comments (which will likely necessitate updates to existing text for clarity) and (2) the style team is not going to prescribe style rules for the myriad comment types and shapes in every possible syntactical position, and as such the guide will give some level of license to formatters to make a best effort or skip formatting a construct in the some comment scenarios.

An outcome of both of those determinations is that on the rustfmt side we can now confidently update rustfmt to not drop comments (there was some ambiguity here previously), but there's still some TBDs on if/how formatting should be done if comments are present.

For example, the presence of a short, block style comment nestled within the header that doesn't change how the item would've been formatted without said comment e.g. macro_rules! /* a */ foo { ... , is something rustfmt can easily format. However, scenarios like a mutliline block style comment or a long comment that would push the ident beyond the width are significantly more complicated and nebulous (at least from my perspective), and if we're going to attempt some "best effort" formatting in those scenarios (as opposed to just maintaining the original content as-is) then we need to consider how we'll handle each of those scenarios (with tests) before we start applying such formatting

@calebcartwright
Copy link
Member

And as a note for future discussion, when and where we do decide to make a best effort attempt at formatting in trickier comment scenarios, is that something we want to try to do consistently, if so, how consistently (e.g. consistent across the board? consistent within items,consistent within exprs, etc. but not necessarily consistent across those?)

@ytmimi
Copy link
Contributor

ytmimi commented Mar 13, 2024

Yes, I'd like to see this reverted until we've discussed our strategy on this front more holistically, because once we have rustfmt start doing something, we can't change it (at least not outside a style edition boundary).

Alright, I'll get a PR out a little later to revert this for now.

And as a note for future discussion, when and where we do decide to make a best effort attempt at formatting in trickier comment scenarios, is that something we want to try to do consistently, if so, how consistently (e.g. consistent across the board? consistent within items,consistent within exprs, etc. but not necessarily consistent across those?)

My initial reaction is to say that we should try to keep things consistent across the board, though that might be easier said than done. I'll definitely take some time to think more about this.


@fee1-dead take a look at ytmimi@3db08e1. Depending on how the comment discussion goes this might be one addition we could make to preserve the comments while still formatting whatever comes after the header.

@ytmimi ytmimi removed pr-ready-to-merge release-notes Needs an associated changelog entry labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants