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 on trait items #4961

Merged
merged 2 commits into from
Aug 22, 2021

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Aug 20, 2021

Fixes #4960.

@upsuper
Copy link

upsuper commented Aug 21, 2021

I have a similar fix in #4858 which has been waiting for review for two months 😞

@calebcartwright
Copy link
Member

I have a similar fix in #4858 which has been waiting for review for two months disappointed

As noted in #4960 (comment) these are indeed different issues, which is why #4858 wouldn't actually address #4960.

@upsuper I appreciate your willingness to submit a PR but would ask that you keep a few things in mind:

  • These are separate issues and resolutions, so please keep the discussions separate in their respective threads
  • Chronology of PRs does not strictly dictate the order in which they are considered, though worth noting there's quite a few that have been pending longer than yours anyway. Factors like impact/severity, risk, and presence/absence of workarounds have a much higher level of influence on the backlog priority

@dtolnay - changes are good but want to double check the history, style guide,etc. to make sure there's not any other changes or pertinent historical context I'm missing

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thank you for the report and the fix.

The Style Guide doesn't have any constraints that would conflict and need to be updated, and best I can tell this behavior wasn't introduced as part of any specific request to drop the visibility. As such it seems the most likely origin is simply that rustfmt drops them because they aren't permitted in the absence of the attribute on the parent item.

Technically this fix will result in subsequent versions of rustfmt producing a different result than the preceding ones for the same given input. However, I do not view that as a violation of the formatting stability guarantee, and definitely don't think it would be worth the effort nor the increased code complexity to have a more exotic solution that would conditionally maintain the visibility based on the attributes of the parent item.

The responsibility for removing non-permitted visibilities on associated items should fall on the developer or rustfix, not rustfmt as the style/formatting tool.

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 loses visibility on some trait items and impl items
3 participants