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

Improve bounds.rs #248

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

DoubleHyphen
Copy link

  1. Implemented all FIXMEs:
    → Trait now contains associated constants rather than non-const functions.
    LowerBounded and UpperBounded are now supertraits of Bounded, not subtraits.
  2. Expanded implementations to also cover NonZero data-types.
  3. Made the code DRYer, with regards to the implementation of bounded_impl!.
  4. Added a couple tests.

Ιωάννης Νικολαΐδης added 2 commits September 25, 2022 20:27
    → Trait now contains associated constants rather than non-`const` functions.
    → `LowerBounded` and `UpperBounded` are now supertraits of `Bounded`, not subtraits.
2) Expanded implementations to also cover `NonZero` data-types.
3) Made the code DRYer, with regards to the implementation of `bounded_impl!`.
4) Added a couple tests.
@DoubleHyphen DoubleHyphen marked this pull request as draft September 25, 2022 18:27
@DoubleHyphen
Copy link
Author

To clarify, as the code is currently written all tests pass. But I think something's amiss.

I just tried a toy program that used the new Bounded trait. So, I tried using the MAX and MIN consts within a function that takes a Bounded parametre, only to realise that the compiler considered <T as LowerBounded>::MIN to be something completely different than <T as Bounded>::MIN, and kept asking for clarification. This is of course unreasonably non-ergonomic, so I deleted the MAX and MIN implementations in the Bounded trait itself… and the test suite lost its mind.

The closest I've come to making some semblance of sense out of this is that, apparently, there are several macros in the test suite (eg assert_eq_from) that use Bounded as a trait object. Apparently, when Bounded has separate MIN and MAX members it's all fine, but as soon as they're removed, all of a sudden it's unacceptable to use it as a trait object because… wait for it… it has associated consts?

I ultimately managed to get it to work by 1) adding use num_traits::bounds::{LowerBounded, UpperBounded}; to the beginning and 2) changing the trait objects to <_ as LowerBounded>::MIN etc. However, I can't say definitively if that fixes the actual issue, because there's probably a delicate dance happening here between visibility modifiers. As such, I'd like to politely ask for the guidance of a more senior person here.

@cuviper
Copy link
Member

cuviper commented Sep 28, 2022

This is of course unreasonably non-ergonomic, so I deleted the MAX and MIN implementations in the Bounded trait itself…

Agreed so far...

and the test suite lost its mind.

The closest I've come to making some semblance of sense out of this is that, apparently, there are several macros in the test suite (eg assert_eq_from) that use Bounded as a trait object.

This is bizarre. I would not expect Bounded::MAX to be a trait object at all, just a partially qualified path, on its way to being inferred as a fully qualified <Something as Bounded>::MAX. It's even Sized!

Even in the same file, test wrapping_bounded complains that it can't find MIN/MAX. I don't know why it's not looking into the supertraits there.

@SunnyWar
Copy link

SunnyWar commented May 9, 2023

I want this so bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants