From f4b155f33727f8d2f983d7c334ba901b8633c8a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Matos?= Date: Mon, 7 Aug 2023 23:40:09 +0100 Subject: [PATCH] Split processing of impl methods in two phases (part 1). (#4890) ## Description This PR mainly refactors existing code around type checking and processing of functions. [Split type checking and namespace insertion for parameters and type parameters](https://github.com/FuelLabs/sway/pull/4890/commits/327deace24af3c354cfbd25b01d0cf2e597108c6) [Split processing of functions in two phases.](https://github.com/FuelLabs/sway/pull/4890/commits/e3cf148f3f1f017a67f6f03dda0ec11e42853986) I've split this up from the rest of the PR for fixing impl methods calling to make it easier to review (also made it easier for me to find and fix some regressions). I also threw in another round of clippy fixes that my Rust toolchain was complaining about. ## 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. --- forc-pkg/src/pkg.rs | 2 +- forc-plugins/forc-tx/src/lib.rs | 4 +- sway-core/src/decl_engine/template.rs | 2 +- sway-core/src/language/ty/code_block.rs | 2 +- sway-core/src/lib.rs | 12 +- .../ast_node/declaration/enum.rs | 8 +- .../ast_node/declaration/function.rs | 142 +++++++++++++----- .../function/function_parameter.rs | 50 +++--- .../ast_node/declaration/impl_trait.rs | 16 +- .../ast_node/declaration/struct.rs | 8 +- .../ast_node/declaration/trait.rs | 8 +- .../to_parsed_lang/convert_parse_tree.rs | 4 +- .../ast_elements/type_parameter.rs | 57 ++++--- sway-ir/src/optimize/misc_demotion.rs | 7 +- test/src/e2e_vm_tests/harness.rs | 6 +- 15 files changed, 213 insertions(+), 115 deletions(-) diff --git a/forc-pkg/src/pkg.rs b/forc-pkg/src/pkg.rs index b403c1fa83..acf705518c 100644 --- a/forc-pkg/src/pkg.rs +++ b/forc-pkg/src/pkg.rs @@ -1872,7 +1872,7 @@ pub fn compile( metrics ); - ops.extend(abi.into_iter()); + ops.extend(abi); ProgramABI::Evm(ops) } diff --git a/forc-plugins/forc-tx/src/lib.rs b/forc-plugins/forc-tx/src/lib.rs index a29772c51f..dded3cd78c 100644 --- a/forc-plugins/forc-tx/src/lib.rs +++ b/forc-plugins/forc-tx/src/lib.rs @@ -397,7 +397,7 @@ pub enum ConvertInputError { WitnessPredicateMismatch, } -const EXAMPLES: &str = r#"EXAMPLES: +const EXAMPLES: &str = r"EXAMPLES: # An example constructing a `create` transaction. forc tx create \ --bytecode ./my-contract/out/debug/my-contract.bin \ @@ -453,7 +453,7 @@ const EXAMPLES: &str = r#"EXAMPLES: output contract-created \ --contract-id 0xCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC \ --state-root 0x0000000000000000000000000000000000000000000000000000000000000000 -"#; +"; impl ParseError { /// Print the error with clap's fancy formatting. diff --git a/sway-core/src/decl_engine/template.rs b/sway-core/src/decl_engine/template.rs index 597a94d057..892bfd0c5e 100644 --- a/sway-core/src/decl_engine/template.rs +++ b/sway-core/src/decl_engine/template.rs @@ -7,7 +7,7 @@ /// [SubstList](crate::type_system::SubstList) contained in this field is simply /// a template for usages of the declaration declared in that particular /// [TyDecl](crate::language::ty::TyDecl) node. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Default)] pub struct Template(T) where T: Clone; diff --git a/sway-core/src/language/ty/code_block.rs b/sway-core/src/language/ty/code_block.rs index 700f3ee056..6ea0cd422c 100644 --- a/sway-core/src/language/ty/code_block.rs +++ b/sway-core/src/language/ty/code_block.rs @@ -7,7 +7,7 @@ use crate::{ type_system::*, types::DeterministicallyAborts, }; -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Default)] pub struct TyCodeBlock { pub contents: Vec, } diff --git a/sway-core/src/lib.rs b/sway-core/src/lib.rs index 0ad304c66c..7d61903a85 100644 --- a/sway-core/src/lib.rs +++ b/sway-core/src/lib.rs @@ -738,14 +738,10 @@ fn module_dead_code_analysis<'eng: 'cfg, 'cfg>( tree_type: &parsed::TreeType, graph: &mut ControlFlowGraph<'cfg>, ) -> Result<(), ErrorEmitted> { - module - .submodules - .iter() - .fold(Ok(()), |res, (_, submodule)| { - let tree_type = parsed::TreeType::Library; - res?; - module_dead_code_analysis(handler, engines, &submodule.module, &tree_type, graph) - })?; + module.submodules.iter().try_fold((), |_, (_, submodule)| { + let tree_type = parsed::TreeType::Library; + module_dead_code_analysis(handler, engines, &submodule.module, &tree_type, graph) + })?; let res = { ControlFlowGraph::append_module_to_dead_code_graph( engines, diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs b/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs index c42d6eba69..76f404f738 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/enum.rs @@ -26,11 +26,15 @@ impl ty::TyEnumDecl { let mut decl_namespace = ctx.namespace.clone(); let mut ctx = ctx.scoped(&mut decl_namespace); - // Type check the type parameters. This will also insert them into the - // current namespace. + // Type check the type parameters. let new_type_parameters = TypeParameter::type_check_type_params(handler, ctx.by_ref(), type_parameters)?; + // Insert them into the current namespace. + for p in &new_type_parameters { + p.insert_into_namespace(handler, ctx.by_ref())?; + } + // type check the variants let mut variants_buf = vec![]; for variant in variants { diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/function.rs b/sway-core/src/semantic_analysis/ast_node/declaration/function.rs index de3638c99a..1bb653b13e 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/function.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/function.rs @@ -8,7 +8,11 @@ use sway_error::{ }; use crate::{ - language::{parsed::*, ty, Visibility}, + language::{ + parsed::*, + ty::{self, TyCodeBlock}, + Visibility, + }, semantic_analysis::*, type_system::*, }; @@ -21,10 +25,27 @@ impl ty::TyFunctionDecl { fn_decl: FunctionDeclaration, is_method: bool, is_in_impl_self: bool, + ) -> Result { + let mut ty_fn_decl = Self::type_check_signature( + handler, + ctx.by_ref(), + fn_decl.clone(), + is_method, + is_in_impl_self, + )?; + Self::type_check_body(handler, ctx, fn_decl, &mut ty_fn_decl) + } + + pub fn type_check_signature( + handler: &Handler, + mut ctx: TypeCheckContext, + fn_decl: FunctionDeclaration, + is_method: bool, + is_in_impl_self: bool, ) -> Result { let FunctionDeclaration { name, - body, + body: _, parameters, span, attributes, @@ -68,16 +89,25 @@ impl ty::TyFunctionDecl { let new_type_parameters = TypeParameter::type_check_type_params(handler, ctx.by_ref(), type_parameters)?; + // Insert them into the current namespace. + for p in &new_type_parameters { + p.insert_into_namespace(handler, ctx.by_ref())?; + } + // type check the function parameters, which will also insert them into the namespace let mut new_parameters = vec![]; handler.scope(|handler| { for parameter in parameters.into_iter() { - new_parameters.push( - match ty::TyFunctionParameter::type_check(handler, ctx.by_ref(), parameter) { - Ok(val) => val, - Err(_) => continue, - }, - ); + new_parameters.push({ + let param = + match ty::TyFunctionParameter::type_check(handler, ctx.by_ref(), parameter) + { + Ok(val) => val, + Err(_) => continue, + }; + param.insert_into_namespace(handler, ctx.by_ref()); + param + }); } Ok(()) })?; @@ -93,6 +123,72 @@ impl ty::TyFunctionDecl { ) .unwrap_or_else(|err| type_engine.insert(engines, TypeInfo::ErrorRecovery(err))); + let (visibility, is_contract_call) = if is_method { + if is_in_impl_self { + (visibility, false) + } else { + (Visibility::Public, false) + } + } else { + (visibility, matches!(ctx.abi_mode(), AbiMode::ImplAbiFn(..))) + }; + + let function_decl = ty::TyFunctionDecl { + name, + body: TyCodeBlock::default(), + parameters: new_parameters, + implementing_type: None, + span, + attributes, + return_type, + type_parameters: new_type_parameters, + visibility, + is_contract_call, + purity, + where_clause, + }; + + Ok(function_decl) + } + + pub fn type_check_body( + handler: &Handler, + mut ctx: TypeCheckContext, + fn_decl: FunctionDeclaration, + ty_fn_decl: &mut Self, + ) -> Result { + let FunctionDeclaration { body, .. } = fn_decl; + + let ty::TyFunctionDecl { + parameters, + purity, + return_type, + type_parameters, + .. + } = ty_fn_decl; + + let type_engine = ctx.engines.te(); + let engines = ctx.engines(); + + // create a namespace for the function + let mut fn_namespace = ctx.namespace.clone(); + let mut ctx = ctx + .by_ref() + .scoped(&mut fn_namespace) + .with_purity(*purity) + .with_const_shadowing_mode(ConstShadowingMode::Sequential) + .disallow_functions(); + + // Insert the previously type checked type parameters into the current namespace. + for p in type_parameters { + p.insert_into_namespace(handler, ctx.by_ref())?; + } + + // Insert the previously type checked function parameters into the namespace. + for p in parameters { + p.insert_into_namespace(handler, ctx.by_ref()); + } + // type check the function body // // If there are no implicit block returns, then we do not want to type check them, so we @@ -100,7 +196,7 @@ impl ty::TyFunctionDecl { let (body, _implicit_block_return) = { let ctx = ctx .by_ref() - .with_purity(purity) + .with_purity(*purity) .with_help_text("Function body's return type does not match up with its return type annotation.") .with_type_annotation(return_type.type_id); ty::TyCodeBlock::type_check(handler, ctx, body).unwrap_or_else(|err| { @@ -125,16 +221,6 @@ impl ty::TyFunctionDecl { return_type.type_id, )?; - let (visibility, is_contract_call) = if is_method { - if is_in_impl_self { - (visibility, false) - } else { - (Visibility::Public, false) - } - } else { - (visibility, matches!(ctx.abi_mode(), AbiMode::ImplAbiFn(..))) - }; - return_type.type_id.check_type_parameter_bounds( handler, &ctx, @@ -142,22 +228,8 @@ impl ty::TyFunctionDecl { vec![], )?; - let function_decl = ty::TyFunctionDecl { - name, - body, - parameters: new_parameters, - implementing_type: None, - span, - attributes, - return_type, - type_parameters: new_type_parameters, - visibility, - is_contract_call, - purity, - where_clause, - }; - - Ok(function_decl) + ty_fn_decl.body = body; + Ok(ty_fn_decl.clone()) } } diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/function/function_parameter.rs b/sway-core/src/semantic_analysis/ast_node/declaration/function/function_parameter.rs index 35b5e56d44..5a6f3c88af 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/function/function_parameter.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/function/function_parameter.rs @@ -62,8 +62,6 @@ impl ty::TyFunctionParameter { type_argument, }; - insert_into_namespace(handler, ctx, &typed_parameter); - Ok(typed_parameter) } @@ -103,31 +101,27 @@ impl ty::TyFunctionParameter { Ok(typed_parameter) } -} -fn insert_into_namespace( - handler: &Handler, - ctx: TypeCheckContext, - typed_parameter: &ty::TyFunctionParameter, -) { - let const_shadowing_mode = ctx.const_shadowing_mode(); - let _ = ctx.namespace.insert_symbol( - handler, - typed_parameter.name.clone(), - ty::TyDecl::VariableDecl(Box::new(ty::TyVariableDecl { - name: typed_parameter.name.clone(), - body: ty::TyExpression { - expression: ty::TyExpressionVariant::FunctionParameter, - return_type: typed_parameter.type_argument.type_id, - span: typed_parameter.name.span(), - }, - mutability: ty::VariableMutability::new_from_ref_mut( - typed_parameter.is_reference, - typed_parameter.is_mutable, - ), - return_type: typed_parameter.type_argument.type_id, - type_ascription: typed_parameter.type_argument.clone(), - })), - const_shadowing_mode, - ); + pub fn insert_into_namespace(&self, handler: &Handler, ctx: TypeCheckContext) { + let const_shadowing_mode = ctx.const_shadowing_mode(); + let _ = ctx.namespace.insert_symbol( + handler, + self.name.clone(), + ty::TyDecl::VariableDecl(Box::new(ty::TyVariableDecl { + name: self.name.clone(), + body: ty::TyExpression { + expression: ty::TyExpressionVariant::FunctionParameter, + return_type: self.type_argument.type_id, + span: self.name.span(), + }, + mutability: ty::VariableMutability::new_from_ref_mut( + self.is_reference, + self.is_mutable, + ), + return_type: self.type_argument.type_id, + type_ascription: self.type_argument.clone(), + })), + const_shadowing_mode, + ); + } } diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs b/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs index 1c3ebbd4ae..6df3d6267a 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/impl_trait.rs @@ -45,11 +45,15 @@ impl ty::TyImplTrait { .with_const_shadowing_mode(ConstShadowingMode::ItemStyle) .allow_functions(); - // Type check the type parameters. This will also insert them into the - // current namespace. + // Type check the type parameters. let new_impl_type_parameters = TypeParameter::type_check_type_params(handler, ctx.by_ref(), impl_type_parameters)?; + // Insert them into the current namespace. + for p in &new_impl_type_parameters { + p.insert_into_namespace(handler, ctx.by_ref())?; + } + // resolve the types of the trait type arguments for type_arg in trait_type_arguments.iter_mut() { type_arg.type_id = @@ -240,11 +244,15 @@ impl ty::TyImplTrait { is_absolute: false, }; - // Type check the type parameters. This will also insert them into the - // current namespace. + // Type check the type parameters. let new_impl_type_parameters = TypeParameter::type_check_type_params(handler, ctx.by_ref(), impl_type_parameters)?; + // Insert them into the current namespace. + for p in &new_impl_type_parameters { + p.insert_into_namespace(handler, ctx.by_ref())?; + } + // type check the type that we are implementing for implementing_for.type_id = ctx.resolve_type_without_self( handler, diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs b/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs index e7d418d1e6..b37749c2ae 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/struct.rs @@ -26,11 +26,15 @@ impl ty::TyStructDecl { let mut decl_namespace = ctx.namespace.clone(); let mut ctx = ctx.scoped(&mut decl_namespace); - // Type check the type parameters. This will also insert them into the - // current namespace. + // Type check the type parameters. let new_type_parameters = TypeParameter::type_check_type_params(handler, ctx.by_ref(), type_parameters)?; + // Insert them into the current namespace. + for p in &new_type_parameters { + p.insert_into_namespace(handler, ctx.by_ref())?; + } + // type check the fields let mut new_fields = vec![]; for field in fields.into_iter() { diff --git a/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs b/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs index c1ae7ba322..a5499e1df7 100644 --- a/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs +++ b/sway-core/src/semantic_analysis/ast_node/declaration/trait.rs @@ -54,11 +54,15 @@ impl ty::TyTraitDecl { let mut trait_namespace = ctx.namespace.clone(); let mut ctx = ctx.scoped(&mut trait_namespace).with_self_type(self_type); - // Type check the type parameters. This will also insert them into the - // current namespace. + // Type check the type parameters. let new_type_parameters = TypeParameter::type_check_type_params(handler, ctx.by_ref(), type_parameters)?; + // Insert them into the current namespace. + for p in &new_type_parameters { + p.insert_into_namespace(handler, ctx.by_ref())?; + } + // Recursively make the interface surfaces and methods of the // supertraits available to this trait. insert_supertraits_into_namespace( diff --git a/sway-core/src/transform/to_parsed_lang/convert_parse_tree.rs b/sway-core/src/transform/to_parsed_lang/convert_parse_tree.rs index 0a95f58855..e7be318f2c 100644 --- a/sway-core/src/transform/to_parsed_lang/convert_parse_tree.rs +++ b/sway-core/src/transform/to_parsed_lang/convert_parse_tree.rs @@ -2361,13 +2361,13 @@ fn statement_to_ast_nodes( } Statement::Item(item) => { let nodes = item_to_ast_nodes(context, handler, engines, item, false, None)?; - nodes.iter().fold(Ok(()), |res, node| { + nodes.iter().try_fold((), |res, node| { if ast_node_is_test_fn(node) { let span = node.span.clone(); let error = ConvertParseTreeError::TestFnOnlyAllowedAtModuleLevel { span }; Err(handler.emit_err(error.into())) } else { - res + Ok(res) } })?; nodes diff --git a/sway-core/src/type_system/ast_elements/type_parameter.rs b/sway-core/src/type_system/ast_elements/type_parameter.rs index ac35fcf0e2..2befd896f6 100644 --- a/sway-core/src/type_system/ast_elements/type_parameter.rs +++ b/sway-core/src/type_system/ast_elements/type_parameter.rs @@ -188,16 +188,6 @@ impl TypeParameter { }, ); - // Insert the trait constraints into the namespace. - for trait_constraint in trait_constraints.iter() { - TraitConstraint::insert_into_namespace( - handler, - ctx.by_ref(), - type_id, - trait_constraint, - )?; - } - // When type parameter is from parent then it was already inserted. // Instead of inserting a type with same name we unify them. if is_from_parent { @@ -225,16 +215,6 @@ impl TypeParameter { } } } - } else { - // Insert the type parameter into the namespace as a dummy type - // declaration. - let type_parameter_decl = - ty::TyDecl::GenericTypeForFunctionScope(ty::GenericTypeForFunctionScope { - name: name_ident.clone(), - type_id, - }); - ctx.insert_symbol(handler, name_ident.clone(), type_parameter_decl) - .ok(); } let type_parameter = TypeParameter { @@ -248,6 +228,43 @@ impl TypeParameter { Ok(type_parameter) } + pub fn insert_into_namespace( + &self, + handler: &Handler, + mut ctx: TypeCheckContext, + ) -> Result<(), ErrorEmitted> { + let Self { + is_from_parent, + name_ident, + type_id, + .. + } = self; + + // Insert the trait constraints into the namespace. + for trait_constraint in self.trait_constraints.iter() { + TraitConstraint::insert_into_namespace( + handler, + ctx.by_ref(), + *type_id, + trait_constraint, + )?; + } + + if !is_from_parent { + // Insert the type parameter into the namespace as a dummy type + // declaration. + let type_parameter_decl = + ty::TyDecl::GenericTypeForFunctionScope(ty::GenericTypeForFunctionScope { + name: name_ident.clone(), + type_id: *type_id, + }); + ctx.insert_symbol(handler, name_ident.clone(), type_parameter_decl) + .ok(); + } + + Ok(()) + } + /// Creates a [DeclMapping] from a list of [TypeParameter]s. pub(crate) fn gather_decl_mapping_from_trait_constraints( handler: &Handler, diff --git a/sway-ir/src/optimize/misc_demotion.rs b/sway-ir/src/optimize/misc_demotion.rs index 9a31191d25..2ae2143b94 100644 --- a/sway-ir/src/optimize/misc_demotion.rs +++ b/sway-ir/src/optimize/misc_demotion.rs @@ -367,9 +367,10 @@ fn wide_binary_op_demotion(context: &mut Context, function: Function) -> Result< let Instruction::BinaryOp { op, arg1, arg2 } = binary_op_instr_val .get_instruction(context) .cloned() - .unwrap() else { - continue; - }; + .unwrap() + else { + continue; + }; let binary_op_metadata = binary_op_instr_val.get_metadata(context); diff --git a/test/src/e2e_vm_tests/harness.rs b/test/src/e2e_vm_tests/harness.rs index 05999d9573..2bf3c2f69e 100644 --- a/test/src/e2e_vm_tests/harness.rs +++ b/test/src/e2e_vm_tests/harness.rs @@ -30,13 +30,11 @@ where let mut output = String::new(); // Capture both stdout and stderr to buffers, run the code and save to a string. - let buf_stdout = Some(gag::BufferRedirect::stdout().unwrap()); - let buf_stderr = Some(gag::BufferRedirect::stderr().unwrap()); + let mut buf_stdout = gag::BufferRedirect::stdout().unwrap(); + let mut buf_stderr = gag::BufferRedirect::stderr().unwrap(); let result = func().await; - let mut buf_stdout = buf_stdout.unwrap(); - let mut buf_stderr = buf_stderr.unwrap(); buf_stdout.read_to_string(&mut output).unwrap(); buf_stderr.read_to_string(&mut output).unwrap(); drop(buf_stdout);