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

Initial start at summarizing unstable const discussion #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
- [`std::mem` and exclusive references](./code-considerations/safety-and-soundness/mem-and-exclusive-refs.md)
- [Using unstable language features](./code-considerations/using-unstable-lang/summary.md)
- [Const generics](./code-considerations/using-unstable-lang/const-generics.md)
- [Const evaluation](./code-considerations/using-unstable-lang/const-evaluation.md)
- [Specialization](./code-considerations/using-unstable-lang/specialization.md)
- [Performance](./code-considerations/performance/summary.md)
- [When to `#[inline]`](./code-considerations/performance/inline.md)
Expand Down
100 changes: 100 additions & 0 deletions src/code-considerations/using-unstable-lang/const-evaluation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
# Const evaluation

Rust's compile-time code evaluation story is still in an experimental state,
with a large amount of unstable surface area, including areas where there is
active design work.

The standard library (as of this writing) uses some amount of hacks to work
around language limitations for some implementations: this document provides a
framework for evaluating the cost/benefit analysis of using such workarounds to
enable adding new `const` functions/traits or `const`-ifying existing APIs.

The overall high-level guidance is to evaluate the tradeoff between enabling
experimentation in users of the standard library (or discovery of limitations
to these language features) and the costs to the maintenance and readability of
the standard library. (This is broad guidance applicable to almost any feature,
but const generics are particularly notable in terms of their potentially wide
scope).

These are good areas to consider in such an evaluation. You should think about
whether the given PR is adding value to outweigh its costs along these
dimensions, and add to the PR description what your justification for the usage
is. Consider including future plans for the feature as well: if the feature is
experimental, does it have a clear path toward stabilization in the current
form?

## Maintainability

Does making this API const require that code be hand-written which was previously automatic?

Does the new code require duplicating, for example through `#[cfg(bootstrap)]`,
increasing the risk of diverging between the two implementations (and
Copy link

Choose a reason for hiding this comment

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

afaik this has never happened before. tests are run on not(bootstrap) mode, and that is the only relevant mode afaict.

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 it's pretty clear that duplicating ~100+ lines of code is a recipe for editing only one of those copies. We don't always have a regression test (e.g., for performance improvements), and due to git these edits may happen entirely in parallel in terms of PRs.

Copy link

Choose a reason for hiding this comment

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

I agree, but we have never seen this happen to my knowledge.

increasing work when bumping the bootstrap compiler)?

Will updates/bugfixes to the code require further workarounds to CTFE
limitations?

Are the changes making a decision for many parts of the standard library? For
example, if a trait is made const, that likely means all implementations in the
standard library have to be const too. It may not be the case that this is easy
Comment on lines +38 to +39
Copy link

Choose a reason for hiding this comment

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

This is never and will never be true. A const trait can have non-const impls, otherwise we'd not be able to mark any traits as const.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, maybe a poor example. I suspect there are some things like this though? e.g., my understanding is that today a const impl is either all const (for every method). I'm happy to reword with help on where the limitation actually is.

(Obviously future work may remove those constraints, but this document is very much emphasizing current reality - or even cfg(bootstrap) reality).

Copy link

Choose a reason for hiding this comment

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

ah, yes, that makes a lot more sense ^^. Either all default methods on a trait are const or none.

While I disagree with the policy (we could just make them all const for now and revert the constness if it becomes a problem), I'm fine following it to save everyone headaches

to accomplish (particularly if their current implementations are complex and
need const hacks to work around limitations in CTFE).

## Stability risks

When making an internal (or external) API `const`, it becomes harder to
evaluate whether a given standard library function is safe to stabilize as
const. One of its dependencies may take a long time to stabilize for
const-evaluation, even though its API doesn't obviously reference unstable
surface area.
Comment on lines +45 to +49
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 a bit confused by this point here. If an API isn't const in the first place, then we don't need to evaluate whether it is okay to stabilize as const. If it is, then we can always split the feature up if unsure.


For example, a function like `[T]::get_many_mut` may rely on
sorting to guarantee distinct indices. If sorting is made unstably const, it is
difficult to know when looking at `get_many_mut` whether it is OK to
const-stabilize it: you have to inspect the full dependency tree with knowledge
of the "fully determined" const surface area.
Comment on lines +45 to +55
Copy link

Choose a reason for hiding this comment

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

we have logic to automate this. You cannot stabilize a const fn's constness if it calls unstable const fns unless you explicitly add #[rustc_allow_const_fn_unstable(foo, bar)]

Copy link
Member Author

Choose a reason for hiding this comment

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

Are all transitively called const fns required to be either stable or unstable by annotation? It was my impression that private helpers are sometimes const but not tagged with a const stability marker. Maybe bad memory on my part though.

Copy link
Member

@fee1-dead fee1-dead Nov 21, 2022

Choose a reason for hiding this comment

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

untagged functions are const stable by default, so you can't call unstable functions in the body unless you tag it

Copy link

Choose a reason for hiding this comment

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

hmm... I should add tests, but I'm fairly certain you can only avoid the stability marker if you're already unstable.

Copy link
Member Author

Choose a reason for hiding this comment

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

This compiles fine today (just threw this into library/core/src/lib.rs). To me that looks like it's exposing in a const-stable function functionality that might depend on const-eval-select (obviously this example doesn't but it's no guarantee).

Even if we forbid this, we'll want some language here about the thought process for adding const_stable to internal functions -- I suspect we're not very good at having rules there yet.

  const fn foo() {
      unsafe { crate::intrinsics::const_eval_select((), comp, run) }
  }

  const fn comp() {}
  fn run() {}

  #[stable(feature = "unreachable", since = "1.27.0")]
  #[rustc_const_stable(feature = "const_unreachable_unchecked", since = "1.57.0")]
  pub const fn hidden_danger() {
      foo();
  }

Copy link

@oli-obk oli-obk Nov 21, 2022

Choose a reason for hiding this comment

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

ah yes. I remember that there was some issue with not being able to add those attributes to functions that don't already have stable/unstable attributes, but that seems to have gotten fixed: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9df5e879c896456abcf21973709242b8 so we could now start checking that internal functions have const stability attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

Cc rust-lang/rust#103193 - looks like a practical example of this case, presumably there's more hiding.


This doesn't mean that we can't make internal functions const (even if
internally these use unstable const hacks to make that feasible), but it does
mean that when doing so we should be cautious. If the function is in widespread
use, it may be best to either avoid this or keep a non-const wrapper function
for most callsites, with a comment linking to this policy.

Like with specialization, we try to avoid stabilizing APIs that *rely* on
long-term unstable language details, so even if it seems like we will
definitely have some feature eventually, it may not make sense to expose it
from the standard library today.

## Unfamiliar syntax

The standard library is intended to be broadly readable to most Rust users. It
should be possible to drop into `[src]` labels from rustdoc or review PRs to
the standard library without being deeply involved with any language
experiment. Using unstable syntax like `~const Trait` which does not have
obvious meaning to someone who has finished documentation like the Rust book is
an impediment to this goal, so in general, keeping its usage to a minimum is
preferred.

Note that this is not purely about syntax; complex APIs or implementations need
justification too. This is especially the case if it makes it harder to
"uplift" an API out of the standard library into a regular crate (because of
unstable feature use or dragging internal helpers not directly related to the
feature), since that makes it challenging to experiment with improvements to it
in a more isolated setting.

Prefer to make code readable and digestible to 'everyday' Rustaceans. This is
also a win for maintainability and reviewability. The library team and the
library contributor teams need to understand this code too, and aren't up to
speed on every language initiative.

As an initial question to ask in this area, consider how one might find out
what a feature you're using in the standard library does. Is there up to date
documentation? Documentation isn't sufficient to make usage okay, but it's a
useful question to ask yourself when opening a PR making use of that feature.

## Decisions in this space

**Replacing a derive with a manual impl**: generally not permitted, see
[#102371](https://github.com/rust-lang/rust/issues/102371) for context. We are
waiting on const-derive being implemented to avoid an increase in manual
implementations throughout the standard library.
Comment on lines +97 to +100
Copy link
Member

Choose a reason for hiding this comment

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

does rust-lang/rust#102049 mean this section can be removed

2 changes: 2 additions & 0 deletions src/code-considerations/using-unstable-lang/const-generics.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ Complete const generics are currently unstable. You can track their progress [he

Const generics are ok to use in public APIs, so long as they fit in the [`min_const_generics` subset](https://github.com/rust-lang/rust/issues/74878).

See [const evaluation](./const-evaluation.md) for details on what is accepted for the *implementation* of `const` functions/contexts, which may not use const generics.

## For reviewers

Look out for const operations on const generics in public APIs like:
Expand Down
1 change: 1 addition & 0 deletions src/code-considerations/using-unstable-lang/summary.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
The standard library codebase is a great place to try unstable language features, but we have to be careful about exposing them publicly. The following is a list of unstable language features that are ok to use within the standard library itself along with any caveats:

- [Const generics](./const-generics.md)
- [Const evaluation](./const-evaluation.md)
- [Specialization](./specialization.md)
- _Something missing?_ Please submit a PR to keep this list up-to-date!

Expand Down