Skip to content

Commit

Permalink
feat!: Restrict bit sizes (#4235)
Browse files Browse the repository at this point in the history
# Description
Restricts allowed bit sizes for integer types to 1,8,32 and 64. 
## Problem\*

Resolves #4162

## Summary\*



## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
  • Loading branch information
sirasistant and kevaundray committed Feb 13, 2024
1 parent 78ef013 commit 1048f81
Show file tree
Hide file tree
Showing 21 changed files with 114 additions and 78 deletions.
5 changes: 4 additions & 1 deletion aztec_macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,10 @@ fn create_context(ty: &str, params: &[Param]) -> Result<Vec<Statement>, AztecMac
add_array_to_hasher(
&id,
&UnresolvedType {
typ: UnresolvedTypeData::Integer(Signedness::Unsigned, 32),
typ: UnresolvedTypeData::Integer(
Signedness::Unsigned,
noirc_frontend::IntegerBitSize::ThirtyTwo,
),
span: None,
},
)
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,8 @@ impl<'a> FunctionContext<'a> {
let element_types = Self::convert_type(element).flatten();
Type::Array(Rc::new(element_types), *len as usize)
}
ast::Type::Integer(Signedness::Signed, bits) => Type::signed(*bits),
ast::Type::Integer(Signedness::Unsigned, bits) => Type::unsigned(*bits),
ast::Type::Integer(Signedness::Signed, bits) => Type::signed((*bits).into()),
ast::Type::Integer(Signedness::Unsigned, bits) => Type::unsigned((*bits).into()),
ast::Type::Bool => Type::unsigned(1),
ast::Type::String(len) => Type::Array(Rc::new(vec![Type::char()]), *len as usize),
ast::Type::FmtString(_, _) => {
Expand Down
63 changes: 59 additions & 4 deletions compiler/noirc_frontend/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,63 @@ use crate::{
};
use iter_extended::vecmap;

#[derive(Debug, PartialEq, Eq, Clone, Copy, Hash, Ord, PartialOrd)]
pub enum IntegerBitSize {
One,
Eight,
ThirtyTwo,
SixtyFour,
}

impl IntegerBitSize {
pub fn allowed_sizes() -> Vec<Self> {
vec![Self::One, Self::Eight, Self::ThirtyTwo, Self::SixtyFour]
}
}

impl From<IntegerBitSize> for u32 {
fn from(size: IntegerBitSize) -> u32 {
use IntegerBitSize::*;
match size {
One => 1,
Eight => 8,
ThirtyTwo => 32,
SixtyFour => 64,
}
}
}

pub struct InvalidIntegerBitSizeError(pub u32);

impl TryFrom<u32> for IntegerBitSize {
type Error = InvalidIntegerBitSizeError;

fn try_from(value: u32) -> Result<Self, Self::Error> {
use IntegerBitSize::*;
match value {
1 => Ok(One),
8 => Ok(Eight),
32 => Ok(ThirtyTwo),
64 => Ok(SixtyFour),
_ => Err(InvalidIntegerBitSizeError(value)),
}
}
}

impl core::fmt::Display for IntegerBitSize {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", u32::from(*self))
}
}

/// The parser parses types as 'UnresolvedType's which
/// require name resolution to resolve any type names used
/// for structs within, but are otherwise identical to Types.
#[derive(Debug, PartialEq, Eq, Clone, Hash)]
pub enum UnresolvedTypeData {
FieldElement,
Array(Option<UnresolvedTypeExpression>, Box<UnresolvedType>), // [4]Witness = Array(4, Witness)
Integer(Signedness, u32), // u32 = Integer(unsigned, 32)
Integer(Signedness, IntegerBitSize), // u32 = Integer(unsigned, ThirtyTwo)
Bool,
Expression(UnresolvedTypeExpression),
String(Option<UnresolvedTypeExpression>),
Expand Down Expand Up @@ -197,11 +246,17 @@ impl UnresolvedType {
}

impl UnresolvedTypeData {
pub fn from_int_token(token: IntType) -> UnresolvedTypeData {
pub fn from_int_token(
token: IntType,
) -> Result<UnresolvedTypeData, InvalidIntegerBitSizeError> {
use {IntType::*, UnresolvedTypeData::Integer};
match token {
Signed(num_bits) => Integer(Signedness::Signed, num_bits),
Unsigned(num_bits) => Integer(Signedness::Unsigned, num_bits),
Signed(num_bits) => {
Ok(Integer(Signedness::Signed, IntegerBitSize::try_from(num_bits)?))
}
Unsigned(num_bits) => {
Ok(Integer(Signedness::Unsigned, IntegerBitSize::try_from(num_bits)?))
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_frontend/src/hir/type_check/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::hir_def::expr::HirBinaryOp;
use crate::hir_def::types::Type;
use crate::BinaryOpKind;
use crate::FunctionReturnType;
use crate::IntegerBitSize;
use crate::Signedness;

#[derive(Error, Debug, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -67,7 +68,7 @@ pub enum TypeCheckError {
#[error("Integers must have the same signedness LHS is {sign_x:?}, RHS is {sign_y:?}")]
IntegerSignedness { sign_x: Signedness, sign_y: Signedness, span: Span },
#[error("Integers must have the same bit width LHS is {bit_width_x}, RHS is {bit_width_y}")]
IntegerBitWidth { bit_width_x: u32, bit_width_y: u32, span: Span },
IntegerBitWidth { bit_width_x: IntegerBitSize, bit_width_y: IntegerBitSize, span: Span },
#[error("{kind} cannot be used in an infix operation")]
InvalidInfixOp { kind: &'static str, span: Span },
#[error("{kind} cannot be used in a unary operation")]
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir/type_check/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ impl<'interner> TypeChecker<'interner> {
HirExpression::Literal(HirLiteral::Integer(value, false)) => {
let v = value.to_u128();
if let Type::Integer(_, bit_count) = annotated_type {
let bit_count: u32 = (*bit_count).into();
let max = 1 << bit_count;
if v >= max {
self.errors.push(TypeCheckError::OverflowingAssignment {
Expand Down
13 changes: 8 additions & 5 deletions compiler/noirc_frontend/src/hir_def/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::{
use crate::{
hir::type_check::TypeCheckError,
node_interner::{ExprId, NodeInterner, TraitId, TypeAliasId},
IntegerBitSize,
};
use iter_extended::vecmap;
use noirc_errors::{Location, Span};
Expand All @@ -27,8 +28,8 @@ pub enum Type {
Array(Box<Type>, Box<Type>),

/// A primitive integer type with the given sign and bit count.
/// E.g. `u32` would be `Integer(Unsigned, 32)`
Integer(Signedness, u32),
/// E.g. `u32` would be `Integer(Unsigned, ThirtyTwo)`
Integer(Signedness, IntegerBitSize),

/// The primitive `bool` type.
Bool,
Expand Down Expand Up @@ -538,7 +539,7 @@ impl Type {
}

pub fn default_range_loop_type() -> Type {
Type::Integer(Signedness::Unsigned, 64)
Type::Integer(Signedness::Unsigned, IntegerBitSize::SixtyFour)
}

pub fn type_variable(id: TypeVariableId) -> Type {
Expand Down Expand Up @@ -1621,8 +1622,10 @@ impl From<&Type> for PrintableType {
PrintableType::Array { length, typ: Box::new(typ.into()) }
}
Type::Integer(sign, bit_width) => match sign {
Signedness::Unsigned => PrintableType::UnsignedInteger { width: *bit_width },
Signedness::Signed => PrintableType::SignedInteger { width: *bit_width },
Signedness::Unsigned => {
PrintableType::UnsignedInteger { width: (*bit_width).into() }
}
Signedness::Signed => PrintableType::SignedInteger { width: (*bit_width).into() },
},
Type::TypeVariable(binding, TypeVariableKind::IntegerOrField) => {
match &*binding.borrow() {
Expand Down
10 changes: 0 additions & 10 deletions compiler/noirc_frontend/src/lexer/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ pub enum LexerErrorKind {
InvalidIntegerLiteral { span: Span, found: String },
#[error("{:?} is not a valid attribute", found)]
MalformedFuncAttribute { span: Span, found: String },
#[error("Integer type is larger than the maximum supported size of u127")]
TooManyBits { span: Span, max: u32, got: u32 },
#[error("Logical and used instead of bitwise and")]
LogicalAnd { span: Span },
#[error("Unterminated block comment")]
Expand All @@ -45,7 +43,6 @@ impl LexerErrorKind {
LexerErrorKind::NotADoubleChar { span, .. } => *span,
LexerErrorKind::InvalidIntegerLiteral { span, .. } => *span,
LexerErrorKind::MalformedFuncAttribute { span, .. } => *span,
LexerErrorKind::TooManyBits { span, .. } => *span,
LexerErrorKind::LogicalAnd { span } => *span,
LexerErrorKind::UnterminatedBlockComment { span } => *span,
LexerErrorKind::UnterminatedStringLiteral { span } => *span,
Expand Down Expand Up @@ -85,13 +82,6 @@ impl LexerErrorKind {
format!(" {found} is not a valid attribute"),
*span,
),
LexerErrorKind::TooManyBits { span, max, got } => (
"Integer literal too large".to_string(),
format!(
"The maximum number of bits needed to represent a field is {max}, This integer type needs {got} bits"
),
*span,
),
LexerErrorKind::LogicalAnd { span } => (
"Noir has no logical-and (&&) operator since short-circuiting is much less efficient when compiling to circuits".to_string(),
"Try `&` instead, or use `if` only if you require short-circuiting".to_string(),
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/lexer/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ impl<'a> Lexer<'a> {

// Check if word an int type
// if no error occurred, then it is either a valid integer type or it is not an int type
let parsed_token = IntType::lookup_int_type(&word, Span::inclusive(start, end))?;
let parsed_token = IntType::lookup_int_type(&word)?;

// Check if it is an int type
if let Some(int_type_token) = parsed_token {
Expand Down
8 changes: 1 addition & 7 deletions compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ impl IntType {
// XXX: Result<Option<Token, LexerErrorKind>
// Is not the best API. We could split this into two functions. One that checks if the the
// word is a integer, which only returns an Option
pub(crate) fn lookup_int_type(word: &str, span: Span) -> Result<Option<Token>, LexerErrorKind> {
pub(crate) fn lookup_int_type(word: &str) -> Result<Option<Token>, LexerErrorKind> {
// Check if the first string is a 'u' or 'i'

let is_signed = if word.starts_with('i') {
Expand All @@ -324,12 +324,6 @@ impl IntType {
Err(_) => return Ok(None),
};

let max_bits = FieldElement::max_num_bits() / 2;

if str_as_u32 > max_bits {
return Err(LexerErrorKind::TooManyBits { span, max: max_bits, got: str_as_u32 });
}

if is_signed {
Ok(Some(Token::IntType(IntType::Signed(str_as_u32))))
} else {
Expand Down
7 changes: 4 additions & 3 deletions compiler/noirc_frontend/src/monomorphization/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use noirc_errors::{
};

use crate::{
hir_def::function::FunctionSignature, BinaryOpKind, Distinctness, Signedness, Visibility,
hir_def::function::FunctionSignature, BinaryOpKind, Distinctness, IntegerBitSize, Signedness,
Visibility,
};

/// The monomorphized AST is expression-based, all statements are also
Expand Down Expand Up @@ -217,8 +218,8 @@ pub struct Function {
#[derive(Debug, PartialEq, Eq, Clone, Hash)]
pub enum Type {
Field,
Array(/*len:*/ u64, Box<Type>), // Array(4, Field) = [Field; 4]
Integer(Signedness, /*bits:*/ u32), // u32 = Integer(unsigned, 32)
Array(/*len:*/ u64, Box<Type>), // Array(4, Field) = [Field; 4]
Integer(Signedness, /*bits:*/ IntegerBitSize), // u32 = Integer(unsigned, ThirtyTwo)
Bool,
String(/*len:*/ u64), // String(4) = str[4]
FmtString(/*len:*/ u64, Box<Type>),
Expand Down
15 changes: 8 additions & 7 deletions compiler/noirc_frontend/src/monomorphization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ use crate::{
},
node_interner::{self, DefinitionKind, NodeInterner, StmtId, TraitImplKind, TraitMethodId},
token::FunctionAttribute,
ContractFunctionType, FunctionKind, Type, TypeBinding, TypeBindings, TypeVariable,
TypeVariableKind, UnaryOp, Visibility,
ContractFunctionType, FunctionKind, IntegerBitSize, Type, TypeBinding, TypeBindings,
TypeVariable, TypeVariableKind, UnaryOp, Visibility,
};

use self::ast::{Definition, FuncId, Function, LocalId, Program};
Expand Down Expand Up @@ -354,6 +354,7 @@ impl<'interner> Monomorphizer<'interner> {
match typ {
ast::Type::Field => Literal(Integer(-value, typ, location)),
ast::Type::Integer(_, bit_size) => {
let bit_size: u32 = bit_size.into();
let base = 1_u128 << bit_size;
Literal(Integer(FieldElement::from(base) - value, typ, location))
}
Expand Down Expand Up @@ -1109,19 +1110,19 @@ impl<'interner> Monomorphizer<'interner> {
}
"modulus_le_bits" => {
let bits = FieldElement::modulus().to_radix_le(2);
Some(self.modulus_array_literal(bits, 1, location))
Some(self.modulus_array_literal(bits, IntegerBitSize::One, location))
}
"modulus_be_bits" => {
let bits = FieldElement::modulus().to_radix_be(2);
Some(self.modulus_array_literal(bits, 1, location))
Some(self.modulus_array_literal(bits, IntegerBitSize::One, location))
}
"modulus_be_bytes" => {
let bytes = FieldElement::modulus().to_bytes_be();
Some(self.modulus_array_literal(bytes, 8, location))
Some(self.modulus_array_literal(bytes, IntegerBitSize::Eight, location))
}
"modulus_le_bytes" => {
let bytes = FieldElement::modulus().to_bytes_le();
Some(self.modulus_array_literal(bytes, 8, location))
Some(self.modulus_array_literal(bytes, IntegerBitSize::Eight, location))
}
_ => None,
};
Expand All @@ -1133,7 +1134,7 @@ impl<'interner> Monomorphizer<'interner> {
fn modulus_array_literal(
&self,
bytes: Vec<u8>,
arr_elem_bits: u32,
arr_elem_bits: IntegerBitSize,
location: Location,
) -> ast::Expression {
use ast::*;
Expand Down
13 changes: 6 additions & 7 deletions compiler/noirc_frontend/src/parser/errors.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::lexer::errors::LexerErrorKind;
use crate::lexer::token::Token;
use crate::Expression;
use crate::IntegerBitSize;
use small_ord_set::SmallOrdSet;
use thiserror::Error;

Expand Down Expand Up @@ -40,8 +41,8 @@ pub enum ParserErrorReason {
NoFunctionAttributesAllowedOnStruct,
#[error("Assert statements can only accept string literals")]
AssertMessageNotString,
#[error("Integer bit size {0} won't be supported")]
DeprecatedBitSize(u32),
#[error("Integer bit size {0} isn't supported")]
InvalidBitSize(u32),
#[error("{0}")]
Lexer(LexerErrorKind),
}
Expand Down Expand Up @@ -132,8 +133,6 @@ impl std::fmt::Display for ParserError {
}
}

pub(crate) static ALLOWED_INTEGER_BIT_SIZES: &[u32] = &[1, 8, 32, 64];

impl From<ParserError> for Diagnostic {
fn from(error: ParserError) -> Diagnostic {
match error.reason {
Expand All @@ -149,9 +148,9 @@ impl From<ParserError> for Diagnostic {
"The 'comptime' keyword has been deprecated. It can be removed without affecting your program".into(),
error.span,
),
ParserErrorReason::DeprecatedBitSize(bit_size) => Diagnostic::simple_warning(
format!("Use of deprecated bit size {}", bit_size),
format!("Bit sizes for integers will be restricted to {}", ALLOWED_INTEGER_BIT_SIZES.iter().map(|n| n.to_string()).collect::<Vec<_>>().join(", ")),
ParserErrorReason::InvalidBitSize(bit_size) => Diagnostic::simple_error(
format!("Use of invalid bit size {}", bit_size),
format!("Allowed bit sizes for integers are {}", IntegerBitSize::allowed_sizes().iter().map(|n| n.to_string()).collect::<Vec<_>>().join(", ")),
error.span,
),
ParserErrorReason::ExperimentalFeature(_) => Diagnostic::simple_warning(
Expand Down
22 changes: 8 additions & 14 deletions compiler/noirc_frontend/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
//! prevent other parsers from being tried afterward since there is no longer an error. Thus, they should
//! be limited to cases like the above `fn` example where it is clear we shouldn't back out of the
//! current parser to try alternative parsers in a `choice` expression.
use super::errors::ALLOWED_INTEGER_BIT_SIZES;
use super::{
foldl_with_span, labels::ParsingRuleLabel, parameter_name_recovery, parameter_recovery,
parenthesized, then_commit, then_commit_ignore, top_level_statement_recovery, ExprParser,
Expand All @@ -36,7 +35,7 @@ use crate::ast::{
};
use crate::lexer::Lexer;
use crate::parser::{force, ignore_then_commit, statement_recovery};
use crate::token::{Attribute, Attributes, IntType, Keyword, SecondaryAttribute, Token, TokenKind};
use crate::token::{Attribute, Attributes, Keyword, SecondaryAttribute, Token, TokenKind};
use crate::{
BinaryOp, BinaryOpKind, BlockExpression, ConstrainKind, ConstrainStatement, Distinctness,
ForLoopStatement, ForRange, FunctionDefinition, FunctionReturnType, FunctionVisibility, Ident,
Expand Down Expand Up @@ -1093,19 +1092,14 @@ fn int_type() -> impl NoirParser<UnresolvedType> {
Err(ParserError::expected_label(ParsingRuleLabel::IntegerType, unexpected, span))
}
}))
.validate(|int_type, span, emit| {
let bit_size = match int_type.1 {
IntType::Signed(bit_size) | IntType::Unsigned(bit_size) => bit_size,
};
if !ALLOWED_INTEGER_BIT_SIZES.contains(&bit_size) {
emit(ParserError::with_reason(
ParserErrorReason::DeprecatedBitSize(bit_size),
span,
));
}
int_type
.validate(|(_, token), span, emit| {
UnresolvedTypeData::from_int_token(token)
.map(|data| data.with_span(span))
.unwrap_or_else(|err| {
emit(ParserError::with_reason(ParserErrorReason::InvalidBitSize(err.0), span));
UnresolvedType::error(span)
})
})
.map_with_span(|(_, token), span| UnresolvedTypeData::from_int_token(token).with_span(span))
}

fn named_type(type_parser: impl NoirParser<UnresolvedType>) -> impl NoirParser<UnresolvedType> {
Expand Down
Loading

0 comments on commit 1048f81

Please sign in to comment.