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

Document the policy for subtree changes #104962

Closed
jyn514 opened this issue Nov 26, 2022 · 9 comments
Closed

Document the policy for subtree changes #104962

jyn514 opened this issue Nov 26, 2022 · 9 comments
Assignees

Comments

@jyn514
Copy link
Member

jyn514 commented Nov 26, 2022

Any of the following seem like good places:

  • (ideally) the ping message that shows up when the subtree is modified
  • the dev-guide
  • the README for your tool

cc @calebcartwright @matklad. For context, Ralf is ok with feature changes being made in-tree and flip is not - it's ok for the policy to vary per-tool but then we need to document it.

Originally posted by @jyn514 in #103660 (comment)

@calebcartwright
Copy link
Member

Thanks for the heads up @jyn514. rustfmt will be in the same boat it sounds like clippy is in, in that we do not want such changes for rustfmt being made in-tree. Will incorporate such in the docs accordingly

@RalfJung
Copy link
Member

RalfJung commented Nov 27, 2022

To be clear, for Miri the policy is that if feature changes can be done purely in the Miri repo, that is generally preferred. It is just fairly common that feature changes to the interpreter core span both repos and then doing them in the rustc repo is the only option.

#103660 was such a change that spanned rustc_driver and Miri. I guess this one could have been split in two since it didn't break API, but then we would have had no idea whether it actually works...

@matklad
Copy link
Member

matklad commented Nov 27, 2022

cc @Veykril

@RalfJung
Copy link
Member

So, I am honestly not entirely sure what to write where here. Our README is for Miri users, not contributors. We have a CONTRIBUTING guide but that is already almost entirely focused on working with out-of-tree Miri; I will clarify the role of the in-tree copy in rust-lang/miri#2698. The ping message seems fine, it will summon a Miri person into the PR who will take a look whether that change looks good to us. So far we had 0 cases of someone submitting a Miri-only change to rustc so I think extending the message has just as much risk of confusing people than clarifying anything.

What went wrong in #103660 is that we didn't get a review from a clippy person. From the Miri side, everything was fine.

@flip1995
Copy link
Member

From the Clippy side the policy is that functional changes always have to be done in Clippy, except there is a compelling reason, like rustc changes are required. But rustc changes are rarely required for Clippy changes.

As Ralf, I'm also not quite sure where to document this, where people would read it.

What went wrong in #103660 is that we didn't get a review from a clippy person.

Yeah, I currently don't get to look at the Rust PRs where the Clippy team gets pinged because of Clippy changes. I just don't have enough time right now. This all worked great when I could take a quick look at every PR.


This policy is also already documented in the rustc-dev-guide

However, enhancements, bug fixes, etc. specific to these tools should be filed against the tools directly in their respective upstream repositories.

I think in #103660 it wasn't clear cut if this is a rustc API change fallout fix or a functional change.

@calebcartwright
Copy link
Member

This policy is also already documented in the rustc-dev-guide

However, enhancements, bug fixes, etc. specific to these tools should be filed against the tools directly in their respective upstream repositories.

True, although I think when I added that in 292072d8e26b8e26c5a048939ff21d3707e869c7 clippy and rustfmt were the only subtrees and the text reflected the policy for those two. Now that there are other subtrees with divergent policies I think we probably need to relax this language and either enumerate the policies inline or link out to the respective policies if defined in the corresponding tool repos

@RalfJung
Copy link
Member

RalfJung commented Nov 28, 2022

The only real difference for Miri is that it is more common that a Miri change requires a rustc change. So I think the underlying policy is basically the same for both tools. I am proposing a clarification in rust-lang/rustc-dev-guide#1518 which I hope the clippy devs also agree with.

I think in #103660 it wasn't clear cut if this is a rustc API change fallout fix or a functional change.

Yeah. It was new optional rustc API added for the benefit of tools. Tools could have kept using the old API but then we'd have no user of the new API, making it an open question whether the PR actually works. And the entire goal of this was to share that code between clippy and Miri, so porting only Miri would not have showed success either.

@jyn514
Copy link
Member Author

jyn514 commented Nov 30, 2022

Ok. I'm going to close this since #103660 was a special case where it was a functional change to tools that could not be made out-of-tree. @flip1995 sorry it landed without your review, I'll make sure you get a chance to review similar PRs in the future before they merge.

@jyn514 jyn514 closed this as completed Nov 30, 2022
@flip1995
Copy link
Member

No worries! I had looking at that PR on my TODO list for a few days before it was merged. So it's also my bad for not getting back to PRs like this in time.

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

No branches or pull requests

5 participants