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.

Sign up for GitHub

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

Jump to bottom

Rewrite the Parameter Environments chapter #1953

Merged
merged 5 commits into from Apr 1, 2024
Merged

Rewrite the Parameter Environments chapter #1953

merged 5 commits into from Apr 1, 2024

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Apr 1, 2024 •
edited

The current section is confusing even to some t-types members, and also generally I have spoken to a large amount of people who did not understand how ParamEnv worked at all even after reading the current section on it.

Solution: rewrite it xd

Would quite like to find someone who currently doesnt understand ParamEnv and get them to read through this with me so I can figure out if it is actually more helpful and what problems crop up in trying to understand this ✨

Also change the location of the chapter to be before most of the chapters on analysis as an understanding of what a ParamEnv is, is pretty useful in understanding trait solving and whatnot

cc @compiler-errors

@compiler-errors
Copy link
Member

I'll definitely review this tomorrow, please annoy me if I forget!

fmease
fmease reviewed Apr 1, 2024
View reviewed changes
src/param_env.md Outdated Show resolved Hide resolved
src/param_env.md Outdated Show resolved Hide resolved
src/param_env.md Outdated Show resolved Hide resolved
src/param_env.md Outdated
Comment on lines 45 to 47
// Trying to prove `T: Clone` with a `ParamEnv` of `[T: Sized]` will
// fail as there is nothing in the environment telling the trait solver
// that `T` implements `Clone`.
Copy link
Member

@fmease fmease Apr 1, 2024 •
edited

Choose a reason for hiding this comment

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

I feel like this should clarified. If we can't find anything in the ParamEnv there may still be other candidates like user-written impls. Might sound a bit nit-picky but I think mentioning this could fortify the reader's understanding. Of course, there's no such thing as impl<T> Clone for T { … } but we might want mention that somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

no no this is perfect thank you for pointing this out. I remember vaguely thinking of this when writing but it must have slipped my mind thanks <3 I definitely dont want to give the impression that the only way of proving things is via the bounds in ParamEnv

compiler-errors
compiler-errors reviewed Apr 1, 2024
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

i was partly through leaving some reviews when fmease came thru, some of these may be not relevant anymore but im putting them up so they dont bitrot (e.g. if lcnr also reviews tomorrow while i am asleep haha)

src/param_env.md Outdated

[pe]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.ParamEnv.html
The type system relies on information in the environment in order for it to function correctly. This information is stored in the [`ParamEnv`][pe] type and it is important to use the correct `ParamEnv` when interacting with the type system.
Copy link
Member

Choose a reason for hiding this comment

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

Should we be more clear abt what an "environment" means here before diving into the details?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps though I don't actually know how to do that. I'm kind of using environment here as a handwavey vibes based thing because I can't put into words what I mean concretely 😅

src/param_env.md Outdated

For example if you have a function
The information represented by `ParamEnv` is a list of in scope where clauses, and a [`Reveal`][reveal]. A `ParamEnv` typically corresponds to a specific item's environment however it can also be created with arbitrary data that is not derived from a specific item. In most cases `ParamEnv`s are initially created via the [`param_env` query][query] which returns a `ParamEnv` derived from the provided item's where clauses.
Copy link
Member

Choose a reason for hiding this comment

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

Should we explain what a Reveal is too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the existing doc comments on Reveal do an adequate job of describing what the purpose of Reveal is. I added a note explicitly instructing people to click on the link if they want information.

src/param_env.md Outdated Show resolved Hide resolved
src/param_env.md Outdated Show resolved Hide resolved
src/param_env.md Outdated Show resolved Hide resolved
compiler-errors
compiler-errors reviewed Apr 1, 2024
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

(some other random comments when i was reading from the bottom up, will review the middle parts tomorrow 😸)

src/param_env.md Outdated Show resolved Hide resolved
src/param_env.md Outdated

When needing a `ParamEnv` in the compiler there generally three options for obtaining one:
- The correct env is already in scope simply use it (or pass it down the call stack to where you are)
- Call `tcx.param_env(def_id)`
Copy link
Member

Choose a reason for hiding this comment

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

(or param_env_reveal_all_normalized -- maybe worth mentioning and explaining why that may be desired)

Also are we forgetting ParamEnv::reveal_all()? 🤔 Useful for monomorphization usages

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated this section to talk about how to acquire ParamEnvs with Reveal::All and when this is desired.

jieyouxu
jieyouxu reviewed Apr 1, 2024
View reviewed changes
Copy link
Contributor

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

