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

Restore public visibility on some parsing functions for rustfmt #76115

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

calebcartwright
Copy link
Member

In #74826 the visibility of several parsing functions was reduced. However, rustfmt is an external consumer of some of these functions as well and needs the visibility to be public, similar to other elements in rustc_parse such as parse_ident

// Public for rustfmt usage.
pub fn parse_ident(&mut self) -> PResult<'a, Ident> {
self.parse_ident_common(true)
}

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2020
@matklad
Copy link
Member

matklad commented Aug 30, 2020

@bors r+ rollup

@calebcartwright is there an easy way for me to check if rustfmt builds with current rustc_parse? Under wg-parslib, we will be changing parser's API a lot in the coming months, so I wonder if there are ways to alleviate the pain here...

@bors
Copy link
Contributor

bors commented Aug 30, 2020

📌 Commit 883b1e7 has been approved by matklad

@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 Aug 30, 2020
@calebcartwright
Copy link
Member Author

@matklad

is there an easy way for me to check if rustfmt builds with current rustc_parse?

Unfortunately no, there really isn't. On rare occasions there's brief windows when it's possible to swap some of the dep references from the rustc auto publish crates to path based in-tree and vice versa, but that requires so many things to line up perfectly that it's not a really a viable approach.

It's the nature of the beast with the submod/auto-publish process, and we'd probably need to move to the subtree approach like clippy as @Manishearth has suggested to check/ensure that rustfmt and latest rustc_parse (and other rustc mods) are compatible.

Under wg-parslib, we will be changing parser's API a lot in the coming months, so I wonder if there are ways to alleviate the pain here

Thanks for the heads up, that's good to know!

Feel free to cc me if there's any questions/concerns about a rustfmt impact and I'll be happy to take a look. rustfmt uses rustc_parse, rustc_ast, and rustc_span extensively, so changes to any of their public members have a decent chance of causing breakage in rustfmt.

With the current process I don't think we'll be able to avoid that pain as the parser APIs are changed, but the earlier we're aware of those changes the more easily and quickly we'll be able to incorporate them in rustfmt. The thing we really want to avoid is running into a ton of breaking rustc changes that have to be resolved before we can address a broken rustfmt toolstate.

@Mark-Simulacrum
Copy link
Member

If it's just building (cargo check) it wouldn't be too hard to wire up rustfmt to get checked as part of x.py check, likely off by default since it's allowed to be broken.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 31, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#75938 (Added some `min_const_generics` revisions into `const_generics` tests)
 - rust-lang#76050 (Remove unused function)
 - rust-lang#76075 (datastructures: replace `once_cell` crate with an impl from std)
 - rust-lang#76115 (Restore public visibility on some parsing functions for rustfmt)
 - rust-lang#76127 (rustbuild: Remove one LLD workaround)

Failed merges:

r? @ghost
@bors bors merged commit d8eb301 into rust-lang:master Aug 31, 2020
@calebcartwright calebcartwright deleted the parser-fn-visibility branch August 31, 2020 15:34
@calebcartwright
Copy link
Member Author

If it's just building (cargo check) it wouldn't be too hard to wire up rustfmt to get checked as part of x.py check, likely off by default since it's allowed to be broken.

LMK if I misunderstood, but my interpretation of the ask was if there was something that could help determine if changes to public parsing APIs would impact rustfmt because of rustfmt's consumption of those APIs. I guess such a check could be done by swapping out all of rustfmt's rustc-ap crate deps to the corresponding in-tree mods?

That could shed some light on the impact of changes, but worth noting such a check would be in a near constant state of failure. As a reference, it would've been failing consistently for the last five weeks due to the renaming of check_name to has_name on MetaItem in rustc_ast alone, along with whatever other breaking changes in the other rustc mods consumed by rustfmt occured since our last rustc-ap bump.

@Mark-Simulacrum
Copy link
Member

Sure, yes. I think such a check would probably only be useful if we were quicker about fixing upstream rustfmt, but with the current "update RLS rustfmt racer" scheme that's, uh, probably impractical.

@matklad let me know if you think an almost-always-broken with preexisting things check would still be useful.

@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants