feat(experimental!): let the default integer type be i32 instead of Field#8965
feat(experimental!): let the default integer type be i32 instead of Field#8965
Conversation
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
There was a problem hiding this comment.
I agree that (-56) as i8 = -55 is confusing but I think defaulting to Field is still sensible from an efficiency standpoint as well as a "what type is most commonly used?" standpoint.
I also think we should keep allowing negative literals for field since negatives are still consistent within field operations. E.g. -5 + 7 = 2. (If anything I think field division is more confusing).
If we don't want (-56) as i8 = -55 as the expected semantics then I'd lean toward altering the field to signed int cast instead. We could add a branch in the cast which would make it slower but perhaps more expected. This may be fine for the signed integer types which are rarely used today:
// This would be built-in, but just to show the logic:
fn field_to_i8(x: Field) -> Option<i8> {
// Absolute value of i8::MIN
let abs_min = pow(2, 7);
// I may be off by 1 here, don't quote me
if x >= -abs_min {
truncate(x + abs_min)
} else {
truncate(x)
}
}
I agree, we could do that. However, I'd find it a bit odd that Maybe the odd thing is that printing the Field |
|
The above would also mean that this could shouldn't compile: fn main() {
let x = 21888242871839275222246405745257275088548364400416034343698204186575808495616;
println(x);
}as |
|
@asterite I don't think we should dedicate bit patterns in field to be exclusively negative unless we wanted to add it as a separate signed field type. For the printing discrepancy, I just think we're going to have to draw the line somewhere.
|
|
Thanks! I'll close this in the meantime. It doesn't seem this change breaks a lot of things, which is what I wanted to know when creating this PR. The compilation errors are:
|
@jfecher I kept thinking about this and wondering if this is still true today: I understand that Field is the most efficient type, but according to the above PR we want to discourage users from using Field. |
2 participants
Description
Problem
No reported issue, but having Field be the default integer type might lead to confusion.
Summary
Note: this is just an experiment to see how much code breaks with this change. There's also this bug that, when fixed, should lead to even less breakage (I'll try to fix that next).
The main issue with having integer literals default to Field is that negative literals are allowed, but Field is mainly an unsigned type. It's okay to allow negative literals for it, but that should only probably be allowed with an explicit type annotation, or if the literal destination is a Field. For example right now
(-56) as i8gives-55, because the field-56is actually a very big positive number that, when casted toi8, gives-55. With the change in this PR,-56is ani32, and casting that toi8preserves the value-56. But regardless of this issue, it might make more sense for the default type to be a signed type because literals can be positive or negative.The reasoning here is that because Noir, like Rust, requires type annotations in function signatures, struct types, etc., changing the default from
Fieldtoi32shouldn't have a big impact, because if existing code expectsField, it will still useField, and code would still compile.For example:
Here it doesn't matter what that
1defaults to, because the type will be bound to the call argument. It's the same case if that's assigned to a struct field, etc. The only issue right now is #8963 where this isn't true, but that's a bug that we can fix.Then in isolated code the type can change. For example here:
That used to print
0x01, but now it prints1becausexisi32instead ofField. But that code is mostly used in tests. For example, only one change was required to be done tonoir_stdlibto make it compile again, and it was in some test code related to macros.In any case, this is just experimental! I understand that Field is more efficient than any other integer type, but my hope is that this change doesn't actually impact performance.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.