Allow const iterator implementations #102225

fee1-dead · 2022-09-24T06:27:07Z

rust-highfive · 2022-09-24T06:27:10Z

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot · 2022-09-24T06:27:11Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

fee1-dead · 2022-09-24T06:30:47Z

cc @rust-lang/wg-const-eval

library/core/src/iter/traits/iterator.rs

bors · 2022-09-24T18:01:18Z

☔ The latest upstream changes (presumably #102234) made this pull request unmergeable. Please resolve the merge conflicts.

thomcc · 2022-09-25T00:12:43Z

I could review the impl changes (so feel free to reassign to me if you'd prefer), but I think I'm not the best here. I know @scottmcm has done considerable work on the iterator implementations, and is also t-lang, so they seem like a more natural fit for this.

r? @scottmcm

bors · 2022-09-25T17:32:58Z

☔ The latest upstream changes (presumably #102265) made this pull request unmergeable. Please resolve the merge conflicts.

onestacked · 2022-09-25T20:25:55Z

Once this lands I can take on const Iterator for Ranges, Slices and maybe some other types if you want.

oli-obk

The drop in place change could go into a separate PR, seems good on its own

On the rest of the PR: quite a few annoyingly big bootstrap diffs, not sure how I feel about them. But I also don't want to block this for a month

oli-obk · 2022-09-24T10:34:28Z

library/core/src/array/mod.rs

+                    // SAFETY: slice contains initialized objects
+                    unsafe { crate::ptr::drop_in_place(x) }
+                }
+                crate::intrinsics::const_eval_select((to_drop,), drop_ct, drop_rt);


This will be needed more often, maybe move into drop_in_place

I can extract this into a separate function, but not to drop_in_place as drop_in_place is for all unsized types and this is only for slices.

scottmcm · 2022-09-26T08:16:47Z

Hmm, this is really dependent on the change from 2 days ago, I take it? My instinct was also that it would be nice to wait for the bootstrap bump, but if it's 6 weeks that's not amazing.

Certainly a bunch of this -- like constifying the adaptor constructors -- is an easy "sure", though.

onestacked · 2022-09-26T21:22:15Z

Once this lands I can take on const Iterator for Ranges, Slices and maybe some other types if you want.

Im not sure its actually possible to do const Iterator for slices, since those rely on many pointer operations that are not sound in const context.

pub struct Iter<'a, T: 'a> {
    ptr: NonNull<T>,
    end: *const T, // If T is a ZST, this is actually ptr+len.  This encoding is picked so that
    // ptr == end is a quick test for the Iterator being empty, that works
    // for both ZST and non-ZST.
    _marker: PhantomData<&'a T>,
}

library/core/src/slice/iter/macros.rs: has the is_empty and len macros, other then those I have implemented const Iterator for slices.

fee1-dead · 2022-09-27T04:09:23Z

Certainly a bunch of this -- like constifying the adaptor constructors -- is an easy "sure", though.

We recently moved to attributes on the entire trait instead of marking individual functions as const. As a result of this, we can't simply constify adaptors separately..

the8472 · 2022-09-27T20:55:21Z

Could this maybe be reduced in scope by only making next const so iterators work in for-each loops?

onestacked · 2022-09-27T22:53:15Z

Could this maybe be reduced in scope by only making next const so iterators work in for-each loops?

No as @fee1-dead said above there was a recent change that disabled partially const Traits. (only allow const implementations for traits with the #[const_trait] attribute)
So it is no longer possible to only implement const next().

Partially const Traits were removed, since a ~const Trait bound would not actually mean that the concrete type used actually has the const functions needed.

thomcc · 2022-09-27T22:57:03Z

This seems unfortunate, since it also means that we're effectively saying that t-libs-api is only allowed to make const iterator methods (or else Rust users can't use iterators in const).

That might be reasonable, but it's a big restriction that seems worth considering.

onestacked · 2022-09-28T09:49:03Z

Keyword generics might allow syntax like this to allow non const functions in traits:

const<C> Trait {
    const<C> fn const_fn() { ...}
    fn non_const_fn() {...}
}

But otherwise I don't know how to make some trait functions never const. (Other than adding some kind of #[never_const] attribute which would be very weird in my opinion)

the8472 · 2022-09-28T09:56:53Z

This seems unfortunate, since it also means that we're effectively saying that t-libs-api is only allowed to make const iterator methods (or else Rust users can't use iterators in const).

That should make this a libs-api concern

fee1-dead · 2022-11-15T11:52:42Z

Re-requesting review, since this now uses derive_const. Partially const traits have serious design concerns, and I don't see how we will ever need a method body that cannot be const, and even if we might need it we can still just leave it unimplemented using const_eval_select. So I don't think having a language feature to allow partially const traits would be the biggest blocker right now.

the8472 · 2022-11-16T18:21:32Z

Partially const traits have serious design concerns

Even if we don't have stable partially const traits enabling that unstably would still bring the benefit of reducing the amount of API surface we need to constify, which is one of the issues with this type of "experimenting with const syntax" PR.

and I don't see how we will ever need a method body that cannot be const

If core and std get unified then it could easily do something that does IO or allocate. Or it could involve concrete types that aren't const.

and even if we might need it we can still just leave it unimplemented using const_eval_select.

It's be quite the hack to explicitly declare the whole thing is const and then go "oops, you can't actually call this in const contexts". That's exactly the time where you'd need a partially const trait to make this explicit.

the8472 · 2022-11-16T18:29:48Z

library/core/src/iter/traits/iterator.rs

+        const fn collect<
+            'a,
+            T: ~const Iterator,
+            R,
+            O: ~const FromIterator<<<T as Iterator>::Item as Try>::Output>,
+        >(
+            x: GenericShunt<'a, T, R>,
+        ) -> O
+        where
+            T::Item: ~const Try<Residual = R>,
+            R: ~const Destruct,
+        {
+            x.collect()
+        }


this looks terrible, can the generic constraints be moved to where bounds?

the8472 · 2022-11-16T18:36:10Z

library/core/src/iter/traits/collect.rs

-            move |(), (t, u)| {
-                a.extend_one(t);
-                b.extend_one(u);
+        pub struct ExtendFold<'a, EA, EB>(&'a mut EA, &'a mut EB);


Is this a temporary thing to be removed later? If so it could use a FIXME(#issuenumber) for the feature needed to solve it.

If this isn't meant to be temporary I have additional concerns.

Yes, this is temporary. This could be improved and made to look better using ConstFnMutClosure, but in the long run we plan to replace these with const closures.

I've improved it.

library/core/src/iter/adapters/zip.rs

library/core/src/iter/adapters/mod.rs

library/core/src/array/mod.rs

the8472 · 2022-11-16T19:04:24Z

There's some ongoing work for namable function items. Would it be possible to have conditionally-const Iterator::next impls and then make const for _ in iter depend on where I: Iterator, I::fn::next: ~const or something like that?

fee1-dead · 2022-11-17T12:37:14Z

Partially const traits have serious design concerns

Even if we don't have stable partially const traits enabling that unstably would still bring the benefit of reducing the amount of API surface we need to constify, which is one of the issues with this type of "experimenting with const syntax" PR.

The serious design concerns make the described language feature perma-unstable if they cannot be resolved.

It's be quite the hack to explicitly declare the whole thing is const and then go "oops, you can't actually call this in const contexts". That's exactly the time where you'd need a partially const trait to make this explicit.

We are not panicking at runtime, though? This would only result in compile errors when called. I would consider adding a perma-unstable language feature for this to be a hack as well.

There's some ongoing work for namable function items. Would it be possible to have conditionally-const Iterator::next impls and then make const for _ in iter depend on where I: Iterator, I::fn::next: ~const or something like that?

I guess. It will certainly need more compiler work in addition to namable function item types to make this happen. I was hoping that we could make comparisons between two slices possible in compile time. Its current default implementation uses iterator combinators. (see)

The thing is that all of these are doable and look way better than the one I did a year ago (#92433) It is a real concern in terms of readability since not many people understand what ~const is right now, and those are valid points. Perhaps you would want const_closure to be implemented (after ~1yr or more of implementation work) first, such that readability of these closures are not impacted at all?

rust-log-analyzer · 2022-11-23T01:21:39Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling memchr v2.5.0
   Compiling std v0.0.0 (/checkout/library/std)
   Compiling compiler_builtins v0.1.84
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error[E0277]: the trait bound `alignment::Alignment: ~const cmp::Eq` is not satisfied
    |
174 | impl const cmp::Ord for Alignment {
174 | impl const cmp::Ord for Alignment {
    |            ^^^^^^^^ the trait `~const cmp::Eq` is not implemented for `alignment::Alignment`
    |
note: the trait `cmp::Eq` is implemented for `alignment::Alignment`, but that implementation is not `const`
    |
174 | impl const cmp::Ord for Alignment {
    |            ^^^^^^^^
note: required by a bound in `cmp::Ord`
note: required by a bound in `cmp::Ord`
   --> library/core/src/cmp.rs:767:16
    |
767 | pub trait Ord: ~const Eq + ~const PartialOrd<Self> {
    |                ^^^^^^^^^ required by this bound in `cmp::Ord`
For more information about this error, try `rustc --explain E0277`.
error: could not compile `core` due to previous error
Build completed unsuccessfully in 0:02:04

bors · 2022-12-10T08:51:10Z

☔ The latest upstream changes (presumably #105512) made this pull request unmergeable. Please resolve the merge conflicts.

apiraino · 2023-01-05T14:12:10Z

I'll tentatively remove T-compiler from the review queue as the code in this patch seems now to be all under ./library/core. Let me know if it's ok!

@rustbot label -T-compiler

…o, r=thomcc implement const iterator using `rustc_do_not_const_check` Previous experiment: rust-lang#102225. Explanation: rather than making all default methods work under `const` all at once, this uses `rustc_do_not_const_check` as a workaround to "trick" the compiler to not run any checks on those other default methods. Any const implementations are only required to implement the `next` method. Any actual calls to the trait methods other than `next` will either error in compile time (at CTFE runs), or run the methods correctly if they do not have any non-const operations. This is extremely easy to maintain, remove, or improve.

…r=the8472,fee1-dead Allow using `Range` as an `Iterator` in const contexts. ~~based on rust-lang#102225 by `@fee1-dead~~`

rust-highfive assigned thomcc Sep 24, 2022

rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 24, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2022

the8472 reviewed Sep 24, 2022

View reviewed changes

library/core/src/iter/traits/iterator.rs Show resolved Hide resolved

rust-highfive assigned scottmcm and unassigned thomcc Sep 25, 2022

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the const_iterator branch from 98284d6 to 3e1fbda Compare September 25, 2022 13:15

onestacked mentioned this pull request Sep 25, 2022

[WIP] Constified slice::sort_unstable, sort_internals #102279

Closed

7 tasks

oli-obk requested changes Sep 26, 2022

View reviewed changes

fee1-dead force-pushed the const_iterator branch 2 times, most recently from efd3979 to 8240186 Compare September 27, 2022 14:34

the8472 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 28, 2022

fee1-dead force-pushed the const_iterator branch from 606e59a to fc49fe2 Compare November 14, 2022 13:04

fee1-dead marked this pull request as ready for review November 14, 2022 13:51

fee1-dead requested a review from scottmcm November 16, 2022 12:21

onestacked mentioned this pull request Nov 16, 2022

Allow using Slice Iterators in const Contexts #104491

Closed

the8472 reviewed Nov 16, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

fee1-dead force-pushed the const_iterator branch from fc49fe2 to 27bd174 Compare November 20, 2022 13:18

This comment has been minimized.

Sign in to view

fee1-dead added 3 commits November 20, 2022 13:25

Allow const iterator implementations

9e7656c

Add libcore test

7d2c911

rebased

8249c6c

fee1-dead force-pushed the const_iterator branch from 27bd174 to 8249c6c Compare November 20, 2022 13:25

Simplified iterator constructor functions

070a4e3

fee1-dead mentioned this pull request Dec 3, 2022

Tracking Issue for const Iterator impls and related const fns #92476

Open

3 tasks

This comment was marked as off-topic.

Sign in to view

rustbot removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 5, 2023

fee1-dead mentioned this pull request Jan 13, 2023

implement const iterator using rustc_do_not_const_check #106541

Merged

jofas mentioned this pull request Jan 29, 2023

Feature: nested struct shall be considered too jofas/struct_field_names_as_array#4

Open

fee1-dead closed this Feb 25, 2023

jaudiger mentioned this pull request May 24, 2024

Calling non-const function core::slice::iter::<impl IntoIterator for &[u8]>::into_iter #125490

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow const iterator implementations #102225

Allow const iterator implementations #102225

fee1-dead commented Sep 24, 2022 •

edited

Loading

rust-highfive commented Sep 24, 2022

rustbot commented Sep 24, 2022

fee1-dead commented Sep 24, 2022

bors commented Sep 24, 2022

thomcc commented Sep 25, 2022 •

edited

Loading

This comment has been minimized.

bors commented Sep 25, 2022

onestacked commented Sep 25, 2022 •

edited

Loading

oli-obk left a comment •

edited

Loading

oli-obk Sep 24, 2022

fee1-dead Sep 27, 2022

scottmcm commented Sep 26, 2022

onestacked commented Sep 26, 2022

fee1-dead commented Sep 27, 2022

the8472 commented Sep 27, 2022

onestacked commented Sep 27, 2022

thomcc commented Sep 27, 2022

onestacked commented Sep 28, 2022

the8472 commented Sep 28, 2022

fee1-dead commented Nov 15, 2022 •

edited

Loading

the8472 commented Nov 16, 2022

the8472 Nov 16, 2022

fee1-dead Nov 20, 2022

the8472 Nov 16, 2022

fee1-dead Nov 17, 2022

fee1-dead Nov 20, 2022

the8472 commented Nov 16, 2022

fee1-dead commented Nov 17, 2022 •

edited

Loading

This comment was marked as resolved.

This comment has been minimized.

rust-log-analyzer commented Nov 23, 2022

bors commented Dec 10, 2022

This comment was marked as off-topic.

apiraino commented Jan 5, 2023

Allow const iterator implementations #102225

Allow const iterator implementations #102225

Conversation

fee1-dead commented Sep 24, 2022 • edited Loading

rust-highfive commented Sep 24, 2022

rustbot commented Sep 24, 2022

fee1-dead commented Sep 24, 2022

bors commented Sep 24, 2022

thomcc commented Sep 25, 2022 • edited Loading

This comment has been minimized.

bors commented Sep 25, 2022

onestacked commented Sep 25, 2022 • edited Loading

oli-obk left a comment • edited Loading

Choose a reason for hiding this comment

oli-obk Sep 24, 2022

Choose a reason for hiding this comment

fee1-dead Sep 27, 2022

Choose a reason for hiding this comment

scottmcm commented Sep 26, 2022

onestacked commented Sep 26, 2022

fee1-dead commented Sep 27, 2022

the8472 commented Sep 27, 2022

onestacked commented Sep 27, 2022

thomcc commented Sep 27, 2022

onestacked commented Sep 28, 2022

the8472 commented Sep 28, 2022

fee1-dead commented Nov 15, 2022 • edited Loading

the8472 commented Nov 16, 2022

the8472 Nov 16, 2022

Choose a reason for hiding this comment

fee1-dead Nov 20, 2022

Choose a reason for hiding this comment

the8472 Nov 16, 2022

Choose a reason for hiding this comment

fee1-dead Nov 17, 2022

Choose a reason for hiding this comment

fee1-dead Nov 20, 2022

Choose a reason for hiding this comment

the8472 commented Nov 16, 2022

fee1-dead commented Nov 17, 2022 • edited Loading

This comment was marked as resolved.

This comment has been minimized.

rust-log-analyzer commented Nov 23, 2022

bors commented Dec 10, 2022

This comment was marked as off-topic.

apiraino commented Jan 5, 2023

fee1-dead commented Sep 24, 2022 •

edited

Loading

thomcc commented Sep 25, 2022 •

edited

Loading

onestacked commented Sep 25, 2022 •

edited

Loading

oli-obk left a comment •

edited

Loading

fee1-dead commented Nov 15, 2022 •

edited

Loading

fee1-dead commented Nov 17, 2022 •

edited

Loading