noir icon indicating copy to clipboard operation
noir copied to clipboard

feat: Add globals associated with integers

Open c410-f3r opened this issue 1 year ago • 6 comments

Description

Problem

Fix #4572

Summary

Adds several global/constant variables associated with integers into the new num module. Feel free to point out if such a module is not desirable.

AFAICT, the lack of an explicit type declaration allows maximum flexibility as long as the number literals can be inferred at the calling site.

// Allowed!
let x: u16 = std::num::U8_MAX;

Documentation

Check one:

  • [ ] No documentation needed.
  • [x] Documentation included in this PR.
  • [ ] [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist

  • [x] I have tested the changes locally.
  • [ ] I have formatted the changes with Prettier and/or cargo fmt on default settings.

c410-f3r avatar Sep 28 '24 23:09 c410-f3r

Thank you for your contribution to the Noir language.

Please do not force push to this branch after the Noir team have started review of this PR. Doing so will only delay us merging your PR as we will need to start the review process from scratch.

Thanks for your understanding.

github-actions[bot] avatar Sep 28 '24 23:09 github-actions[bot]

Can you confirm? The issue explicitly says that the constants should be in the standard library.

c410-f3r avatar Sep 30 '24 17:09 c410-f3r

Can you confirm? The issue explicitly says that the constants should be in the standard library.

Yes they should be in the standard library, but we allow for impls to be written on primitive types in the stdlib. Here are some examples: https://github.com/noir-lang/noir/blob/619c5451b152d62e01d3c4c1da7e13ff6502f915/noir_stdlib/src/cmp.nr#L31 and https://github.com/noir-lang/noir/blob/619c5451b152d62e01d3c4c1da7e13ff6502f915/noir_stdlib/src/field/mod.nr#L5.

U8_BITS can exist directly on the type. Like in the example posted above.

vezenovm avatar Oct 03 '24 17:10 vezenovm

but we allow for impls to be written on primitive types

OK, then it is about creating a trait and implement it for integers. Should be done now.

AFAICT it is not possible to use generic types or generic associated types so Field is the only solution at the current time. Field also implies numbers up to u32.

Only unsigned integer constants up to `u32`, globals, generics, +, -, *, /, and % may be used in this context.

run_stdlib_tests is passing but despite my efforts, I couldn't create unit tests.

  1. <T as IntConstans>::MAX: not yet implemented: Implement AsTraitPath.
  2. u8::MAX: noirc thinks that MAX is a function.

I kept the num module, feel free to say if this is undesirable or anything else.

c410-f3r avatar Oct 03 '24 23:10 c410-f3r

I think that it would be best for us to just wait on this feature until we can have an associated constant applied directly to the integer type impls here rather than bringing in a trait and dealing with the issues associated with needing to case these Fields (and the necessary breaking change to fix later on).

TomAFrench avatar Oct 04 '24 09:10 TomAFrench

Sure, I will wait.

c410-f3r avatar Oct 04 '24 10:10 c410-f3r