From 573a9ff7570f2baad774a4c05e3af9afb7808daa Mon Sep 17 00:00:00 2001 From: Daniel Frederico Lins Leite Date: Mon, 20 May 2024 13:31:55 +0100 Subject: [PATCH] fix dca for encoding v1 autogenerated code (#6006) ## Description This PR is part of https://github.com/FuelLabs/sway/issues/5727 and fixes a problem with AbiEncode/AbiDecode code generation. The problem lies in the fact that to encode/decode structs we need to "touch" all fields. Which makes all fields to pass DCA. Now, when a `StructFieldAccess` lives inside an autogenerated source, the "code flow graph" will be built as usual, but no edge will be created to the struct field, nor to the field owner struct declaration. ## 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] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [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. --------- Co-authored-by: Joshua Batty --- .../dead_code_analysis.rs | 18 +++++++++++++----- sway-core/src/type_system/unify/unifier.rs | 1 - sway-ir/src/verify.rs | 12 ++---------- sway-types/src/source_engine.rs | 8 ++++++++ test/src/e2e_vm_tests/harness.rs | 2 +- .../should_pass/dca/unused_fields/src/main.sw | 1 - .../should_pass/dca/unused_fields/test.toml | 5 ++++- .../return_in_strange_positions/test.toml | 8 ++++++++ test/src/sdk-harness/Forc.toml | 2 -- 9 files changed, 36 insertions(+), 21 deletions(-) diff --git a/sway-core/src/control_flow_analysis/dead_code_analysis.rs b/sway-core/src/control_flow_analysis/dead_code_analysis.rs index 74c10d10e63..15a2b7fa69f 100644 --- a/sway-core/src/control_flow_analysis/dead_code_analysis.rs +++ b/sway-core/src/control_flow_analysis/dead_code_analysis.rs @@ -1643,13 +1643,21 @@ fn connect_expression<'eng: 'cfg, 'cfg>( for leaf in leaves { graph.add_edge(*leaf, this_ix, "".into()); } - graph.add_edge(this_ix, field_ix, "".into()); - if let Some(struct_node_ix) = graph - .namespace - .find_struct_decl(resolved_type_of_parent.suffix.as_str()) + // autogenerated code should not increase usage of a struct field + if !engines + .se() + .is_span_in_autogenerated(&expression_span) + .unwrap_or(false) { - graph.add_edge(this_ix, *struct_node_ix, "".into()); + graph.add_edge(this_ix, field_ix, "".into()); + + if let Some(struct_node_ix) = graph + .namespace + .find_struct_decl(resolved_type_of_parent.suffix.as_str()) + { + graph.add_edge(this_ix, *struct_node_ix, "".into()); + } } Ok(vec![this_ix]) diff --git a/sway-core/src/type_system/unify/unifier.rs b/sway-core/src/type_system/unify/unifier.rs index 164f9a5f40d..0efc7883153 100644 --- a/sway-core/src/type_system/unify/unifier.rs +++ b/sway-core/src/type_system/unify/unifier.rs @@ -428,7 +428,6 @@ impl<'a> Unifier<'a> { self.unify(handler, rtp.type_id, etp.type_id, span); }); } else { - dbg!(rn == en, rvs.len() == evs.len(), rtps.len() == etps.len()); let internal = format!("[{received:?}] versus [{expected:?}]"); let (received, expected) = self.assign_args(received, expected); handler.emit_err( diff --git a/sway-ir/src/verify.rs b/sway-ir/src/verify.rs index 87dcd19e13b..9e6a925aaf1 100644 --- a/sway-ir/src/verify.rs +++ b/sway-ir/src/verify.rs @@ -816,16 +816,8 @@ impl<'a, 'eng> InstructionVerifier<'a, 'eng> { fn verify_load(&self, src_val: &Value) -> Result<(), IrError> { // Just confirm `src_val` is a pointer. - let r = self - .get_ptr_type(src_val, IrError::VerifyLoadFromNonPointer) - .map(|_| ()); - - if r.is_err() { - let meta = src_val.get_metadata(self.context).unwrap(); - dbg!(&self.context.metadata[meta.0], &r); - } - - r + self.get_ptr_type(src_val, IrError::VerifyLoadFromNonPointer) + .map(|_| ()) } fn verify_log(&self, log_val: &Value, log_ty: &Type, log_id: &Value) -> Result<(), IrError> { diff --git a/sway-types/src/source_engine.rs b/sway-types/src/source_engine.rs index 2909322c912..117a04bfc56 100644 --- a/sway-types/src/source_engine.rs +++ b/sway-types/src/source_engine.rs @@ -40,6 +40,14 @@ impl Clone for SourceEngine { impl SourceEngine { const AUTOGENERATED_PATH: &'static str = ""; + pub fn is_span_in_autogenerated(&self, span: &crate::Span) -> Option { + span.source_id().map(|s| self.is_source_id_autogenerated(s)) + } + + pub fn is_source_id_autogenerated(&self, source_id: &SourceId) -> bool { + self.get_path(source_id).starts_with("") + } + /// This function retrieves an integer-based source ID for a provided path buffer. /// If an ID already exists for the given path, the function will return that /// existing ID. If not, a new ID will be created. diff --git a/test/src/e2e_vm_tests/harness.rs b/test/src/e2e_vm_tests/harness.rs index 57d7fea2822..1655c2acce4 100644 --- a/test/src/e2e_vm_tests/harness.rs +++ b/test/src/e2e_vm_tests/harness.rs @@ -82,7 +82,7 @@ pub(crate) async fn deploy_contract(file_name: &str, run_config: &RunConfig) -> true => BuildProfile::RELEASE.to_string(), false => BuildProfile::DEBUG.to_string(), }, - no_encoding_v1: !dbg!(run_config.experimental.new_encoding), + no_encoding_v1: !run_config.experimental.new_encoding, ..Default::default() }) .await diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/main.sw b/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/main.sw index 6e5ff7fdc12..d024699ad25 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/main.sw +++ b/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/src/main.sw @@ -3,7 +3,6 @@ script; mod utils; use utils::Foo; - struct Bar { value: u64 } diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/test.toml index 555fafa187b..67f8fc2b8a7 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_pass/dca/unused_fields/test.toml @@ -1,3 +1,6 @@ category = "compile" +expected_warnings = 1 -expected_warnings = 0 +# check: $()struct Bar { +# nextln: $()value: u64 +# nextln: $()This struct field is never accessed. diff --git a/test/src/e2e_vm_tests/test_programs/should_pass/return_in_strange_positions/test.toml b/test/src/e2e_vm_tests/test_programs/should_pass/return_in_strange_positions/test.toml index 8d82a0d72c4..dcf5696ac12 100644 --- a/test/src/e2e_vm_tests/test_programs/should_pass/return_in_strange_positions/test.toml +++ b/test/src/e2e_vm_tests/test_programs/should_pass/return_in_strange_positions/test.toml @@ -4,6 +4,14 @@ expected_result_new_encoding = { action = "return_data", value = "00000000000020 validate_abi = true expected_warnings = 26 +#check: $()pub struct S +#nextln: $()This struct field is never accessed. +#nextln: $()pub enum Enum + +#check: $()pub struct S +#nextln: $()This struct field is never accessed. +#nextln: $()pub enum Enum + #check: $()pub enum Enum #nextln: $()This enum is never used. diff --git a/test/src/sdk-harness/Forc.toml b/test/src/sdk-harness/Forc.toml index 4e4b54ecb9d..1346c99b5f1 100644 --- a/test/src/sdk-harness/Forc.toml +++ b/test/src/sdk-harness/Forc.toml @@ -5,8 +5,6 @@ members = [ "test_projects/asset_ops", "test_projects/block", "test_projects/call_frames", - # TODO Uncomment these when SDK supports encoding V1 for configurables - # https://github.com/FuelLabs/sway/issues/5727 "test_projects/configurables_in_contract", "test_projects/configurables_in_script", "test_projects/context",