As someone who is :ferrisClueless: about typeck and ParamEnv, this looks great to me and is a very clear explanation on what ParamEnv is / what it's used for. Pointing out the pitfalls is also very 😎.

src/param_env.md Outdated Show resolved Hide resolved
src/param_env.md Outdated

[query]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ty_utils/ty/fn.param_env.html
It's very important to use the correct `ParamEnv` when interacting with the type system as otherwise it can lead to ICEs or things compiling when they shouldn't (or vice versa). See [#82159](https://github.com/rust-lang/rust/pull/82159) and [#82067](https://github.com/rust-lang/rust/pull/82067) as examples of PRs that changed rustc to use the correct param env to avoid ICE.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful pointing out what is the correct ParamEnv here. I understand it might be very difficult to come up with clear guidance, if so, ignore this comment :3

Copy link
Member Author

Choose a reason for hiding this comment

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

We do this to some degree in the 3rd subchapter which is hopefully enough. It is pretty difficult to give general guidance here yeah 😅 hopefully what we have now is adequate

src/param_env.md Outdated
`[T: Sized, T: Copy, T: Clone, T: Trait, T: SuperTrait, T: SuperSuperTrait]`
This allows us to prove `T: Clone` and `T: SuperSuperTrait` when type checking `bar`.

The `Clone` trait has a `Sized` supertrait however we do not end up with two `T: Sized` bounds in the env (one for the supertrait and one for the implicitly added `T: Sized` bound). This is because the elaboration process (implemented via [`util::elaborate`][elaborate]) deduplicates the where clauses to avoid this.
Copy link
Contributor

Choose a reason for hiding this comment

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

The links can be very helpful 👍

compiler-errors
compiler-errors approved these changes Apr 1, 2024
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

✨

@BoxyUwU BoxyUwU merged commit 84bbfc3 into rust-lang:master Apr 1, 2024
1 check passed
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Apr 1, 2024

big thanks to all of you for the reviews it was invaluable <3

@rustbot rustbot mentioned this pull request Apr 8, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 9, 2024
@matthiaskrgr
Rollup merge of rust-lang#123636 - rustbot:docs-update, r=ehuss
6146bdb 
Update books

## rust-lang/book

23 commits in 19c40bfd2d57641d962f3119a1c343355f1b3c5e..3131aa4642c627a24f523c82566b94a7d920f68c
2024-04-05 22:09:59 UTC to 2024-03-27 18:14:05 UTC

- Correct the description of Listing 5-6 (rust-lang/book#3878)
- Minor text improvements (rust-lang/book#3790)
- Update full code reference main.rs in ch20-03 (rust-lang/book#3857)
- Make Note text in ch. 20 consistent with other notes (rust-lang/book#3876)
- Consistency fix: start note sentence with a capital (rust-lang/book#3652)
- Update README.md (rust-lang/book#3656)
- Update panic! formatting style for Guess example (rust-lang/book#3767)
- Small correction in ch13-01 (rust-lang/book#3780)
- Fix a wording typo in ch07 (rust-lang/book#3694)
- Fix a typo: remove an extra 's' from ch. 18.01 (rust-lang/book#3874)
- Remove redundant words (rust-lang/book#3672)
- Fix rust-lang#3703 (rust-lang/book#3704)
- Update loop to Result: 10 (rust-lang/book#3711)
- Added "--" between run and args on ch12-03-l12 (rust-lang/book#3726)
- Add  maintenance time section in  appendix-07-nightly-rust.md  (rust-lang/book#3859)
- Update the link to the farsi translation repository (rust-lang/book#3839)
- Update README to use --locked for installing mdbook (rust-lang/book#3830)
- Reword "`union`s" to "a `union`" (rust-lang/book#3738)
- Fix two typos in 04-03 and 07-03 (rust-lang/book#3849)
- Fix typo in Chapter 7 Section 3 (rust-lang/book#3743)
- Improve ch03-05-control-flow collection looping wording (rust-lang/book#3758)
- Fix missing column separator (rust-lang/book#3855)
- Update compiler message (rust-lang/book#3856)

## rust-lang/edition-guide

3 commits in 98b33e9a441457b0a491fe1be90e7de64eafc3e5..eb3eb80e106d03250c1fb7c5666b1c8c59672862
2024-03-27 20:44:27 UTC to 2024-03-26 19:26:15 UTC

- some mdbook conveniences (rust-lang/edition-guide#297)
- typo (rust-lang/edition-guide#296)
- Clean up the `editions/index.md` page (rust-lang/edition-guide#294)

## rust-embedded/book

1 commits in 2e95fc2fd31d669947e993aa07ef10dc9828bee7..aa7d4b0b4653ddb47cb1de2036d090ec2ba9dab1
2024-04-05 07:42:54 UTC to 2024-04-05 07:42:54 UTC

- Dependencies: changed "qemu-arch-extra" to "qemu-system-arm" on arch section (rust-embedded/book#368)

## rust-lang/nomicon

2 commits in 6bc2415218d4dd0cb01433d8320f5ccf79c343a1..0d5f88475fe285affa6dbbc806e9e44d730797c0
2024-04-06 13:51:11 UTC to 2024-04-03 02:23:07 UTC

- chore: fix typo (rust-lang/nomicon#448)
- add link to reference about undefined behavior (rust-lang/nomicon#447)

## rust-lang/reference

3 commits in 984b36eca4b9293df04d5ba4eb5c4f77db0f51dc..55694913b1301cc809f9bf4a1ad1b3d6920efbd9
2024-04-03 21:31:14 UTC to 2024-04-01 19:56:13 UTC

- Add the `#[diagnostic]` attribute namespace and the `#[diagnostic::on_unimplemented]` feature to the reference (rust-lang/reference#1449)
- type-layout: be more specific about 32-bit alignments (rust-lang/reference#1393)
- Fix clippy warning in procedural macro example (rust-lang/reference#1488)

## rust-lang/rust-by-example

1 commits in 7601e0c5ad29d5bd3b518700ea63fddfff5915a7..60d34b5fd33db1346f9aabfc0c9d0bda6c8e42be
2024-04-07 13:00:53 UTC to 2024-04-07 13:00:53 UTC

- chore: fix some typos (rust-lang/rust-by-example#1833)

## rust-lang/rustc-dev-guide

11 commits in ffa246b7fd95a96e1cd54883e613aed42c32547d..b77a34bd46399687b4ce6a17198e9f316c988794
2024-04-06 20:41:09 UTC to 2024-03-27 08:49:05 UTC

- Explicitly mention compiletest directives are supported in rmake.rs (rust-lang/rustc-dev-guide#1949)
- Add docs for sharded descriptions (rust-lang/rustc-dev-guide#1959)
- Add basic docs for the new `aux-bin` header (rust-lang/rustc-dev-guide#1942)
- Add needs-threads header command (rust-lang/rustc-dev-guide#1943)
- Fix some broken links under bootstrapping. (rust-lang/rustc-dev-guide#1958)
- Replace -Zno-parallel-llvm with -Zno-parallel-backend (rust-lang/rustc-dev-guide#1957)
- Rewrite the `Parameter Environments` chapter (rust-lang/rustc-dev-guide#1953)
- Add quickstart for how to build and run the compiler (rust-lang/rustc-dev-guide#1951)
- Delete length check (rust-lang/rustc-dev-guide#1952)
- Fix some comments (rust-lang/rustc-dev-guide#1950)
- add opaque-types-region-inference-restrictions (rust-lang/rustc-dev-guide#1948)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2024
@rust-timer
Unrolled build for rust-lang#123636
1539e98 
Rollup merge of rust-lang#123636 - rustbot:docs-update, r=ehuss

Update books

## rust-lang/book

23 commits in 19c40bfd2d57641d962f3119a1c343355f1b3c5e..3131aa4642c627a24f523c82566b94a7d920f68c
2024-04-05 22:09:59 UTC to 2024-03-27 18:14:05 UTC

- Correct the description of Listing 5-6 (rust-lang/book#3878)
- Minor text improvements (rust-lang/book#3790)
- Update full code reference main.rs in ch20-03 (rust-lang/book#3857)
- Make Note text in ch. 20 consistent with other notes (rust-lang/book#3876)
- Consistency fix: start note sentence with a capital (rust-lang/book#3652)
- Update README.md (rust-lang/book#3656)
- Update panic! formatting style for Guess example (rust-lang/book#3767)
- Small correction in ch13-01 (rust-lang/book#3780)
- Fix a wording typo in ch07 (rust-lang/book#3694)
- Fix a typo: remove an extra 's' from ch. 18.01 (rust-lang/book#3874)
- Remove redundant words (rust-lang/book#3672)
- Fix rust-lang#3703 (rust-lang/book#3704)
- Update loop to Result: 10 (rust-lang/book#3711)
- Added "--" between run and args on ch12-03-l12 (rust-lang/book#3726)
- Add  maintenance time section in  appendix-07-nightly-rust.md  (rust-lang/book#3859)
- Update the link to the farsi translation repository (rust-lang/book#3839)
- Update README to use --locked for installing mdbook (rust-lang/book#3830)
- Reword "`union`s" to "a `union`" (rust-lang/book#3738)
- Fix two typos in 04-03 and 07-03 (rust-lang/book#3849)
- Fix typo in Chapter 7 Section 3 (rust-lang/book#3743)
- Improve ch03-05-control-flow collection looping wording (rust-lang/book#3758)
- Fix missing column separator (rust-lang/book#3855)
- Update compiler message (rust-lang/book#3856)

## rust-lang/edition-guide

3 commits in 98b33e9a441457b0a491fe1be90e7de64eafc3e5..eb3eb80e106d03250c1fb7c5666b1c8c59672862
2024-03-27 20:44:27 UTC to 2024-03-26 19:26:15 UTC

- some mdbook conveniences (rust-lang/edition-guide#297)
- typo (rust-lang/edition-guide#296)
- Clean up the `editions/index.md` page (rust-lang/edition-guide#294)

## rust-embedded/book

1 commits in 2e95fc2fd31d669947e993aa07ef10dc9828bee7..aa7d4b0b4653ddb47cb1de2036d090ec2ba9dab1
2024-04-05 07:42:54 UTC to 2024-04-05 07:42:54 UTC

- Dependencies: changed "qemu-arch-extra" to "qemu-system-arm" on arch section (rust-embedded/book#368)

## rust-lang/nomicon

2 commits in 6bc2415218d4dd0cb01433d8320f5ccf79c343a1..0d5f88475fe285affa6dbbc806e9e44d730797c0
2024-04-06 13:51:11 UTC to 2024-04-03 02:23:07 UTC

- chore: fix typo (rust-lang/nomicon#448)
- add link to reference about undefined behavior (rust-lang/nomicon#447)

## rust-lang/reference

3 commits in 984b36eca4b9293df04d5ba4eb5c4f77db0f51dc..55694913b1301cc809f9bf4a1ad1b3d6920efbd9
2024-04-03 21:31:14 UTC to 2024-04-01 19:56:13 UTC

- Add the `#[diagnostic]` attribute namespace and the `#[diagnostic::on_unimplemented]` feature to the reference (rust-lang/reference#1449)
- type-layout: be more specific about 32-bit alignments (rust-lang/reference#1393)
- Fix clippy warning in procedural macro example (rust-lang/reference#1488)

## rust-lang/rust-by-example

1 commits in 7601e0c5ad29d5bd3b518700ea63fddfff5915a7..60d34b5fd33db1346f9aabfc0c9d0bda6c8e42be
2024-04-07 13:00:53 UTC to 2024-04-07 13:00:53 UTC

- chore: fix some typos (rust-lang/rust-by-example#1833)

## rust-lang/rustc-dev-guide

11 commits in ffa246b7fd95a96e1cd54883e613aed42c32547d..b77a34bd46399687b4ce6a17198e9f316c988794
2024-04-06 20:41:09 UTC to 2024-03-27 08:49:05 UTC

- Explicitly mention compiletest directives are supported in rmake.rs (rust-lang/rustc-dev-guide#1949)
- Add docs for sharded descriptions (rust-lang/rustc-dev-guide#1959)
- Add basic docs for the new `aux-bin` header (rust-lang/rustc-dev-guide#1942)
- Add needs-threads header command (rust-lang/rustc-dev-guide#1943)
- Fix some broken links under bootstrapping. (rust-lang/rustc-dev-guide#1958)
- Replace -Zno-parallel-llvm with -Zno-parallel-backend (rust-lang/rustc-dev-guide#1957)
- Rewrite the `Parameter Environments` chapter (rust-lang/rustc-dev-guide#1953)
- Add quickstart for how to build and run the compiler (rust-lang/rustc-dev-guide#1951)
- Delete length check (rust-lang/rustc-dev-guide#1952)
- Fix some comments (rust-lang/rustc-dev-guide#1950)
- add opaque-types-region-inference-restrictions (rust-lang/rustc-dev-guide#1948)
surechen added a commit to surechen/rustc-dev-guide that referenced this pull request May 9, 2024
@surechen
Edit Parameter Environments's url as it has been edited in rust-lan…
96f66d9 
…g#1953
@surechen surechen mentioned this pull request May 9, 2024
Merged
fmease pushed a commit that referenced this pull request May 9, 2024
@surechen @fmease
Edit Parameter Environments's url as it has been edited in #1953
4b692c0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease left review comments

@jieyouxu jieyouxu jieyouxu left review comments

@compiler-errors compiler-errors compiler-errors approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@BoxyUwU @compiler-errors @fmease @jieyouxu