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

SemVer: Add section on RPIT capturing #14849

Merged
merged 2 commits into from
Dec 8, 2024

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 22, 2024

This adds a section on RPIT capturing, added in Rust 1.82, with a note about how this changes in the 2024 edition.

@rustbot
Copy link
Collaborator

rustbot commented Nov 22, 2024

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2024
@weihanglo
Copy link
Member

Not sure why.

---- freshness::rebuild_tests_if_lib_changes stdout ----
running `/home/runner/work/cargo/cargo/target/debug/cargo build`
running `/home/runner/work/cargo/cargo/target/debug/cargo test`
running `/home/runner/work/cargo/cargo/target/debug/cargo build -v`
running `/home/runner/work/cargo/cargo/target/debug/cargo test -v`
thread 'freshness::rebuild_tests_if_lib_changes' panicked at tests/testsuite/freshness.rs:633:10:

---- expected: tests/testsuite/freshness.rs:623:27
++++ actual:   stderr
   1    1 | [DIRTY] foo v0.0.1 ([ROOT]/foo): the dependency foo was rebuilt ([TIME_DIFF_AFTER_LAST_BUILD])
   2    2 | [COMPILING] foo v0.0.1 ([ROOT]/foo)
   3    3 | [RUNNING] `rustc --crate-name foo [..]
   4      - [RUNNING] `rustc --crate-name foo [..]
   5    4 | error[E0425]: cannot find function `foo` in crate `foo`
        5 +  --> tests/foo.rs:4:34
   6    6 | ...
   7    7 | [ERROR] could not compile `foo` (test "foo") due to 1 previous error
   8    8 | ...∅

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks great!

cc @obi1kenobi though I've already noticed obi1kenobi/cargo-semver-checks#815.

Copy link
Member

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Very nice! I look forward to being able to cite this new section in cargo-semver-checks lints :)

Thanks for the tag @weihanglo!


See the [edition guide][rpit-capture-guide] and the [reference][rpit-reference] for more information on RPIT capturing.

It is a minor change to capture fewer generic parameters in an RPIT.
Copy link
Member

@obi1kenobi obi1kenobi Nov 22, 2024

Choose a reason for hiding this comment

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

I believe this might not be true for functions in non-sealed traits. If the trait's function definition changes the RPIT captures, I expect implementations of the trait have to be updated to match.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm… sounds very likely. Could you give an example of it?

Copy link
Member

Choose a reason for hiding this comment

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

Ah it seems use syntax in RPIT in traits isn't supported yet, so I can't give you a playground example.

But here's proof from a related thread: rust-lang/rust#132795

It shows this example:

trait TypeParam<T> {
    fn test() -> impl Sized;
}
// Indirectly capturing a lifetime param through a type param substitution.
impl<'a> TypeParam<&'a ()> for i32 {
    fn test() -> impl Sized + use<> {}
    //~^ WARN impl trait in impl method captures fewer lifetimes than in trait
}

and states that the impl is a valid refinement of the trait.

But if it's a refinement, that means not all implementations of the trait have to adhere to it — it's a stricter interface than what the trait requires.

Therefore, applying this to our question at hand about removing generics from use<>: If the trait were edited to go from (implicitly) fn test() -> impl Sized + use<T> to fn test() -> impl Sized + use<>, that would be a breaking change. The impls that had applied the refinement would continue to work, and impls that didn't refine in this way would be broken.

Copy link
Member

@obi1kenobi obi1kenobi Dec 6, 2024

Choose a reason for hiding this comment

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

So to summarize, the statement is accurate as written so long as use<> in RPITIT isn't supported. But it sounds like that limitation is of a temporary "work in progress" nature, so it's probably a good idea to make the semver reference a bit more future-proof.

I think the accurate statement here is "major change in RPIT in traits unless the trait is sealed, minor change otherwise."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite following here. Since RPITIT is not stabilized, I would prefer to not refer to it or document its behavior. When it is stabilized, we can update the documentation to say that it is a major change to change which generics are captured in RPITIT unless the trait is sealed. (Depending on if the design of RPITIT changes.) Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry. My statement was not intended for inclusion in this document. It was merely intended as a summary of my understanding of Rust's present and plausible future rules here.

If you're sure we can remember to follow up on this section when use<> in RPITIT is stabilized, then I'm fine with either wording here. Otherwise, it may be wise to consider slightly more defensive wording here that explicitly excludes RPIT in traits.

I just wanted to raise this for your consideration — it's your call to make and I'm happy with whatever you think is best.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how to make sure to remember updating the doc, but at least now we have three people aware of it. I think it's good to move forward.

Thanks folks for the discussion!
(I have more understanding for generics capturing after this 😆)

