Skip to content

Commit

Permalink
chore: Remove --use-legacy and resolution code (#5248)
Browse files Browse the repository at this point in the history 
# Description

## Problem\*

## Summary\*

Made this PR as an experiment to see how much legacy code we can remove
today.

It looks like we can remove the majority of resolution code at the
`dc_crate` level, but we can't remove the NameResolver or TypeChecker
since these are used by the aztec macros crate. This is something we'll
have to watch out for when the elaborator is used by default - that code
injected by macros will still use the old resolution & type checking
code. I'm not aware of any compatibility errors when running both but it
is something to keep in mind.

Edit: Removed the NameResolver and TypeChecker in a separate PR:
#5525

## Additional Context

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] 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.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
@jfecher
jfecher authored Jul 16, 2024
1 parent af7d481 commit e30b347
Show file tree
Hide file tree
Showing 24 changed files with 27 additions and 1,359 deletions.
10 changes: 1 addition & 9 deletions compiler/noirc_driver/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,6 @@ pub struct CompileOptions {
#[arg(long, hide = true)]
pub force_brillig: bool,

/// Use the deprecated name resolution & type checking passes instead of the elaborator
#[arg(long, hide = true)]
pub use_legacy: bool,

/// Enable printing results of comptime evaluation: provide a path suffix
/// for the module to debug, e.g. "package_name/src/main.nr"
#[arg(long)]
Expand Down Expand Up @@ -262,15 +258,13 @@ pub fn check_crate(
crate_id: CrateId,
deny_warnings: bool,
disable_macros: bool,
use_legacy: bool,
debug_comptime_in_file: Option<&str>,
) -> CompilationResult<()> {
let macros: &[&dyn MacroProcessor] =
if disable_macros { &[] } else { &[&aztec_macros::AztecMacro as &dyn MacroProcessor] };

let mut errors = vec![];
let diagnostics =
CrateDefMap::collect_defs(crate_id, context, use_legacy, debug_comptime_in_file, macros);
let diagnostics = CrateDefMap::collect_defs(crate_id, context, debug_comptime_in_file, macros);
errors.extend(diagnostics.into_iter().map(|(error, file_id)| {
let diagnostic = CustomDiagnostic::from(&error);
diagnostic.in_file(file_id)
Expand Down Expand Up @@ -307,7 +301,6 @@ pub fn compile_main(
crate_id,
options.deny_warnings,
options.disable_macros,
options.use_legacy,
options.debug_comptime_in_file.as_deref(),
)?;

Expand Down Expand Up @@ -349,7 +342,6 @@ pub fn compile_contract(
crate_id,
options.deny_warnings,
options.disable_macros,
options.use_legacy,
options.debug_comptime_in_file.as_deref(),
)?;

Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_driver/tests/stdlib_warnings.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings>
let root_crate_id = prepare_crate(&mut context, file_name);

let ((), warnings) =
noirc_driver::check_crate(&mut context, root_crate_id, false, false, false, None)?;
noirc_driver::check_crate(&mut context, root_crate_id, false, false, None)?;

assert_eq!(warnings, Vec::new(), "stdlib is producing {} warnings", warnings.len());

Expand Down
255 changes: 8 additions & 247 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,14 @@ use super::dc_mod::collect_defs;
use super::errors::{DefCollectorErrorKind, DuplicateType};
use crate::elaborator::Elaborator;
use crate::graph::CrateId;
use crate::hir::comptime::{Interpreter, InterpreterError};
use crate::hir::comptime::InterpreterError;
use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId};
use crate::hir::resolution::errors::ResolverError;
use crate::hir::resolution::path_resolver;
use crate::hir::type_check::TypeCheckError;
use crate::{ResolvedGeneric, Type};

use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution};
use crate::hir::resolution::{
collect_impls, collect_trait_impls, path_resolver, resolve_free_functions, resolve_globals,
resolve_impls, resolve_structs, resolve_trait_by_path, resolve_trait_impls, resolve_traits,
resolve_type_aliases,
};
use crate::hir::type_check::{
check_trait_impl_method_matches_declaration, type_check_func, TypeCheckError, TypeChecker,
};
use crate::hir::Context;

use crate::macros_api::{MacroError, MacroProcessor};
Expand All @@ -39,16 +33,6 @@ use std::fmt::Write;
use std::path::PathBuf;
use std::vec;

#[derive(Default)]
pub struct ResolvedModule {
pub globals: Vec<(FileId, GlobalId)>,
pub functions: Vec<(FileId, FuncId)>,
pub trait_impl_functions: Vec<(FileId, FuncId)>,
pub debug_comptime_in_file: Option<FileId>,

pub errors: Vec<(CompilationError, FileId)>,
}

/// Stores all of the unresolved functions in a particular file/mod
#[derive(Clone)]
pub struct UnresolvedFunctions {
Expand All @@ -68,35 +52,6 @@ impl UnresolvedFunctions {
pub fn function_ids(&self) -> Vec<FuncId> {
vecmap(&self.functions, |(_, id, _)| *id)
}

pub fn resolve_trait_bounds_trait_ids(
&mut self,
def_maps: &BTreeMap<CrateId, CrateDefMap>,
crate_id: CrateId,
) -> Vec<DefCollectorErrorKind> {
let mut errors = Vec::new();

for (local_id, _, func) in &mut self.functions {
let module = ModuleId { krate: crate_id, local_id: *local_id };

for bound in &mut func.def.where_clause {
match resolve_trait_by_path(def_maps, module, bound.trait_bound.trait_path.clone())
{
Ok((trait_id, warning)) => {
bound.trait_bound.trait_id = Some(trait_id);
if let Some(warning) = warning {
errors.push(DefCollectorErrorKind::PathResolutionError(warning));
}
}
Err(err) => {
errors.push(err);
}
}
}
}

errors
}
}

pub struct UnresolvedStruct {
Expand Down Expand Up @@ -201,13 +156,6 @@ pub enum CompilationError {
DebugComptimeScopeNotFound(Vec<PathBuf>),
}

impl CompilationError {
pub fn is_error(&self) -> bool {
let diagnostic = CustomDiagnostic::from(self);
diagnostic.is_error()
}
}

impl<'a> From<&'a CompilationError> for CustomDiagnostic {
fn from(value: &'a CompilationError) -> Self {
match value {
Expand Down Expand Up @@ -284,7 +232,6 @@ impl DefCollector {
context: &mut Context,
ast: SortedModule,
root_file_id: FileId,
use_legacy: bool,
debug_comptime_in_file: Option<&str>,
macro_processors: &[&dyn MacroProcessor],
) -> Vec<(CompilationError, FileId)> {
Expand All @@ -302,7 +249,6 @@ impl DefCollector {
errors.extend(CrateDefMap::collect_defs(
dep.crate_id,
context,
use_legacy,
debug_comptime_in_file,
macro_processors,
));
Expand Down Expand Up @@ -425,121 +371,19 @@ impl DefCollector {
})
});

if !use_legacy {
let mut more_errors = Elaborator::elaborate(
context,
crate_id,
def_collector.items,
debug_comptime_in_file,
);
errors.append(&mut more_errors);

for macro_processor in macro_processors {
macro_processor.process_typed_ast(&crate_id, context).unwrap_or_else(
|(macro_err, file_id)| {
errors.push((macro_err.into(), file_id));
},
);
}
return errors;
}

let mut resolved_module =
ResolvedModule { errors, debug_comptime_in_file, ..Default::default() };

// We must first resolve and intern the globals before we can resolve any stmts inside each function.
// Each function uses its own resolver with a newly created ScopeForest, and must be resolved again to be within a function's scope
//
// Additionally, we must resolve integer globals before structs since structs may refer to
// the values of integer globals as numeric generics.
let (literal_globals, other_globals) = filter_literal_globals(def_collector.items.globals);

resolved_module.resolve_globals(context, literal_globals, crate_id);

resolved_module.errors.extend(resolve_type_aliases(
context,
def_collector.items.type_aliases,
crate_id,
));

resolved_module.errors.extend(resolve_traits(
context,
def_collector.items.traits,
crate_id,
));

// Must resolve structs before we resolve globals.
resolved_module.errors.extend(resolve_structs(
context,
def_collector.items.types,
crate_id,
));

// Bind trait impls to their trait. Collect trait functions, that have a
// default implementation, which hasn't been overridden.
resolved_module.errors.extend(collect_trait_impls(
context,
crate_id,
&mut def_collector.items.trait_impls,
));

// Before we resolve any function symbols we must go through our impls and
// re-collect the methods within into their proper module. This cannot be
// done before resolution since we need to be able to resolve the type of the
// impl since that determines the module we should collect into.
//
// These are resolved after trait impls so that struct methods are chosen
// over trait methods if there are name conflicts.
resolved_module.errors.extend(collect_impls(context, crate_id, &def_collector.items.impls));

// We must wait to resolve non-integer globals until after we resolve structs since struct
// globals will need to reference the struct type they're initialized to ensure they are valid.
resolved_module.resolve_globals(context, other_globals, crate_id);

// Resolve each function in the crate. This is now possible since imports have been resolved
resolved_module.functions = resolve_free_functions(
&mut context.def_interner,
crate_id,
&context.def_maps,
def_collector.items.functions,
None,
&mut resolved_module.errors,
);

resolved_module.functions.extend(resolve_impls(
&mut context.def_interner,
crate_id,
&context.def_maps,
def_collector.items.impls,
&mut resolved_module.errors,
));

resolved_module.trait_impl_functions = resolve_trait_impls(
context,
def_collector.items.trait_impls,
crate_id,
&mut resolved_module.errors,
);
let mut more_errors =
Elaborator::elaborate(context, crate_id, def_collector.items, debug_comptime_in_file);
errors.append(&mut more_errors);

for macro_processor in macro_processors {
macro_processor.process_typed_ast(&crate_id, context).unwrap_or_else(
|(macro_err, file_id)| {
resolved_module.errors.push((macro_err.into(), file_id));
errors.push((macro_err.into(), file_id));
},
);
}

let cycle_errors = context.def_interner.check_for_dependency_cycles();
let cycles_present = !cycle_errors.is_empty();
resolved_module.errors.extend(cycle_errors);

resolved_module.type_check(context);

if !cycles_present {
resolved_module.evaluate_comptime(&mut context.def_interner, crate_id);
}

resolved_module.errors
errors
}
}

Expand Down Expand Up @@ -615,86 +459,3 @@ pub fn filter_literal_globals(
_ => false,
})
}

impl ResolvedModule {
fn type_check(&mut self, context: &mut Context) {
self.type_check_globals(&mut context.def_interner);
self.type_check_functions(&mut context.def_interner);
self.type_check_trait_impl_function(&mut context.def_interner);
}

fn type_check_globals(&mut self, interner: &mut NodeInterner) {
for (file_id, global_id) in self.globals.iter() {
for error in TypeChecker::check_global(*global_id, interner) {
self.errors.push((error.into(), *file_id));
}
}
}

fn type_check_functions(&mut self, interner: &mut NodeInterner) {
for (file, func) in self.functions.iter() {
for error in type_check_func(interner, *func) {
self.errors.push((error.into(), *file));
}
}
}

fn type_check_trait_impl_function(&mut self, interner: &mut NodeInterner) {
for (file, func) in self.trait_impl_functions.iter() {
for error in check_trait_impl_method_matches_declaration(interner, *func) {
self.errors.push((error.into(), *file));
}
for error in type_check_func(interner, *func) {
self.errors.push((error.into(), *file));
}
}
}

/// Evaluate all `comptime` expressions in this module
fn evaluate_comptime(&mut self, interner: &mut NodeInterner, crate_id: CrateId) {
if self.count_errors() == 0 {
let mut scopes = vec![HashMap::default()];
let mut interpreter_errors = vec![];
let mut interpreter = Interpreter::new(
interner,
&mut scopes,
crate_id,
self.debug_comptime_in_file,
&mut interpreter_errors,
);

for (_file, global) in &self.globals {
if let Err(error) = interpreter.scan_global(*global) {
self.errors.push(error.into_compilation_error_pair());
}
}

for (_file, function) in &self.functions {
// The file returned by the error may be different than the file the
// function is in so only use the error's file id.
if let Err(error) = interpreter.scan_function(*function) {
self.errors.push(error.into_compilation_error_pair());
}
}
self.errors.extend(
interpreter_errors.into_iter().map(InterpreterError::into_compilation_error_pair),
);
}
}

fn resolve_globals(
&mut self,
context: &mut Context,
literal_globals: Vec<UnresolvedGlobal>,
crate_id: CrateId,
) {
let globals = resolve_globals(context, literal_globals, crate_id);
self.globals.extend(globals.globals);
self.errors.extend(globals.errors);
}

/// Counts the number of errors (minus warnings) this program currently has
fn count_errors(&self) -> usize {
self.errors.iter().filter(|(error, _)| error.is_error()).count()
}
}
2 changes: 0 additions & 2 deletions compiler/noirc_frontend/src/hir/def_map/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ impl CrateDefMap {
pub fn collect_defs(
crate_id: CrateId,
context: &mut Context,
use_legacy: bool,
debug_comptime_in_file: Option<&str>,
macro_processors: &[&dyn MacroProcessor],
) -> Vec<(CompilationError, FileId)> {
Expand Down Expand Up @@ -123,7 +122,6 @@ impl CrateDefMap {
context,
ast,
root_file_id,
use_legacy,
debug_comptime_in_file,
macro_processors,
));
Expand Down
Loading

0 comments on commit e30b347

Please sign in to comment.