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

Impl Default for ranges #73197

Merged
merged 2 commits into from
Jul 21, 2020
Merged

Impl Default for ranges #73197

merged 2 commits into from
Jul 21, 2020

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Jun 10, 2020

Couldn't find an issue about it.

Range and friends probably can implement Default if Idx: Default. For example, the following would be possible:

#[derive(Default)]
struct Foo(core::ops::RangeToInclusive<u64>);

let _ = [1, 2, 3].get(core::ops::Range::default());

core::ops::RangeFrom::<u8>::default().take(20).for_each(|x| { dbg!(x); });

fn stuff<T: Default>() { let instance = T::default(); ... more stuff }
stuff::<core::ops::RangeTo<f32>>();

Maybe there are some concerns about safety or misunderstandings?

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 10, 2020
@clarfonthey
Copy link
Contributor

Although empty makes sense, how could we be sure that starting and ending at zero is the best choice?

@taiki-e
Copy link
Member

taiki-e commented Jun 10, 2020

In this PR's implementation, it seems that the defaults of Range/RangeTo/RangeInclusive/RangeInclusiveTo are "empty", the defaults of RangeInclusive/RangeInclusiveTo are "the first element" and the defaults of RangeFrom/RangeFull are "all elements".

I'm not sure if it makes sense to implement default on ranges that have bounds only at either the start or end.

EDIT: fixed wrong comment, defaults of RangeInclusive/RangeInclusiveTo is actually "the first element".