let iter = updated_crate::f(&a, &b);
drop(b); // Error: cannot move out of `b` because it is borrowed
}
```
Copy link
Member

Choose a reason for hiding this comment

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

A thought: would it be worth adding an example, or even just mentioning in a sentence or two that use<> syntax can also be used to capture type parameters, not just lifetimes? That wasn't immediately obvious to me when I first heard about this new language feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I added a note here. I don't think type and const generics have any impact here because you cannot change them. I generally would like to avoid using this space for teaching how the language works, and that's why there are links to the reference and edition guide which do elaborate on how it works.


See the [edition guide][rpit-capture-guide] and the [reference][rpit-reference] for more information on RPIT capturing.

It is a minor change to capture fewer generic parameters in an RPIT.
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how to make sure to remember updating the doc, but at least now we have three people aware of it. I think it's good to move forward.

Thanks folks for the discussion!
(I have more understanding for generics capturing after this 😆)

@weihanglo weihanglo added this pull request to the merge queue Dec 8, 2024
Merged via the queue into rust-lang:master with commit bf79c8b Dec 8, 2024
20 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
Update cargo

18 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..7847c03965260b5dcc8d93218d6af295a717abb6
2024-12-06 21:56:56 +0000 to 2024-12-13 18:06:39 +0000
- fix(base): Support bases in patches in virtual manifests  (rust-lang/cargo#14931)
- fix(resolver): Report invalid index entries  (rust-lang/cargo#14927)
- feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928)
- fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923)
- fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926)
- fix(script): Don't override the release profile (rust-lang/cargo#14925)
- feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917)
- fix(resolver): Don't report all versions as rejected  (rust-lang/cargo#14921)
- fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897)
- fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911)
- a faster hash for ActivationsKey (rust-lang/cargo#14915)
- feat(build-script): Pass CARGO_CFG_FEATURE  (rust-lang/cargo#14902)
- fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913)
- chore: update auto-label to include build-rs crate (rust-lang/cargo#14912)
- refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908)
- feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910)
- fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899)
- SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2024
Update cargo

19 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..769f622e12db0001431d8ae36d1093fb8727c5d9
2024-12-06 21:56:56 +0000 to 2024-12-14 04:27:35 +0000
- test(build-std): dont require rustup (rust-lang/cargo#14933)
- fix(base): Support bases in patches in virtual manifests  (rust-lang/cargo#14931)
- fix(resolver): Report invalid index entries  (rust-lang/cargo#14927)
- feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928)
- fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923)
- fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926)
- fix(script): Don't override the release profile (rust-lang/cargo#14925)
- feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917)
- fix(resolver): Don't report all versions as rejected  (rust-lang/cargo#14921)
- fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897)
- fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911)
- a faster hash for ActivationsKey (rust-lang/cargo#14915)
- feat(build-script): Pass CARGO_CFG_FEATURE  (rust-lang/cargo#14902)
- fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913)
- chore: update auto-label to include build-rs crate (rust-lang/cargo#14912)
- refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908)
- feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910)
- fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899)
- SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)
@rustbot rustbot added this to the 1.85.0 milestone Dec 14, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 15, 2024
Update cargo

19 commits in 20a443231846b81c7b909691ec3f15eb173f2b18..769f622e12db0001431d8ae36d1093fb8727c5d9
2024-12-06 21:56:56 +0000 to 2024-12-14 04:27:35 +0000
- test(build-std): dont require rustup (rust-lang/cargo#14933)
- fix(base): Support bases in patches in virtual manifests  (rust-lang/cargo#14931)
- fix(resolver): Report invalid index entries  (rust-lang/cargo#14927)
- feat: Implement `--depth workspace` for `cargo tree` command (rust-lang/cargo#14928)
- fix(resolver): In errors, show rejected versions over alt versions (rust-lang/cargo#14923)
- fix: emit_serialized_unit_graph uses the configured shell (rust-lang/cargo#14926)
- fix(script): Don't override the release profile (rust-lang/cargo#14925)
- feature(SourceId): use stable hash from rustc-stable-hash (rust-lang/cargo#14917)
- fix(resolver): Don't report all versions as rejected  (rust-lang/cargo#14921)
- fix(resolver): Report unmatched versions, rather than saying no package (rust-lang/cargo#14897)
- fix(build-rs): Implicitly report rerun-if-env-changed for input (rust-lang/cargo#14911)
- a faster hash for ActivationsKey (rust-lang/cargo#14915)
- feat(build-script): Pass CARGO_CFG_FEATURE  (rust-lang/cargo#14902)
- fix(build-rs): Correctly refer to the item in assert (rust-lang/cargo#14913)
- chore: update auto-label to include build-rs crate (rust-lang/cargo#14912)
- refactor: use Path::push to construct remap-path-prefix (rust-lang/cargo#14908)
- feat(build-rs): Add the 'error' directive (rust-lang/cargo#14910)
- fix(build-std): determine root crates by target spec `std:bool` (rust-lang/cargo#14899)
- SemVer: Add section on RPIT capturing (rust-lang/cargo#14849)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants