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

Explain the new valtree system for type level constants. #1097

Merged
merged 3 commits into from
Sep 13, 2022
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 29, 2021

Dependent on getting rust-lang/rust#83234 merged first.

cc @rust-lang/wg-const-eval

@@ -20,17 +20,44 @@ Additionally constant evaluation can be used to reduce the workload or binary
size at runtime by precomputing complex operations at compiletime and only
storing the result.

All uses of constant evaluation can either be categorized as "influencing the type system"
(array lengths, enum variant discriminants, const generic parameters), or as solely being
Copy link
Contributor

@lcnr lcnr Mar 29, 2021

Choose a reason for hiding this comment

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

do discriminants influence the type system in any way?

I think ValTrees are fine here because enum variants have to be plain integers, but I don't think that this is strictly necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do discriminants influence the type system in any way?

as tags are computed from discriminants, yes, the tag is part of the representation.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, how does the tag feel into the type system? It feels like I am missing something here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... You can technically transmute an enum into a layout equivalent datastructure and then use the discriminant to change an array length. But in a less whacky (and unsound) way, if you put a non-c-like enum into a const generic, the tag will end up in the ValTree and thus in the parameters of the const generic.

Enums are encoded by adding their tag as a field before all variant fields. Alternatively we could encode enums by having an Option<VariantIdx> on ValTree::Branches. There is no strong reason to prefer either, except that enums are rare right now, so we'd be pessimizing non-enum aggregates with an unused None field.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, yeah 🤔 if we encode the discriminant in the val tree we need to do it this way. Using the VariantIdx might be cleaner in the long run, but I don't really think it matters too much.

Copy link
Member

@RalfJung RalfJung Apr 3, 2021

Choose a reason for hiding this comment

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

Maybe "affect the type system" is the wrong terminology... it's more about "does the value semantically matter to the compiler, or does the compiler just treat it as an opaque blobs (and the actual value is entirely irrelevant)". Discriminant values semantically matter -- the compiler computes on them to ensure uniqueness, determine layout niches, things like that.

Patterns, at least the subset of them that are considered for exhaustiveness checking, are another example of values that semantically matter, and that hence do/should use valtrees.

src/const-eval.md Outdated Show resolved Hide resolved
As a consequence, all decoding of `ValTree` must happen by matching on the type first and making decisions
depending on that. The value itself gives no useful information without the type that belongs to it.
One notable oddity is `&str` representation. There is no sized equivalent of it, so unlike slices we cannot
choose to represent them as their sized variant (slices are represented as arrays). `&str` thus has
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a correct implication? I assume that we do so for perf reasons, I don't expect there to be any semantic reason we can't represent &str in the same way as &[u8].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well... there is that tiny difference described in the text, and I'm using it to justify the existance of a custom variant for &str. But you are right, the main reason is performance, but it didn't actually do much difference in my tests (at best going from an 18% regression to a 17% regression), so I may revert it in the future once I have a better handle on the perf issues

src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

This looks really good! I left some more stylistic comments (can't comment much on the content since I'm learning about valtrees as I'm reading this :). Totally fine to skip some of the suggestions if you prefer the way it is now :)

src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
@jyn514 jyn514 added the S-waiting-on-author Status: this PR is waiting for additional action by the OP label Apr 27, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 27, 2021

I think @oli-obk is on break - @RalfJung would you be interested in following up on this? NBD if not, it can wait until they get back.

@RalfJung
Copy link
Member

Given that rust-lang/rust#83234 didn't land yet, I think it might be a bit early to take this one over.

@jyn514 jyn514 added S-blocked Status: this PR is blocked waiting for something and removed S-waiting-on-author Status: this PR is waiting for additional action by the OP labels Apr 27, 2021
@tshepang
Copy link
Member

tshepang commented Aug 9, 2022

closing as the PR it documents is also closed... feel free to re-open when that PR is revived

@tshepang tshepang closed this Aug 9, 2022
@lcnr
Copy link
Contributor

lcnr commented Aug 10, 2022

valtrees have been merged in rust-lang/rust#96591

going to check if this document is still up to date

@lcnr lcnr reopened this Aug 10, 2022
src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: this PR is waiting for additional action by the OP and removed S-blocked Status: this PR is blocked waiting for something labels Aug 10, 2022
@JohnTitor
Copy link
Member

@oli-obk Friendly-ping, could you resolve merge conflicts and address the above comments? Once these are done, I think we could merge this!

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 13, 2022

done

src/const-eval.md Outdated Show resolved Hide resolved
src/const-eval.md Outdated Show resolved Hide resolved
@lcnr lcnr merged commit f1609a3 into master Sep 13, 2022
@JohnTitor JohnTitor deleted the valtree branch September 13, 2022 12:48
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 21, 2022
Update books

## nomicon

1 commits in d880e6ac2acf133dce640da24b9fb692844f02d4..f53bfa056929217870a5d2df1366d2e7ba35096d
2022-08-24 12:42:34 -0700 to 2022-09-05 07:19:02 -0700
- Small typo (rust-lang/nomicon#379)

## reference

9 commits in f62e93c28323ed9637d0a205a0c256498674a509..a7cdac33ca7356ad49d5c2b5e2c5010889b33eee
2022-08-28 10:01:28 -0700 to 2022-09-19 17:39:58 -0700
- Clarify wording for references. (rust-lang/reference#1223)
- Update Unicode reference to match rustc implementation (rust-lang/reference#1271)
- Add documentation for raw-dylib and link_ordinal (rust-lang/reference#1244)
- Specify guarantees for repr(rust) structs (rust-lang/reference#1152)
- Classify AsyncBlockExpression as ExpressionWithoutBlock (rust-lang/reference#1268)
- Update closure-expr.md (rust-lang/reference#1269)
- Clarify that 0 is a valid multiple of a type's alignment (rust-lang/reference#1260)
- Remove `ne` from derive example (rust-lang/reference#1264)
- Clarify reference on async blocks (rust-lang/reference#1262)

## book

6 commits in 0a5421ceb238357b3634fb75234eba4d1dad643c..f1e5ad844d0c61738006cdef26227beeb136948e
2022-08-28 19:51:04 -0400 to 2022-09-19 09:48:21 -0400
- Fix punctuation in ch05-02
- Ownership move chapter link fix
- Wrong listing number
- Reword text around box
- `Box&lt;T&gt;` instead of "box"
- Update Clippy output in Appendix D

## rust-by-example

2 commits in 03301f8ae55fa6f20f7ea152a517598e6db2cdb7..767a6bd9727a596d7cfdbaeee475e65b2670ea3a
2022-08-14 08:51:44 -0300 to 2022-09-14 09:17:18 -0300
- struct_visibility.md:  Remove unneeded '#[allow(dead_code)]' (rust-lang/rust-by-example#1609)
- Fix assorted typos (rust-lang/rust-by-example#1601)

## rustc-dev-guide

15 commits in 04892c1a6fc145602ac7367945fda9d4ee83c9fb..f587d6e7cddeaa3cf0a33ec1e368df1a408fa0aa
2022-08-29 20:07:51 +0200 to 2022-09-20 07:43:59 +0900
- Update stability guide to use CURRENT_RUSTC_VERSION (rust-lang/rustc-dev-guide#1468)
- Add a note about building `rust-analyzer-proc-macro-srv` (rust-lang/rustc-dev-guide#1467)
- Link from "implementing to new features" to mcp.md (rust-lang/rustc-dev-guide#1465)
- remove stray **
- Explain the new valtree system for type level constants. (rust-lang/rustc-dev-guide#1097)
- fix typos and formatting
- Say "bootstrap" instead of "rustbuild"; the latter is not explained anywhere and is not much more clear.
- Rewrite the section on passing flags to subcommands
- Remove the diagram of all outputs generated by x.py
- "symbol names" =&gt; ABI
- Add symbol-addition to the how-to for new features (rust-lang/rustc-dev-guide#1457)
- Fix typo (rust-lang/rustc-dev-guide#1459)
- Document multipart_suggestion derive on SessionSubdiagnostic
- Add reference for updating Windows PATH and fix typo
- Update for removal of RLS (rust-lang/rustc-dev-guide#1450)

## embedded-book

1 commits in befe6840874311635c417cf731377f07234ee373..4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954
2022-07-25 07:51:14 +0000 to 2022-09-15 08:53:09 +0000
- Create CITATION.bib (as per rust-embedded/book#327)  (rust-embedded/book#329)
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Sep 22, 2022
Update books

## nomicon

1 commits in d880e6ac2acf133dce640da24b9fb692844f02d4..f53bfa056929217870a5d2df1366d2e7ba35096d
2022-08-24 12:42:34 -0700 to 2022-09-05 07:19:02 -0700
- Small typo (rust-lang/nomicon#379)

## reference

9 commits in f62e93c28323ed9637d0a205a0c256498674a509..a7cdac33ca7356ad49d5c2b5e2c5010889b33eee
2022-08-28 10:01:28 -0700 to 2022-09-19 17:39:58 -0700
- Clarify wording for references. (rust-lang/reference#1223)
- Update Unicode reference to match rustc implementation (rust-lang/reference#1271)
- Add documentation for raw-dylib and link_ordinal (rust-lang/reference#1244)
- Specify guarantees for repr(rust) structs (rust-lang/reference#1152)
- Classify AsyncBlockExpression as ExpressionWithoutBlock (rust-lang/reference#1268)
- Update closure-expr.md (rust-lang/reference#1269)
- Clarify that 0 is a valid multiple of a type's alignment (rust-lang/reference#1260)
- Remove `ne` from derive example (rust-lang/reference#1264)
- Clarify reference on async blocks (rust-lang/reference#1262)

## book

6 commits in 0a5421ceb238357b3634fb75234eba4d1dad643c..f1e5ad844d0c61738006cdef26227beeb136948e
2022-08-28 19:51:04 -0400 to 2022-09-19 09:48:21 -0400
- Fix punctuation in ch05-02
- Ownership move chapter link fix
- Wrong listing number
- Reword text around box
- `Box&lt;T&gt;` instead of "box"
- Update Clippy output in Appendix D

## rust-by-example

2 commits in 03301f8ae55fa6f20f7ea152a517598e6db2cdb7..767a6bd9727a596d7cfdbaeee475e65b2670ea3a
2022-08-14 08:51:44 -0300 to 2022-09-14 09:17:18 -0300
- struct_visibility.md:  Remove unneeded '#[allow(dead_code)]' (rust-lang/rust-by-example#1609)
- Fix assorted typos (rust-lang/rust-by-example#1601)

## rustc-dev-guide

15 commits in 04892c1a6fc145602ac7367945fda9d4ee83c9fb..f587d6e7cddeaa3cf0a33ec1e368df1a408fa0aa
2022-08-29 20:07:51 +0200 to 2022-09-20 07:43:59 +0900
- Update stability guide to use CURRENT_RUSTC_VERSION (rust-lang/rustc-dev-guide#1468)
- Add a note about building `rust-analyzer-proc-macro-srv` (rust-lang/rustc-dev-guide#1467)
- Link from "implementing to new features" to mcp.md (rust-lang/rustc-dev-guide#1465)
- remove stray **
- Explain the new valtree system for type level constants. (rust-lang/rustc-dev-guide#1097)
- fix typos and formatting
- Say "bootstrap" instead of "rustbuild"; the latter is not explained anywhere and is not much more clear.
- Rewrite the section on passing flags to subcommands
- Remove the diagram of all outputs generated by x.py
- "symbol names" =&gt; ABI
- Add symbol-addition to the how-to for new features (rust-lang/rustc-dev-guide#1457)
- Fix typo (rust-lang/rustc-dev-guide#1459)
- Document multipart_suggestion derive on SessionSubdiagnostic
- Add reference for updating Windows PATH and fix typo
- Update for removal of RLS (rust-lang/rustc-dev-guide#1450)

## embedded-book

1 commits in befe6840874311635c417cf731377f07234ee373..4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954
2022-07-25 07:51:14 +0000 to 2022-09-15 08:53:09 +0000
- Create CITATION.bib (as per rust-embedded/book#327)  (rust-embedded/book#329)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: this PR is waiting for additional action by the OP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants