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

Preserve visibility of fn items without body #4858

Closed
wants to merge 1 commit into from

Conversation

upsuper
Copy link

@upsuper upsuper commented Jun 3, 2021

While it may not make sense in grammar to allow fn items without body to have visibility, as they are semantically only used as required methods in trait definition, I believe it is not on rustfmt to make this judgement. Such usage should be warned by clippy or rustc if desirable.

On the other hand, this may be used in macros, in which case the visibility is important, see for example #4631 and delegate-attr.

This fixes #4631.

@calebcartwright
Copy link
Member

While I personally tend to agree with you that this isn't really a formatting concern, there's historically been a philosophy of utilizing rustfmt to clean things like this up (non-macro scenarios) and regularly occurring notions of doing it for more things as syntax evolves.

As such I don't want to move ahead with this at this time, because if we do, then we'd never be able to revert back due to the formatting stability guarantee. The macro use case is insufficient justification for making the change categorically given the current formatting behavior around macro calls that's based on the user's chosen delimiters. The solution for those macro use cases is to brace delimiters when the args are invalid Rust syntax.

@upsuper
Copy link
Author

upsuper commented Nov 15, 2021

I don't see how this reasoning doesn't apply to #4961 which you merged. Is it just because that's from @dtolnay rather than a random contributor?

@calebcartwright
Copy link
Member

I don't see how this reasoning doesn't apply to #4961 which you merged. Is it just because that's from @dtolnay rather than a random contributor?

No, and let's try to avoid making any more insinuations like that.

The difference is the target/use case, where as noted in my previous comments, the driver for this PR is around args to macro calls that's part of a much broader lack of rustfmt support for macros and which has a standard, operable workaround. #4961 was not strictly related to macro calls, and had no such workaround.

@upsuper
Copy link
Author

upsuper commented Nov 15, 2021

No, this is not only about macro call specifically. It also affects procedural macros like delegate-attr, and there is no workaround as well.

@calebcartwright
Copy link
Member

Can you share a specific use case that isn't already supported with the latest version of rustfmt? The test cases you provided are already handled, and based on a cursory read of the docs for delegate-attr those snippets are covered too with visibility preserved.

@upsuper
Copy link
Author

upsuper commented Nov 18, 2021

It seems that everything this PR tries to solve has already been solved by #4961. Sorry about that.

But I hope you get that it was very frustrated that this PR was ignored for an extended time and another PR which basically did the same thing was accepted swiftly, then you claimed with a long reasoning that this PR is not going to be accepted because of the reasons that apply exactly the same way for the other PR that you accepted.

@upsuper upsuper deleted the fn-item-no-body branch November 18, 2021 11:18
@calebcartwright
Copy link
Member

But I hope you get that it was very frustrated that this PR was ignored for an extended time and another PR which basically did the same thing was accepted swiftly, then you claimed with a long reasoning that this PR is not going to be accepted because of the reasons that apply exactly the same way for the other PR that you accepted.

It's not clear to me what outcome or benefits one could hope to realize from what seems like a continuation of the theme and tone of the accusation in your previous comment. I'm responding because I view your description as a rather inaccurate characterization of events that is missing important context, however, this will likely be my last response here as I simply don't have the bandwidth to go back and forth.

For better or worse, rustfmt does generally have a heavy bias towards the status quo as I explained in my previous comment. However, we certainly can, and do, make (non-breaking) behavioral changes when needed, provided adequate context and rationalization are provided. This includes invalid syntax/grammatically irrelevant/etc. types of scenarios, with some recent examples being #4936 and #4960; both of which had associated clarity on their respective motivations.

While I now understand that delegate_attr was a significant, if not the primary, motivating factor for your PR, that was not initially clear from the passing link to the crate towards the end of your description. It would've been very helpful if that context had been provided, either as an issue explaining the use case, included in a more detailed description in the PR, or even as a follow up comment on the PR. It's great and appreciated that you found and noted an existing issue that would be impacted, but the way you described the PR made it seem overly skewed towards the macro call case in said issue and proffering of opinions on general behavior. If the PR had been framed differently/clarity of context provided like those other examples, then your PR would've been a higher priority like #4961.

You seem to be of the belief that your PR was singled out or otherwise treated differently, and while I don't understand how you arrived at that conclusion, I am sorry you feel that way. rustfmt is not a massive project, but it's a fairly good sized one that's maintained by a single volunteer (me) with whatever amount of my personal time I can carve out. That means the amount of work always significantly exceeds capacity, which in turn means that lower priority items typically get less attention/require a longer lead time to resolution.

As I noted above and previously in response to another of your comments, your PR was not very high on the priority list (due to the lack of clarity on your use case), and there's a massive backlog of issues and PRs that have been around and pending for far, far longer than yours. That spans requests/suggestions from both the community and members of the broader Rust team, and that actually includes a few from David that have been pending for years.

The author of a report/request does not dictate priority, and any suggestion otherwise is patently false.

@upsuper
Copy link
Author

upsuper commented Nov 18, 2021

Okay, thanks for the clarification.

I thought the description was clear enough what it's trying to fix, and I didn't submit a new issue because I believe they are the same underlying issue, and I don't want to submit duplicate issues to add your work, but clearly that caused the reverse.

@calebcartwright
Copy link
Member

No worries, and regardless of the path we took to get here, I'm glad this is finally sorted for you anyway!

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.

Rustfmt breaks macros due to changes
2 participants