EDIT2: this is only for unsigned integer (#73197 (comment))

@jonas-schievink jonas-schievink added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 10, 2020
@jonas-schievink jonas-schievink added this to the 1.45 milestone Jun 10, 2020
@c410-f3r
Copy link
Contributor Author

Not sure if it is possible to have optioned implementations because the implications and semantics will vary depending on the Idx type or the selected structure, in other words, the user is aware of what he is doing. Which is different from the implicit consequences of #27186.

@Elinvynia Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2020
@Dylan-DPC-zz
Copy link

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Jun 26, 2020

I don't think Default really makes sense for ranges. Is there a specific use case that this would enable?

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jun 26, 2020

There are four listed use-cases and I personally have a trait Foo that expects Foo: Default and one of its implementations should be core::ops::Range, effectively forcing a custom struct YetAnotherStructToEncapsulateYetAnotherCoreType(core::ops::Range<f64>); wrapper.

IMHO, these implementations have no side-effects.

@Mark-Simulacrum
Copy link
Member

The listed use cases don't suggest obvious defaults (at least to me). Do you have a case that's less cut down (i.e., a real world example)? I don't think empty ranges are obviously "default" to me.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jun 26, 2020

Implementation

A default instance is, in my option, an independent constant value and if the listed examples are starting and ending as zero, it is because their Idx types are also zero (<u64 as Default>::default() == 0). Not sure if it is something that can be modified in a generalized way, let's suppose that "empty" is not a good default, then what the default should be like if Idx is not known? Manual hard-coded implementation using declarative macro or a type bound constraint isn't a good alternative because it ceases user freedom. For example:

// For whatever reason, <Range<u32> as Default>::default() = 0..u32::MAX is a good default
impl Default for Range<u32> { fn default() -> Self { 0..u32::MAX } }

// Problem: There isn't a Range implementation for arbitrary types
#[derive(Default)]
struct FancyDecimalType;
#[derive(Default)]
struct UserTypeWithRange(Range<FancyDecimalType>); // error!

// Because: Generic implementation will conflict with <Range<u32> as Default>
impl<T: Default> Default for Range<T> { fn default() -> Self { Default::default()..Default::default() } } // error!
// Bound constraints suffer the same problem, suppose an imaginary std Number trait that has `min()` and `max()`
impl<T: Number> Default for Range<T> { fn default() -> Self { T::min()..T::max() } }

// As a consequence, it also won't be possible to have a generic implementation for Range<T>

As previously said, a generic impl<T: Default> for Range*<T> { ... } gives freedom and the user should be aware of what kind of instance value Range(From|To|ToInclusive) is going to have based on T: Default.

Use-case

If all given explanations weren't enough, then I will try to explain a hopefully "real-world" use-case: I have a project that solves nondeterministic polynomial problems which is basically composed of trait Solution, trait Domain<S> and trait Criteria. Most problems have variables represented as vectors of continuous numbers ([1.0, 2.0, 3.0]) with their respective contiguous domains represented as vectors of continuous ranges ([0.0..10.0, 0.0..5.0, 1.0..3.0]), therefore:

// Just a simplification. The real code were made using declarative macros and num
impl Solution for [f64; 1] { ... }
impl Domain<[f64; 1]> for [core::ops::Range<f64>; 1] { ... }

All posterior operations don't known what Solution, Domain or Criteria are because they are abstracted away in a Problem<S: Solution, D: Domain, C: Criteria> { ... } structure and it is important to have a default dummy instance to evaluate a solver's performance/quality or as an intermediate state between the conversion of different constrained, unconstrained, single or multi-objectives problems.
With a expected Problem< ... >: Default and without Range<T>: Default, every single problem definition that uses ranges as domains will have to create a cumbersome and duplicated wrapper:

pub struct RangeWrapperBecauseOfDefaultDomain<T>(pub core::ops::Range<T>);

impl<T> Default for RangeWrapperBecauseOfDefaultDomain<T>
where
  T: Default
{
  fn default() -> Self {
    Self(T::default()..T::default())
}

This effect can be minimized by other means but like the subtitle says, it is just an additional use-case that hopefully can be interpreted as a more "real-world" example.

@Amanieu
Copy link
Member

Amanieu commented Jun 27, 2020

I can see the reasoning behind this and tentatively agree that we should add Default.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jun 27, 2020

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 27, 2020
@Dylan-DPC-zz Dylan-DPC-zz modified the milestones: 1.45, 1.46 Jun 30, 2020
@scottmcm
Copy link
Member

scottmcm commented Jul 6, 2020

The fact that 0..0 and 0..=0 are so different makes me somewhat skeptical here. Even in the domain example, it doesn't seem obvious that the Range<T>: Default would be useful -- defaulting to an empty domain (0..0) doesn't seem like what would be wanted, nor would defaulting to a singleton domain (0..=0).

I'll note that, for example, std::ops::Bound doesn't impl Default either.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I am skeptical of some of these too. I am definitely on board with the impl for RangeFull. I am also on board with an impl for Range, which does the equivalent of default()..default(). I think I would prefer to leave out the rest of the impls: RangeFrom, RangeTo, RangeInclusive, RangeToInclusive.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 6, 2020

core::ops::Bound can be 3 different things and a default value wouldn't make sense for every scenario, thus the reason why #[derive(Default)] can't be applied to enums. This PR only touches concrete ranges structures that clearly express what they were made for.

As previously said, "empty" ranges are just a consequence of a generic impl<T: Default> Default for Range(From|To|ToInclusive)<T> that specific hand-picked implementations would break due to conflicting resolution.

Regarding on what is a "good" default, I can say IMHO that it will vary from person to person.

@dtolnay
Copy link
Member

dtolnay commented Jul 10, 2020

@rfcbot concern let's not do for RangeFrom, RangeTo, RangeInclusive, RangeToInclusive

Per #73197 (review).

@SimonSapin
Copy link
Contributor

In this PR's implementation, it seems that the defaults of Range/RangeTo/RangeInclusive/RangeInclusiveTo are "empty", the defaults of RangeInclusive/RangeInclusiveTo are "the first element" and the defaults of RangeFrom/RangeFull are "all elements".

Only for unsigned integer

@dtolnay
Copy link
Member

dtolnay commented Jul 10, 2020

@rfcbot resolve let's not do for RangeFrom, RangeTo, RangeInclusive, RangeToInclusive
@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jul 10, 2020
@rfcbot
Copy link

rfcbot commented Jul 10, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 10, 2020
@Dylan-DPC-zz Dylan-DPC-zz modified the milestones: 1.46, 1.47 Jul 18, 2020
@avl
Copy link

avl commented Jul 19, 2020

Since Vec defaults to an empty Vec, isn't it only logical that Range would default to en empty Range?

I personally think it is often convenient to have default implemented for as many types as possible. For instance, it allows resize on a Vec. Default is needed even if the vector is only ever shrunk in practice.

@scottmcm
Copy link
Member

Since Vec defaults to an empty Vec, isn't it only logical that Range would default to en empty Range?

Let say it is: then wouldn't it also be "only logical" that the other kinds of RangeBounds would also default to an empty range? Which of course can't happen: It's straight impossible for RangeFull, RangeFrom, RangeTo, and RangeToInclusive. And doing it for RangeInclusive either means a new trait (because Default::default()..=Default::default() is typically non-empty) or forcing forever the "there's an extra flag to ignore the fields" implementation of RangeInclusive.

And the other kind of RangeBounds in std is (Bound, Bound), but @c410-f3r just argued earlier that Bound shouldn't be Default, which means that that pair can't be Default either.

For instance, it allows resize on a Vec. Default is needed even if the vector is only ever shrunk in practice.

resize doesn't use Default; nor does resize_with. resize_default does, obviously, but that's currently deprecated. Not to mention that there's always just truncate for shrinking...

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 20, 2020
@rfcbot
Copy link

rfcbot commented Jul 20, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@dtolnay
Copy link
Member

dtolnay commented Jul 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 20, 2020

📌 Commit c375692 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2020
@avl
Copy link

avl commented Jul 20, 2020

Since Vec defaults to an empty Vec, isn't it only logical that Range would default to en empty Range?

Let say it is: then wouldn't it also be "only logical" that the other kinds of RangeBounds would also default to an empty range? Which of course can't happen: It's straight impossible for RangeFull, RangeFrom, RangeTo, and RangeToInclusive. And doing it for RangeInclusive either means a new trait (because Default::default()..=Default::default() is typically non-empty) or forcing forever the "there's an extra flag to ignore the fields" implementation of RangeInclusive.

All good points! I think I overstated things when I said "only logical". I think it's not "obvious" that the Range types should implement Default. But I still think it would be useful, and not that much more surprising than Vec or u32 implementing Default.

resize doesn't use Default; nor does resize_with. resize_default does, obviously, but that's currently deprecated. Not to mention that there's always just truncate for shrinking...

Ah, I didn't know about truncate!

My use case was I had a program which processed lots of data in parallel in many threads. It could use a thread pool of N threads. The program had a Vec<T> of length N for scratch data. Each worker thread needed to use some memory for its processing, and this scratch data was stored in the Vec. If the number of worker threads was changed, the Vec needed to be resized.

I wanted to automatically resize the Vec to the correct number, and required T:Default so there was value to use for any new elements. If type T anywhere contained a Range, it would be inconvenient because it couldn't as easily implement Default in that case. Reading what I wrote I realize that my argument isn't super-convincing, but I at least wished that Default was implemented for all types where it can be argued that one value is more default than any other value. Such as the u32 0, the empty Vec, and conceivably, the range 0..0 .

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72714 (Fix debug assertion in typeck)
 - rust-lang#73197 (Impl Default for ranges)
 - rust-lang#73323 (wf: check foreign fn decls for well-formedness)
 - rust-lang#74051 (disallow non-static lifetimes in const generics)
 - rust-lang#74376 (test caching opt_const_param_of on disc)
 - rust-lang#74501 (Ayu theme: Use different background color for Run button)
 - rust-lang#74505 (Fix search input focus in ayu theme)
 - rust-lang#74522 (Update sanitizer docs)
 - rust-lang#74546 (Fix duplicate maybe_uninit_extra attribute)
 - rust-lang#74552 (Stabilize TAU constant.)
 - rust-lang#74555 (Improve "important traits" popup display on mobile)
 - rust-lang#74557 (Fix an ICE on an invalid `binding @ ...` in a tuple struct pattern)
 - rust-lang#74561 (update backtrace-rs)

Failed merges:

r? @ghost
@bors bors merged commit 241374a into rust-lang:master Jul 21, 2020
@c410-f3r c410-f3r deleted the ranges branch July 21, 2020 01:25
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 13, 2020
Pkgsrc changes:
 * Remove patches now integrated upstream, many related to SunOS / Illumos.
 * The LLVM fix for powerpc is also now integrated upstream.
 * Adapt those patches where the source has moved or parts are integrated.
 * The randomness patches no longer applies, and I could not find
   where those files went...
 * Provide a separate bootstrap for NetBSD/powerpc 9.0, since apparently
   the C++ ABI is different from 8.0.  Yes, this appears to be specific to
   the NetBSD powerpc ports.

Upstream changes:

Version 1.47.0 (2020-10-08)
==========================

Language
--------
- [Closures will now warn when not used.][74869]

Compiler
--------
- [Stabilized the `-C control-flow-guard` codegen option][73893], which enables
  [Control Flow Guard][1.47.0-cfg] for Windows platforms, and is ignored on
  other platforms.
- [Upgraded to LLVM 11.][73526]
- [Added tier 3\* support for the `thumbv4t-none-eabi` target.][74419]
- [Upgrade the FreeBSD toolchain to version 11.4][75204]
- [`RUST_BACKTRACE`'s output is now more compact.][75048]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`CStr` now implements `Index<RangeFrom<usize>>`.][74021]
- [Traits in `std`/`core` are now implemented for arrays of any length, not just
  those of length less than 33.][74060]
- [`ops::RangeFull` and `ops::Range` now implement Default.][73197]
- [`panic::Location` now implements `Copy`, `Clone`, `Eq`, `Hash`, `Ord`,
  `PartialEq`, and `PartialOrd`.][73583]

Stabilized APIs
---------------
- [`Ident::new_raw`]
- [`Range::is_empty`]
- [`RangeInclusive::is_empty`]
- [`Result::as_deref`]
- [`Result::as_deref_mut`]
- [`Vec::leak`]
- [`pointer::offset_from`]
- [`f32::TAU`]
- [`f64::TAU`]

The following previously stable APIs have now been made const.

- [The `new` method for all `NonZero` integers.][73858]
- [The `checked_add`,`checked_sub`,`checked_mul`,`checked_neg`, `checked_shl`,
  `checked_shr`, `saturating_add`, `saturating_sub`, and `saturating_mul`
  methods for all integers.][73858]
- [The `checked_abs`, `saturating_abs`, `saturating_neg`, and `signum`  for all
  signed integers.][73858]
- [The `is_ascii_alphabetic`, `is_ascii_uppercase`, `is_ascii_lowercase`,
  `is_ascii_alphanumeric`, `is_ascii_digit`, `is_ascii_hexdigit`,
  `is_ascii_punctuation`, `is_ascii_graphic`, `is_ascii_whitespace`, and
  `is_ascii_control` methods for `char` and `u8`.][73858]

Cargo
-----
- [`build-dependencies` are now built with opt-level 0 by default.][cargo/8500]
  You can override this by setting the following in your `Cargo.toml`.
  ```toml
  [profile.release.build-override]
  opt-level = 3
  ```
- [`cargo-help` will now display man pages for commands rather just the
  `--help` text.][cargo/8456]
- [`cargo-metadata` now emits a `test` field indicating if a target has
  tests enabled.][cargo/8478]
- [`workspace.default-members` now respects `workspace.exclude`.][cargo/8485]
- [`cargo-publish` will now use an alternative registry by default if it's the
  only registry specified in `package.publish`.][cargo/8571]

Misc
----
- [Added a help button beside Rustdoc's searchbar that explains rustdoc's
  type based search.][75366]
- [Added the Ayu theme to rustdoc.][71237]

Compatibility Notes
-------------------
- [Bumped the minimum supported Emscripten version to 1.39.20.][75716]
- [Fixed a regression parsing `{} && false` in tail expressions.][74650]
- [Added changes to how proc-macros are expanded in `macro_rules!` that should
  help to preserve more span information.][73084] These changes may cause
  compiliation errors if your macro was unhygenic or didn't correctly handle
  `Delimiter::None`.
- [Moved support for the CloudABI target to tier 3.][75568]
- [`linux-gnu` targets now require minimum kernel 2.6.32 and glibc 2.11.][74163]

Internal Only
--------
- [Improved default settings for bootstrapping in `x.py`.][73964]
  You can read details about this change in the ["Changes to `x.py`
  defaults"](https://blog.rust-lang.org/inside-rust/2020/08/30/changes-to-x-py-defaults.html)
  post on the Inside Rust blog.

- [Added the `rustc-docs` component.][75560] This allows you to install
  and read the documentation for the compiler internal APIs. (Currently only
  available for `x86_64-unknown-linux-gnu`.)

[1.47.0-cfg]: https://docs.microsoft.com/en-us/windows/win32/secbp/control-flow-guard
[76980]: rust-lang/rust#76980
[75048]: rust-lang/rust#75048
[74163]: rust-lang/rust#74163
[71237]: rust-lang/rust#71237
[74869]: rust-lang/rust#74869
[73858]: rust-lang/rust#73858
[75716]: rust-lang/rust#75716
[75908]: rust-lang/rust#75908
[75516]: rust-lang/rust#75516
[75560]: rust-lang/rust#75560
[75568]: rust-lang/rust#75568
[75366]: rust-lang/rust#75366
[75204]: rust-lang/rust#75204
[74650]: rust-lang/rust#74650
[74419]: rust-lang/rust#74419
[73964]: rust-lang/rust#73964
[74021]: rust-lang/rust#74021
[74060]: rust-lang/rust#74060
[73893]: rust-lang/rust#73893
[73526]: rust-lang/rust#73526
[73583]: rust-lang/rust#73583
[73084]: rust-lang/rust#73084
[73197]: rust-lang/rust#73197
[72488]: rust-lang/rust#72488
[cargo/8456]: rust-lang/cargo#8456
[cargo/8478]: rust-lang/cargo#8478
[cargo/8485]: rust-lang/cargo#8485
[cargo/8500]: rust-lang/cargo#8500
[cargo/8571]: rust-lang/cargo#8571
[`Ident::new_raw`]:  https://doc.rust-lang.org/nightly/proc_macro/struct.Ident.html#method.new_raw
[`Range::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.Range.html#method.is_empty
[`RangeInclusive::is_empty`]: https://doc.rust-lang.org/nightly/std/ops/struct.RangeInclusive.html#method.is_empty
[`Result::as_deref_mut`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref_mut
[`Result::as_deref`]: https://doc.rust-lang.org/nightly/std/result/enum.Result.html#method.as_deref
[`TypeId::of`]: https://doc.rust-lang.org/nightly/std/any/struct.TypeId.html#method.of
[`Vec::leak`]: https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#method.leak
[`f32::TAU`]: https://doc.rust-lang.org/nightly/std/f32/consts/constant.TAU.html
[`f64::TAU`]: https://doc.rust-lang.org/nightly/std/f64/consts/constant.TAU.html
[`pointer::offset_from`]: https://doc.rust-lang.org/nightly/std/primitive.pointer.html#method.offset_from
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet