Too-large field literals silently truncated

[michaeljklein]

Aim

The following program, with the included Prover.toml, should fail to be solved when running nargo execute:

• main.nr:

fn main(x: Field) {
    let too_large: Field = 233149999999999999999999999999999999999999999999999999999999923314999999999999999999999999999999999999999999999999999999999923314999999999999999999999999999999999999999999999999999999999;
    assert(x == too_large);
}

• Prover.toml:

x = "5071095234663615940968590748783449899267984026467185323749645563121644617054"

Expected Behavior

The program should fail to compile because of an invalid Field literal.

Bug

The program successfully compiles and executes, solving an assertion that:

5071095234663615940968590748783449899267984026467185323749645563121644617054
==
233149999999999999999999999999999999999999999999999999999999923314999999999999999999999999999999999999999999999999999999999923314999999999999999999999999999999999999999999999999999999999

To Reproduce

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

[TomAFrench]

Note that this is currently used to determine which field we're using as we can use p == 0 to determine the modulus. Fixing this would need a replacement for this.

[michaeljklein]

@TomAFrench which code does that?

[jfecher]

@michaeljklein https://github.com/noir-lang/noir/blob/master/noir_stdlib/src/compat.nr#L3

[asterite]

Some more things I found.

There are already range checks for unsigned integers:

fn main(x: u8) {
    let too_large: u8 = 256; // The literal `2⁸` cannot fit into `u8` which has range `0..=255`
    assert(x == too_large);
}

It even works when the type is inferred from usage:

fn main(x: u8) {
    let too_large = 256; // 2⁸ does not fit within the type bounds for u8
    assert(x == too_large);
}

The same thing happens for signed integers:

fn main(x: i8) {
    let too_large = 256; // 2⁸ does not fit within the type bounds for i8
    assert(x == too_large);
}

However, this compiles fine:

fn main(x: i8) {
    let too_large = 255;
    assert(x == too_large);
}

In reality the maximum value of i8 is 127 so the above code should also be rejected. When we print the value:

fn main(x: i8) {
    let too_large = 255;
    println(too_large);
    assert(x == too_large);
}

we get -1 as the output.

So, I'll try to fix this too as part of this issue. I might also change the error message to include the possible range of values, like Rust does.

[asterite]

And things get a bit more tricky. This program:

fn main() {
    foo(21888242871839275222246405745257275088548364400416034343698204186575808495874)
}

fn foo(_x: u8) {}

gives this error:

error: 257 does not fit within the type bounds for u8
  ┌─ /Users/asterite/Projects/noir/test_programs/compile_failure/unsigned_integer_literal_overflow/src/main.nr:2:9
  │
2 │     foo(21888242871839275222246405745257275088548364400416034343698204186575808495874)
  │         -----------------------------------------------------------------------------

and this program compiles fine:

fn main() {
    foo(21888242871839275222246405745257275088548364400416034343698204186575808495617)
}

fn foo(_x: u8) {}

The reason is that numeric literals are stored as FieldElement:

noir/compiler/noirc_frontend/src/monomorphization/ast.rs

Line 96 in 03e25b4

Integer(FieldElement, Type, Location),

at least at the point where this check is done (in codegen):

noir/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs

Lines 274 to 277 in 03e25b4

	if !typ.value_is_within_limits(value) {
	let call_stack = self.builder.get_call_stack();
	return Err(RuntimeError::IntegerOutOfBounds { value, typ, call_stack });
	}

and the information of whether the literal was positive or negative is totally lost, making it impossible to check if the value fits or not into the target type.

So two options:

1. Include the original value in this Literal at this stage
2. Do this check earlier

Maybe 2 is better as doing these checks before codegen is probably preferred, though it might involve a larger change.

[asterite]

Actually, FieldElement starts at the token level:

noir/compiler/noirc_frontend/src/lexer/token.rs

Line 119 in 03e25b4

Int(FieldElement),

So I think it would make sense to change it there first. Or maybe there's a way to get the raw input to FieldElement and use that... otherwise, the value is already wrapped when tokenizing things and we can't know if it wrapped.

I'll timebox working on this, then jump to another issue and eventually come back to this one.

[asterite]

I captured another issue for checking overflow/underflow of other integer types: #5372