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

Consider making ::new to be const fn #35

Open
greyblake opened this issue Apr 29, 2023 · 13 comments
Open

Consider making ::new to be const fn #35

greyblake opened this issue Apr 29, 2023 · 13 comments
Labels
Specs required The issue requires analysis and detailed specification

Comments

@greyblake
Copy link
Owner

No description provided.

@greyblake greyblake added the Specs required The issue requires analysis and detailed specification label Jul 22, 2023
@schneiderfelipe
Copy link

@greyblake is there a way to build const values at present? Correct me if I'm wrong, but new_unchecked doesn't seem to be const either:

/// Creates a value of type skipping the sanitization and validation
/// rules. Generally, you should avoid using `::new_unchecked()` without a real need.
/// Use `::new()` instead when it's possible.
pub unsafe fn new_unchecked(inner_value: #inner_type) -> #type_name {
#type_name(inner_value)
}

@greyblake
Copy link
Owner Author

@schneiderfelipe hi!

is there a way to build const values at present?
Unfortunately no

Correct me if I'm wrong, but new_unchecked doesn't seem to be const either:
You're right, it does not.

One of the challenges with const is that it doesn't allow any arbitrary rust code. So to say, when we generate ::new const function, we have to be sure that everything what is within is const-compliant, but it is not very easy.

It's practically impossible to say upfront if a generated implementation will be const-compliant or not.

In order to avoid situation when nutype generates code with const fn new(), but by some reason, the new function cannot be const, I'd prefer either to introduce an extra attribute to the nutype macro, so users have control over that attribute.

@schneiderfelipe
Copy link

I understand. +1 for having users control whether ::new is const.

What about new_unchecked? It looks to me it could be const by default, what do you think?

@greyblake
Copy link
Owner Author

I am not sure why you're asking about new_unchecked again, it looks like you misunderstand it.
Please read Breaking constraints with new_unchecked

new_unchecked is to have a loophole to bypass sanitizers and validators (which I don't recommend normally).

It has nothing to do with const functions. It won't work for heap allocated types (e.g. String or anything else).

So if we take a very simplified code similar to what nutype generates:

struct Email(String);

impl Email {
    const fn new(email: &str) -> Email {
        Email(email.to_string())
    }
}

This won't compile:

error[E0015]: cannot call non-const fn `<str as ToString>::to_string` in constant functions
 --> src/main.rs:7:21
  |
7 |         Email(email.to_string())
  |                     ^^^^^^^^^^^
  |

@schneiderfelipe I am not sure what you're doing, but I guess you can just use either

So going forward with the Email example, the following code works as expected.

use nutype::nutype;
use once_cell::sync::Lazy;

#[nutype(
    sanitize(trim, lowercase),
    validate(not_empty, len_char_max = 50),
    derive(Debug, PartialEq, Clone, Display),
)]
struct Email(String);

static DEFAULT_EMAIL: Lazy<Email> =
    Lazy::new(|| Email::new("[email protected]").unwrap());

fn main() {
    println!("Default email is {}", *DEFAULT_EMAIL);
}

DEFAULT_EMAIL is initialized only once on start.

@schneiderfelipe
Copy link

Sorry, I may have mixed up things and I should have mentioned my use case from the start.

I would like to fill up an associated const value to my type, so I would be building values up at compile time, e.g.,

impl MyType {
    const my_value: MyType = MyType::new_([1, 2, 3]);
    // ...
}

I'm 100% sure those initializations won't break invariants. Since new is not const, I was expecting some other way of initializing those values at compile-time and new_unchecked just came to mind. Is there a way to do that?

@greyblake
Copy link
Owner Author

@schneiderfelipe

For your example to compile MyType::new must be defined as const fn new(..), which is not the case at the moment (new_unchecked does not help here)

@helgoboss
Copy link

helgoboss commented Feb 20, 2024

I have the same use case as @schneiderfelipe and I think new_unchecked would help here. The body of the new_unchecked function doesn't contain any validation logic, it just wraps the given value into the newtype. As a result it can be trivially made const, which would make it usable for associated constants. I don't think doing that would break any existing code.

Having associated constants can be very useful. I would love to migrate my newtypes (with existing constants) to nutype but not being able to migrate the constants turns out to be a showstopper.

@greyblake
Copy link
Owner Author

@helgoboss It's not that simple as it may seems.

  1. You don't wanna use nutype to easily avoid the validation constraints (if that's your case, you don't need nutype at all)
  2. Having const fn works very well for non-heap allocated things (like i32 or f64). It will not compile with String.

@greyblake
Copy link
Owner Author

But I see a demand for that. A solution could be at least partial support of const fn, where it's possible.

@helgoboss
Copy link

helgoboss commented Feb 20, 2024

Thanks for considering it. BTW, at this occasion, I really love the idea of nutype and how it's implemented. It makes writing robust and coherent newtypes much easier.

  1. You don't wanna use nutype to easily avoid the validation constraints (if that's your case, you don't need nutype at all)

I'm aware that new_unchecked is a compromise. I would certainly prefer new for this use case but there's no way to make it const at the moment (hopefully one day).

I appreciate that you want to make it hard to construct invalid types. On the other hand ... having some well-defined popular constants (with the author of the crate "promising" that they are correct) can actually contribute to safety for the consumers of this crate because constants can be used basically everywhere and are very convenient to use.

  1. Having const fn works very well for non-heap allocated things (like i32 or f64). It will not compile with String.

Declaring the function as const is not a problem, even if it takes a String argument. It compiles. What you can't do is actually using the function from a const context, simply because there's no way to create a const String. But I don't think that hurts.

I think adding a const is all it takes to make this work. Tried it with my own newtypes including a String wrapper:

helgoboss@e9f2538#diff-a50c55e38c78545fe73f1b3fb129600fd97bb29ce5e2afd4b95f5940132bcd12R17

@helgoboss
Copy link

helgoboss commented Feb 20, 2024

A potential alternative would be to add a dedicated syntax for adding associated constants and checking the validity of the wrapped values in the same way as the default value is checked at the moment (a unit test).

@nalply
Copy link

nalply commented Sep 9, 2024

I had the following situation, different, but similar.

I need to implement the Minimum for ConcurrentMap. Default would have helped if fn default() only were const.

#[nutype::nutype(..., derive(Default, ...), default = ...)]
PlayerId(TinyAsciiStr<8>)

impl Minimum for PlayerId {
  const MIN: Self = PlayerId::default();
}

parse() is also not const fn.

LazyLock doesn't help because you cannot assign consts from statics.

const MIN: Self = PlayerId(b"00000000") or whatever in PlayerId() won't work because nutype makes direct access of the tuple impossible.

If I didn't miss something there seems to be a real need to have const fn for getting specific values that is missing. Perhaps const fn const_default() which is provided whenever default = ... is used? Or more generally the ability to define associated consts in #[nutype], perhaps like this:

#[nutype(..., const = ..., ...)

@greyblake
Copy link
Owner Author

@nalply Thanks for your comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Specs required The issue requires analysis and detailed specification
Projects
None yet
Development

No branches or pull requests

4 participants