Skip to content

Commit

Permalink
Refactor compiler error handling (#4878)
Browse files Browse the repository at this point in the history
## Description

Change error handling to use `Handler` instead of the macro based
`CompileResult`.

Fix #2734

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
  • Loading branch information
IGI-111 authored Jul 29, 2023
1 parent 492870b commit f5f8566
Show file tree
Hide file tree
Showing 93 changed files with 5,573 additions and 7,769 deletions.
4 changes: 3 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 8 additions & 4 deletions forc-pkg/src/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use std::{
path::{Path, PathBuf},
sync::Arc,
};
use sway_error::handler::Handler;

use sway_core::{fuel_prelude::fuel_tx, language::parsed::TreeType, parse_tree_type, BuildTarget};
use sway_utils::constants;
Expand Down Expand Up @@ -391,10 +392,13 @@ impl PackageManifestFile {
/// Parse and return the associated project's program type.
pub fn program_type(&self) -> Result<TreeType> {
let entry_string = self.entry_string()?;
let parse_res = parse_tree_type(entry_string);
parse_res
.value
.ok_or_else(|| parsing_failed(&self.project.name, parse_res.errors))
let handler = Handler::default();
let parse_res = parse_tree_type(&handler, entry_string);

parse_res.map_err(|_| {
let (errors, _warnings) = handler.consume();
parsing_failed(&self.project.name, errors)
})
}

/// Given the current directory and expected program type,
Expand Down
90 changes: 53 additions & 37 deletions forc-pkg/src/pkg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ use sway_core::{
semantic_analysis::namespace,
source_map::SourceMap,
transform::AttributeKind,
BuildTarget, CompileResult, Engines, FinalizedEntry,
BuildTarget, Engines, FinalizedEntry,
};
use sway_error::{error::CompileError, warning::CompileWarning};
use sway_error::{error::CompileError, handler::Handler, warning::CompileWarning};
use sway_types::{Ident, Span, Spanned};
use sway_utils::{constants, time_expr, PerformanceData, PerformanceMetric};
use tracing::{info, warn};
Expand Down Expand Up @@ -1658,15 +1658,17 @@ pub fn dependency_namespace(
}
}

namespace.star_import_with_reexports(
let _ = namespace.star_import_with_reexports(
&Handler::default(),
&[CORE, PRELUDE].map(|s| Ident::new_no_span(s.into())),
&[],
engines,
true,
);

if has_std_dep(graph, node) {
namespace.star_import_with_reexports(
let _ = namespace.star_import_with_reexports(
&Handler::default(),
&[STD, PRELUDE].map(|s| Ident::new_no_span(s.into())),
&[],
engines,
Expand Down Expand Up @@ -1761,17 +1763,27 @@ pub fn compile(
sway_build_config(pkg.manifest_file.dir(), &entry_path, pkg.target, profile)?;
let terse_mode = profile.terse;
let reverse_results = profile.reverse_results;
let fail = |warnings, errors| {
print_on_failure(engines.se(), terse_mode, warnings, errors, reverse_results);
let fail = |handler: Handler| {
let (errors, warnings) = handler.consume();
print_on_failure(
engines.se(),
terse_mode,
&warnings,
&errors,
reverse_results,
);
bail!("Failed to compile {}", pkg.name);
};
let source = pkg.manifest_file.entry_string()?;

let handler = Handler::default();

// First, compile to an AST. We'll update the namespace and check for JSON ABI output.
let ast_res = time_expr!(
"compile to ast",
"compile_to_ast",
sway_core::compile_to_ast(
&handler,
engines,
source,
namespace,
Expand All @@ -1782,12 +1794,13 @@ pub fn compile(
Some(sway_build_config.clone()),
metrics
);
let programs = match ast_res.value.as_ref() {
None => return fail(&ast_res.warnings, &ast_res.errors),
Some(programs) => programs,

let programs = match ast_res {
Err(_) => return fail(handler),
Ok(programs) => programs,
};
let typed_program = match programs.typed.as_ref() {
None => return fail(&ast_res.warnings, &ast_res.errors),
None => return fail(handler),
Some(typed_program) => typed_program,
};

Expand All @@ -1800,14 +1813,14 @@ pub fn compile(

let namespace = typed_program.root.namespace.clone().into();

if !ast_res.errors.is_empty() {
return fail(&ast_res.warnings, &ast_res.errors);
if handler.has_error() {
return fail(handler);
}

let asm_res = time_expr!(
"compile ast to asm",
"compile_ast_to_asm",
sway_core::ast_to_asm(engines, &ast_res, &sway_build_config),
sway_core::ast_to_asm(&handler, engines, &programs, &sway_build_config),
Some(sway_build_config.clone()),
metrics
);
Expand All @@ -1834,8 +1847,8 @@ pub fn compile(
BuildTarget::EVM => {
// Merge the ABI output of ASM gen with ABI gen to handle internal constructors
// generated by the ASM backend.
let mut ops = match &asm_res.value {
Some(ref asm) => match &asm.0.abi {
let mut ops = match &asm_res {
Ok(ref asm) => match &asm.0.abi {
Some(ProgramABI::Evm(ops)) => ops.clone(),
_ => vec![],
},
Expand All @@ -1859,37 +1872,37 @@ pub fn compile(
};

let entries = asm_res
.value
.as_ref()
.map(|asm| asm.0.entries.clone())
.unwrap_or_default();
let entries = entries
.iter()
.map(|finalized_entry| PkgEntry::from_finalized_entry(finalized_entry, engines))
.collect::<anyhow::Result<_>>()?;

let asm = match asm_res {
Err(_) => return fail(handler),
Ok(asm) => asm,
};

let bc_res = time_expr!(
"compile asm to bytecode",
"compile_asm_to_bytecode",
sway_core::asm_to_bytecode(asm_res, source_map, engines.se()),
sway_core::asm_to_bytecode(&handler, asm, source_map, engines.se()),
Some(sway_build_config),
metrics
);

let errored =
!bc_res.errors.is_empty() || (!bc_res.warnings.is_empty() && profile.error_on_warnings);
let errored = handler.has_error() || (handler.has_warning() && profile.error_on_warnings);

let compiled = match bc_res.value {
Some(compiled) if !errored => compiled,
_ => return fail(&bc_res.warnings, &bc_res.errors),
let compiled = match bc_res {
Ok(compiled) if !errored => compiled,
_ => return fail(handler),
};

print_warnings(
engines.se(),
terse_mode,
&pkg.name,
&bc_res.warnings,
&tree_type,
);
let (_, warnings) = handler.consume();

print_warnings(engines.se(), terse_mode, &pkg.name, &warnings, &tree_type);

// TODO: This should probably be in `fuel_abi_json::generate_json_abi_program`?
// If ABI requires knowing config offsets, they should be inputs to ABI gen.
Expand Down Expand Up @@ -1918,7 +1931,7 @@ pub fn compile(
tree_type,
bytecode,
namespace,
warnings: bc_res.warnings,
warnings,
metrics,
};
Ok(compiled_package)
Expand Down Expand Up @@ -2582,7 +2595,7 @@ pub fn check(
terse_mode: bool,
include_tests: bool,
engines: &Engines,
) -> anyhow::Result<Vec<CompileResult<Programs>>> {
) -> anyhow::Result<Vec<(Option<Programs>, Handler)>> {
let mut lib_namespace_map = Default::default();
let mut source_map = SourceMap::new();
// During `check`, we don't compile so this stays empty.
Expand Down Expand Up @@ -2629,19 +2642,22 @@ pub fn check(
.include_tests(include_tests);

let mut metrics = PerformanceData::default();
let input = manifest.entry_string()?;
let handler = Handler::default();
let programs_res = sway_core::compile_to_ast(
&handler,
engines,
manifest.entry_string()?,
input,
dep_namespace,
Some(&build_config),
&pkg.name,
&mut metrics,
);

let programs = match programs_res.value.as_ref() {
Some(programs) => programs,
let programs = match programs_res.as_ref() {
Ok(programs) => programs,
_ => {
results.push(programs_res);
results.push((programs_res.ok(), handler));
return Ok(results);
}
};
Expand All @@ -2666,12 +2682,12 @@ pub fn check(
source_map.insert_dependency(manifest.dir());
}
None => {
results.push(programs_res);
results.push((programs_res.ok(), handler));
return Ok(results);
}
}

results.push(programs_res)
results.push((programs_res.ok(), handler))
}

if results.is_empty() {
Expand Down
8 changes: 4 additions & 4 deletions forc-plugins/forc-doc/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,12 @@ pub fn main() -> Result<()> {
let graph = plan.graph();
let manifest_map = plan.manifest_map();

for (node, compile_result) in order.iter().zip(compile_results) {
for (node, (compile_result, _handler)) in order.iter().zip(compile_results) {
let id = &graph[*node].id();

if let Some(pkg_manifest_file) = manifest_map.get(id) {
let manifest_file = ManifestFile::from_dir(pkg_manifest_file.path())?;
let ty_program = match compile_result.value.and_then(|programs| programs.typed) {
let ty_program = match compile_result.and_then(|programs| programs.typed) {
Some(ty_program) => ty_program,
_ => bail!(
"documentation could not be built from manifest located at '{}'",
Expand All @@ -129,8 +129,8 @@ pub fn main() -> Result<()> {
} else {
let ty_program = match compile_results
.pop()
.and_then(|compilation| compilation.value)
.and_then(|programs| programs.typed)
.and_then(|(programs, _handler)| programs)
.and_then(|p| p.typed)
{
Some(ty_program) => ty_program,
_ => bail!(
Expand Down
1 change: 1 addition & 0 deletions forc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ hex = "0.4.3"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0.73"
sway-core = { version = "0.42.1", path = "../sway-core" }
sway-error = { version = "0.42.1", path = "../sway-error" }
sway-types = { version = "0.42.1", path = "../sway-types" }
sway-utils = { version = "0.42.1", path = "../sway-utils" }
term-table = "1.3"
Expand Down
2 changes: 1 addition & 1 deletion forc/src/cli/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub struct Command {
pub(crate) fn exec(command: Command) -> ForcResult<()> {
let engines = Engines::default();
let res = forc_check::check(command, &engines)?;
if !res.is_ok() {
if res.0.is_none() {
forc_result_bail!("unable to type check");
}
Ok(())
Expand Down
15 changes: 8 additions & 7 deletions forc/src/ops/forc_check.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use crate::cli::CheckCommand;
use anyhow::Result;
use forc_pkg::{self as pkg};
use forc_pkg as pkg;
use pkg::manifest::ManifestFile;
use std::path::PathBuf;
use sway_core::{language::ty, CompileResult, Engines};
use sway_core::{language::ty, Engines};
use sway_error::handler::Handler;

pub fn check(command: CheckCommand, engines: &Engines) -> Result<CompileResult<ty::TyProgram>> {
pub fn check(command: CheckCommand, engines: &Engines) -> Result<(Option<ty::TyProgram>, Handler)> {
let CheckCommand {
build_target,
path,
Expand Down Expand Up @@ -34,9 +35,9 @@ pub fn check(command: CheckCommand, engines: &Engines) -> Result<CompileResult<t
let tests_enabled = !disable_tests;

let mut v = pkg::check(&plan, build_target, terse_mode, tests_enabled, engines)?;
let res = v
let (res, handler) = v
.pop()
.expect("there is guaranteed to be at least one elem in the vector")
.flat_map(|programs| CompileResult::new(programs.typed, vec![], vec![]));
Ok(res)
.expect("there is guaranteed to be at least one elem in the vector");
let res = res.and_then(|programs| programs.typed);
Ok((res, handler))
}
1 change: 0 additions & 1 deletion sway-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ rustc-hash = "1.1.0"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0.91"
sha2 = "0.9"
smallvec = "1.7"
strum = { version = "0.24.1", features = ["derive"] }
sway-ast = { version = "0.42.1", path = "../sway-ast" }
sway-error = { version = "0.42.1", path = "../sway-error" }
Expand Down
9 changes: 7 additions & 2 deletions sway-core/src/asm_generation/asm_builder.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use sway_error::handler::{ErrorEmitted, Handler};
use sway_ir::Function;

use crate::{asm_lang::Label, CompileResult};
use crate::asm_lang::Label;

use super::{
evm::EvmAsmBuilderResult, fuel::fuel_asm_builder::FuelAsmBuilderResult,
Expand All @@ -15,6 +16,10 @@ pub enum AsmBuilderResult {

pub trait AsmBuilder {
fn func_to_labels(&mut self, func: &Function) -> (Label, Label);
fn compile_function(&mut self, function: Function) -> CompileResult<()>;
fn compile_function(
&mut self,
handler: &Handler,
function: Function,
) -> Result<(), ErrorEmitted>;
fn finalize(&self) -> AsmBuilderResult;
}
Loading

0 comments on commit f5f8566

Please sign in to comment.