-
Notifications
You must be signed in to change notification settings - Fork 879
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
Allow rustfmt to abort when using nightly only features on stable channel #5025
base: master
Are you sure you want to change the base?
Allow rustfmt to abort when using nightly only features on stable channel #5025
Conversation
1ad1e9a
to
c6181b0
Compare
Thank you for the PR! You've raised some good questions that I need to think on a bit. An interesting wrinkle to the testing is that we actually only run our own CI/local builds on nightly, so we'd either need some trickery for the unit tests, or would otherwise just have to drop them and rely on human testing/validation. The integration test story might be "easier" in the sense that we could compile on nightly and then use the stable version of cargo fmt and the associated env var to utilize the built version of rustfmt instead of the bundled one. As far as the behavior goes, as long as all the unstable/unavailable options are still printed (as opposed to just the first one and then bailing), then I think that should be good |
Thinking about the implementation a bit more and glancing over the diff, I've a few thoughts:
|
Also agree that it would be a lot better to have this baked into the rustfmt library. I think I initially implemented it the way that I did because I didn't want to make changes to function calls / signatures, but if you're alright with it then I'll update the implementation! Would it make sense for
currently, I think that we're only tracking unstable options in a configuration file. Could you point me to where we parse the command line overrides.
I think this is a great idea. I was hoping you could give an example to go off of. |
Understood. Fortunately now that we're no longer publishing rustfmt to crates.io we don't need to be constrained by semver compatibility on the public API of the library. There's a handful of folks that are still attempting to consume as a library directly from source, but we've made it clear to them that the API is subject to change without notice. Feel free to suggest any changes that you think would be beneficial 👍
Feel free to give it a try
Ah that's right, and there's been some past debate about whether the ability to use unstable opts on stable via the cli override is a feature or a bug. IIRC the returned
This one is a bit more complicated than what we need on the config side, but here's the main error kind def we return from the main entry point Lines 99 to 143 in 2c442cc
Note that we don't necessarily have to use an error, but I think we'll want some kind of extension to the return type either way that callers (including the rustfmt bin) can use |
c6181b0
to
be5e68d
Compare
I've been doing a little more work on this PR to implement the custom error return type for The main issue is that
1) Unstable options are passed from the command line and we don't use
|
Just to clarify my original line of thinking, I don't think As a general philosophy I think libraries should be cautious about writing directly to stdout/stderr buffers, especially in a manner that callers/consumers can't control. This starts to become more concretely relevant for us as we've had requests for error presentation to be incorporated into certain emitters vs. dumped to std{out,err} (e.g. if a user has selected the I'm a bit surprised by the scenario you reported in number 1 because I thought the CLI overrides were set with a dedicated function that's on an entirely different call path than the standard config loading, and accordingly I was thinking it'd be possible to handle config file vs. cli overrides differently. Perhaps I'm remembering incorrectly though rustfmt/src/config/config_type.rs Lines 224 to 238 in 4389a4c
Regardless, and to clarify a bit more, I'm not terribly concerned if we can't display all unstable opts from both config files and command line overrides. I still have a sense that we can, but the scenario I was really trying to avoid was with your example config file: error_on_line_overflow = true
error_on_unformatted = true
version = "Two" rustfmt only emitting a complaint about I realize that may not have addressed each individual item from your last comment, but does it help? |
8bafc8a
to
312c454
Compare
I see what you're saying. The current implementation still prints a warning to I've made sure to include the unstable options in the new
You're right about the command line options being set using In scenario 1) above, this causes I guess one scenario I didn't explore was if the Lines 290 to 336 in 4389a4c
Got it! In that case I think the current implementation achieves what you had in mind.
Helps out a lot, thanks! I think we're both on the same page with everything. I think the implementation achieves everything you were looking for, but if not please let me know so I can make adjustments.
Now that #5109 #5112 have been merged into master we have a mechanism to test nightly only and stable only code, and we're now testing both stable and nightly in CI. I'll update the code to use the new testing attribute macros. |
I might've confused myself with the labels 😆 Remind me, is this just waiting on me to take another pass or were there still some other changes you had in mind? |
There are still a few minor tweaks that I'd like to make, like using the There might be other little changes too, but it's been a little while so I need to review the PR myself to know for sure 😅 |
312c454
to
43e6ac9
Compare
@calebcartwright Okay, now I think I'm ready for a re-review (assuming all the CI that's running right now passes successfully!). The PR is on the larger side, so no rush on this. I added end-to-end tests to Definitely let me know if we still need to make changes here. The last thing that I want to make sure about is that the new configuration option has the most descriptive name we can give it. Do you think that |
43e6ac9
to
2245fdf
Compare
Apologies in advance for what may seem like me moving the goal posts, but I'm actually now thinking that we may want to perform the check elsewhere. Main points driving my rationale:
Accordingly, what if we instead performed the "are we on stable, were there any unstable options, and if both then what's the expected behavior" logic check within https://github.com/rust-lang/rustfmt/blob/master/src/formatting.rs#L31-L40 I think a new
I always strive to avoid double negatives as those tend to be far too easy for folks to mix up or otherwise cause confusion, so would suggest we invert with something like "format with unusable, unstable options specified on stable" that's defaulted to Thoughts? |
2245fdf
to
3b4c02f
Compare
I like all your suggestions. I moved the exit early / warning logic to I still need to:
Let me know if there's anything else you'd like me to do on this that I'm missing. |
This option was proposed in issue 5022 and allows rustfmt to exit early in the event that any unstable options are set in the projects rustfmt.toml
3b4c02f
to
6f4a598
Compare
Resolves #5022
Summary
At a high level, this feature was implemented by adding a new configuration option that toggles whether we abort rustfmt or log warnings when using unstable options on the stable channel. If we're on the stable channel and unstable features have been used, we decide how to move forward based on the set configuration option.
At a lower level this PR was implemented by making the following changes:
abort_on_unrecognised_options
. This was the name suggested in the original issue, but I don't mind changing it.false
and the option was marked as unstable. Because it's unstable we needed to special case this option when determining if a user used an unstable option.Vec
UnstableOptions
struct toConfig
to track unstable options used when on stable rustfmtfill_from_parsed_config
abort_or_warn_on_unstable_options
was implemented for Config which returns a new enumHandleUnstableOptions
.A macro named.abort_or_warn_on_unstable_options!
was added to bring all the implementation details together in an ergonomic wayload_config
now returns aResult<(Config, Option<PathBuf>) ,LoadConfigurationError>
Testing
Tests were added for all the helper methods. Some tests are only intended to run on nightly, and the rest of the tests are expected to run on stable.
I wasn't quite sure how to write the full integration test, so I'd appreciate some guidance on that as well as any other feedback on the PR.
I think the following integration tests are still needed:
abort_on_unrecognised_options = true
while using unstable optionsabort_on_unrecognised_options = true
while not using unstable optionsabort_on_unrecognised_options = false
while using unstable optionsabort_on_unrecognised_options = false
while not using unstable options5-8) Run all combinations of test listed above but use nightly instead of stable.
abort_on_unrecognised_options
shouldn't have any effect on nightlyAdditional Thoughts
Also, something that I wondered about but didn't implement was what happens when you set the new option to true on nightly? Currently nothing happens, but should we print out a warning to let users know that the option won't have any effect?