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

feat(cli)!: Add --unstable-features to gate unstable features #7449

Merged
merged 17 commits into from
Feb 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use noirc_evaluator::errors::RuntimeError;
use noirc_evaluator::ssa::{SsaLogging, SsaProgramArtifact};
use noirc_frontend::debug::build_debug_crate_file;
use noirc_frontend::elaborator::{FrontendOptions, UnstableFeature};
use noirc_frontend::hir::def_map::{Contract, CrateDefMap};
use noirc_frontend::hir::Context;
use noirc_frontend::monomorphization::{
Expand Down Expand Up @@ -150,11 +151,11 @@
#[arg(long, hide = true)]
pub skip_brillig_constraints_check: bool,

/// Flag to turn on the lookback feature of the Brillig call constraints

Check warning on line 154 in compiler/noirc_driver/src/lib.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (lookback)
/// check, allowing tracking argument values before the call happens preventing
/// certain rare false positives (leads to a slowdown on large rollout functions)
#[arg(long)]
pub enable_brillig_constraints_check_lookback: bool,

Check warning on line 158 in compiler/noirc_driver/src/lib.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (lookback)

/// Setting to decide on an inlining strategy for Brillig functions.
/// A more aggressive inliner should generate larger programs but more optimized
Expand All @@ -179,6 +180,11 @@
/// Used internally to test for non-determinism in the compiler.
#[clap(long, hide = true)]
pub check_non_determinism: bool,

/// Unstable features to enable for this current build
#[arg(value_parser = clap::value_parser!(UnstableFeature))]
#[clap(long, short = 'Z', value_delimiter = ',')]
pub unstable_features: Vec<UnstableFeature>,
}

pub fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::Error> {
Expand All @@ -197,6 +203,16 @@
}
}

impl CompileOptions {
pub fn frontend_options(&self) -> FrontendOptions {
FrontendOptions {
debug_comptime_in_file: self.debug_comptime_in_file.as_deref(),
pedantic_solving: self.pedantic_solving,
enabled_unstable_features: &self.unstable_features,
}
}
}

#[derive(Debug)]
pub enum CompileError {
MonomorphizationError(MonomorphizationError),
Expand Down Expand Up @@ -336,12 +352,7 @@
crate_id: CrateId,
options: &CompileOptions,
) -> CompilationResult<()> {
let diagnostics = CrateDefMap::collect_defs(
crate_id,
context,
options.debug_comptime_in_file.as_deref(),
options.pedantic_solving,
);
let diagnostics = CrateDefMap::collect_defs(crate_id, context, options.frontend_options());
let crate_files = context.crate_files(&crate_id);
let warnings_and_errors: Vec<FileDiagnostic> = diagnostics
.into_iter()
Expand Down Expand Up @@ -697,8 +708,8 @@
},
emit_ssa: if options.emit_ssa { Some(context.package_build_path.clone()) } else { None },
skip_underconstrained_check: options.skip_underconstrained_check,
enable_brillig_constraints_check_lookback: options

Check warning on line 711 in compiler/noirc_driver/src/lib.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (lookback)
.enable_brillig_constraints_check_lookback,

Check warning on line 712 in compiler/noirc_driver/src/lib.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (lookback)
enable_brillig_constraints_check: options.enable_brillig_constraints_check,
inliner_aggressiveness: options.inliner_aggressiveness,
max_bytecode_increase_percent: options.max_bytecode_increase_percent,
Expand Down
1 change: 0 additions & 1 deletion compiler/noirc_frontend/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ strum.workspace = true
strum_macros.workspace = true
fxhash.workspace = true


[dev-dependencies]
base64.workspace = true
proptest.workspace = true
Expand Down
7 changes: 3 additions & 4 deletions compiler/noirc_frontend/src/elaborator/comptime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,8 @@ impl<'context> Elaborator<'context> {
self.usage_tracker,
self.crate_graph,
self.crate_id,
self.debug_comptime_in_file,
self.interpreter_call_stack.clone(),
self.pedantic_solving,
self.options,
self.elaborate_reasons.clone(),
);

Expand Down Expand Up @@ -496,15 +495,15 @@ impl<'context> Elaborator<'context> {
Some(DependencyId::Function(function)) => Some(function),
_ => None,
};
Interpreter::new(self, self.crate_id, current_function, self.pedantic_solving)
Interpreter::new(self, self.crate_id, current_function)
}

pub(super) fn debug_comptime<T: Display, F: FnMut(&mut NodeInterner) -> T>(
&mut self,
location: Location,
mut expr_f: F,
) {
if Some(location.file) == self.debug_comptime_in_file {
if Some(location.file) == self.options.debug_comptime_in_file {
let displayed_expr = expr_f(self.interner);
let error: CompilationError =
InterpreterError::debug_evaluate_comptime(displayed_expr, location).into();
Expand Down
2 changes: 2 additions & 0 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1107,6 +1107,8 @@ impl<'context> Elaborator<'context> {
location: Location,
) -> (HirExpression, Type) {
let span = location.span;
self.use_unstable_feature(super::UnstableFeature::Enums, span);

let (expression, typ) = self.elaborate_expression(match_expr.expression);
let (let_, variable) = self.wrap_in_let(expression, typ);

Expand Down
67 changes: 37 additions & 30 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use crate::{
DefinitionKind, DependencyId, ExprId, FuncId, FunctionModifiers, GlobalId, NodeInterner,
ReferenceId, TraitId, TraitImplId, TypeAliasId, TypeId,
},
parser::{ParserError, ParserErrorReason},
token::SecondaryAttribute,
EnumVariant, Shared, Type, TypeVariable,
};
Expand All @@ -52,6 +53,7 @@ mod comptime;
mod enums;
mod expressions;
mod lints;
mod options;
mod path_resolution;
mod patterns;
mod scope;
Expand All @@ -63,7 +65,9 @@ mod unquote;

use fm::FileId;
use iter_extended::vecmap;
use noirc_errors::{Located, Location};
use noirc_errors::{Located, Location, Span};
pub(crate) use options::ElaboratorOptions;
pub use options::{FrontendOptions, UnstableFeature};
pub use path_resolution::Turbofish;
use path_resolution::{PathResolution, PathResolutionItem};
use types::bind_ordered_generics;
Expand Down Expand Up @@ -179,9 +183,6 @@ pub struct Elaborator<'context> {

crate_id: CrateId,

/// The scope of --debug-comptime, or None if unset
debug_comptime_in_file: Option<FileId>,

/// These are the globals that have yet to be elaborated.
/// This map is used to lazily evaluate these globals if they're encountered before
/// they are elaborated (e.g. in a function's type or another global's RHS).
Expand All @@ -195,8 +196,8 @@ pub struct Elaborator<'context> {
/// that comptime value and any visibility errors were already reported.
silence_field_visibility_errors: usize,

/// Use pedantic ACVM solving
pedantic_solving: bool,
/// Options from the nargo cli
options: ElaboratorOptions<'context>,

/// Sometimes items are elaborated because a function attribute ran and generated items.
/// The Elaborator keeps track of these reasons so that when an error is produced it will
Expand All @@ -211,6 +212,7 @@ pub enum ElaborateReason {
/// Evaluating `Module::add_item`
AddingItemToModule,
}

impl ElaborateReason {
fn to_macro_error(self, error: CompilationError, location: Location) -> ComptimeError {
match self {
Expand Down Expand Up @@ -250,9 +252,8 @@ impl<'context> Elaborator<'context> {
usage_tracker: &'context mut UsageTracker,
crate_graph: &'context CrateGraph,
crate_id: CrateId,
debug_comptime_in_file: Option<FileId>,
interpreter_call_stack: im::Vector<Location>,
pedantic_solving: bool,
options: ElaboratorOptions<'context>,
elaborate_reasons: im::Vector<(ElaborateReason, Location)>,
) -> Self {
Self {
Expand All @@ -275,32 +276,29 @@ impl<'context> Elaborator<'context> {
trait_bounds: Vec::new(),
function_context: vec![FunctionContext::default()],
current_trait_impl: None,
debug_comptime_in_file,
unresolved_globals: BTreeMap::new(),
current_trait: None,
interpreter_call_stack,
in_comptime_context: false,
silence_field_visibility_errors: 0,
pedantic_solving,
options,
elaborate_reasons,
}
}

pub fn from_context(
context: &'context mut Context,
crate_id: CrateId,
debug_comptime_in_file: Option<FileId>,
pedantic_solving: bool,
options: ElaboratorOptions<'context>,
) -> Self {
Self::new(
&mut context.def_interner,
&mut context.def_maps,
&mut context.usage_tracker,
&context.crate_graph,
crate_id,
debug_comptime_in_file,
im::Vector::new(),
pedantic_solving,
options,
im::Vector::new(),
)
}
Expand All @@ -309,28 +307,18 @@ impl<'context> Elaborator<'context> {
context: &'context mut Context,
crate_id: CrateId,
items: CollectedItems,
debug_comptime_in_file: Option<FileId>,
pedantic_solving: bool,
options: ElaboratorOptions<'context>,
) -> Vec<(CompilationError, FileId)> {
Self::elaborate_and_return_self(
context,
crate_id,
items,
debug_comptime_in_file,
pedantic_solving,
)
.errors
Self::elaborate_and_return_self(context, crate_id, items, options).errors
}

pub fn elaborate_and_return_self(
context: &'context mut Context,
crate_id: CrateId,
items: CollectedItems,
debug_comptime_in_file: Option<FileId>,
pedantic_solving: bool,
options: ElaboratorOptions<'context>,
) -> Self {
let mut this =
Self::from_context(context, crate_id, debug_comptime_in_file, pedantic_solving);
let mut this = Self::from_context(context, crate_id, options);
this.elaborate_items(items);
this.check_and_pop_function_context();
this
Expand Down Expand Up @@ -414,6 +402,11 @@ impl<'context> Elaborator<'context> {
self.push_errors(self.interner.check_for_dependency_cycles());
}

/// True if we should use pedantic ACVM solving
pub fn pedantic_solving(&self) -> bool {
self.options.pedantic_solving
}

/// Runs `f` and if it modifies `self.generics`, `self.generics` is truncated
/// back to the previous length.
fn recover_generics<T>(&mut self, f: impl FnOnce(&mut Self) -> T) -> T {
Expand Down Expand Up @@ -1941,8 +1934,12 @@ impl<'context> Elaborator<'context> {
self.generics.clear();

let datatype = self.interner.get_type(*type_id);
let generics = datatype.borrow().generic_types();
self.add_existing_generics(&typ.enum_def.generics, &datatype.borrow().generics);
let datatype_ref = datatype.borrow();
let generics = datatype_ref.generic_types();
self.add_existing_generics(&typ.enum_def.generics, &datatype_ref.generics);

self.use_unstable_feature(UnstableFeature::Enums, datatype_ref.name.span());
drop(datatype_ref);

let self_type = Type::DataType(datatype.clone(), generics);
let self_type_id = self.interner.push_quoted_type(self_type.clone());
Expand Down Expand Up @@ -2227,4 +2224,14 @@ impl<'context> Elaborator<'context> {
_ => true,
})
}

/// Register a use of the given unstable feature. Errors if the feature has not
/// been explicitly enabled in this package.
pub fn use_unstable_feature(&mut self, feature: UnstableFeature, span: Span) {
if !self.options.enabled_unstable_features.contains(&feature) {
let reason = ParserErrorReason::ExperimentalFeature(feature);
let location = Location::new(span, self.file);
self.push_err(ParserError::with_reason(reason, location), self.file);
}
}
}
62 changes: 62 additions & 0 deletions compiler/noirc_frontend/src/elaborator/options.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
use std::str::FromStr;

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum UnstableFeature {
Enums,
ArrayOwnership,
}

impl std::fmt::Display for UnstableFeature {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Self::Enums => write!(f, "enums"),
Self::ArrayOwnership => write!(f, "array-ownership"),
}
}
}

impl FromStr for UnstableFeature {
type Err = String;

fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"enums" => Ok(Self::Enums),
"array-ownership" => Ok(Self::ArrayOwnership),
other => Err(format!("Unknown unstable feature '{other}'")),
}
}
}

/// Generic options struct meant to resolve to ElaboratorOptions below when
/// we can resolve a file path to a file id later. This generic struct is used
/// so that FrontendOptions doesn't need to duplicate fields and methods with ElaboratorOptions.
#[derive(Copy, Clone)]
pub struct GenericOptions<'a, T> {
/// The scope of --debug-comptime, or None if unset
pub debug_comptime_in_file: Option<T>,

/// Use pedantic ACVM solving
pub pedantic_solving: bool,

/// Unstable compiler features that were explicitly enabled. Any unstable features
/// that are not in this list result in an error when used.
pub enabled_unstable_features: &'a [UnstableFeature],
}

/// Options from nargo_cli that need to be passed down to the elaborator
pub(crate) type ElaboratorOptions<'a> = GenericOptions<'a, fm::FileId>;

/// This is the unresolved version of `ElaboratorOptions`
/// CLI options that need to be passed to the compiler frontend (the elaborator).
pub type FrontendOptions<'a> = GenericOptions<'a, &'a str>;

impl<'a, T> GenericOptions<'a, T> {
/// A sane default of frontend options for running tests
pub fn test_default() -> GenericOptions<'static, T> {
GenericOptions {
debug_comptime_in_file: None,
pedantic_solving: true,
enabled_unstable_features: &[UnstableFeature::Enums],
}
}
}
6 changes: 1 addition & 5 deletions compiler/noirc_frontend/src/hir/comptime/interpreter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,6 @@ pub struct Interpreter<'local, 'interner> {

/// Stateful bigint calculator.
bigint_solver: BigIntSolverWithId,

/// Use pedantic ACVM solving
pedantic_solving: bool,
}

#[allow(unused)]
Expand All @@ -78,8 +75,8 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
elaborator: &'local mut Elaborator<'interner>,
crate_id: CrateId,
current_function: Option<FuncId>,
pedantic_solving: bool,
) -> Self {
let pedantic_solving = elaborator.pedantic_solving();
let bigint_solver = BigIntSolverWithId::with_pedantic_solving(pedantic_solving);
Self {
elaborator,
Expand All @@ -88,7 +85,6 @@ impl<'local, 'interner> Interpreter<'local, 'interner> {
bound_generics: Vec::new(),
in_loop: false,
bigint_solver,
pedantic_solving,
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ impl<'local, 'context> Interpreter<'local, 'context> {
arguments,
return_type,
location,
self.pedantic_solving,
self.elaborator.pedantic_solving(),
)
}
}
Expand Down
6 changes: 2 additions & 4 deletions compiler/noirc_frontend/src/hir/comptime/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use noirc_errors::Location;
use super::errors::InterpreterError;
use super::value::Value;
use super::Interpreter;
use crate::elaborator::Elaborator;
use crate::elaborator::{Elaborator, ElaboratorOptions};
use crate::hir::def_collector::dc_crate::{CompilationError, DefCollector};
use crate::hir::def_collector::dc_mod::collect_defs;
use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleData};
Expand Down Expand Up @@ -60,13 +60,11 @@ pub(crate) fn with_interpreter<T>(

let main = context.get_main_function(&krate).expect("Expected 'main' function");

let pedantic_solving = true;
let mut elaborator = Elaborator::elaborate_and_return_self(
&mut context,
krate,
collector.items,
None,
pedantic_solving,
ElaboratorOptions::test_default(),
);

let errors = elaborator.errors.clone();
Expand Down
Loading
Loading