From 8b8dc30ca84ca59f75835ff2409baa7f01e2673d Mon Sep 17 00:00:00 2001 From: Sean Young Date: Mon, 19 Jun 2023 16:12:31 +0100 Subject: [PATCH] Implicit accessor function should return struct members, not struct If the acessor function returns a single struct, then the members should be returned. If any of those members are structs, then those members will remain structs. contract C { struct S { int a; bool b; } S public s; function foo() public { (int a, bool b) = this.s(); } } Also, the accesor function should be declared `external` and therefore not accessible as an internal function call. Signed-off-by: Sean Young --- src/abi/tests.rs | 17 +- src/sema/variables.rs | 155 ++++++++++++++++-- .../solana/accessor/accessor_is_external.sol | 10 ++ tests/evm.rs | 2 +- tests/solana_tests/accessor.rs | 41 +++++ tests/substrate_tests/inheritance.rs | 4 +- 6 files changed, 206 insertions(+), 23 deletions(-) create mode 100644 tests/contract_testcases/solana/accessor/accessor_is_external.sol diff --git a/src/abi/tests.rs b/src/abi/tests.rs index 25bd0db86a..fbfe9c3e1a 100644 --- a/src/abi/tests.rs +++ b/src/abi/tests.rs @@ -84,10 +84,15 @@ fn constants_and_types() { assert_eq!(idl.types.len(), 2); - assert_eq!(idl.types[0].name, "MyStruct"); - assert!(idl.types[0].docs.is_none()); + assert_eq!(idl.types[1].name, "cte5_returns"); assert_eq!( - idl.types[0].ty, + idl.types[1].docs, + Some(vec![ + "Data structure to hold the multiple returns of function cte5".into() + ]) + ); + assert_eq!( + idl.types[1].ty, IdlTypeDefinitionTy::Struct { fields: vec![ IdlField { @@ -104,10 +109,10 @@ fn constants_and_types() { } ); - assert_eq!(idl.types[1].name, "Week"); - assert!(idl.types[1].docs.is_none()); + assert_eq!(idl.types[0].name, "Week"); + assert!(idl.types[0].docs.is_none()); assert_eq!( - idl.types[1].ty, + idl.types[0].ty, IdlTypeDefinitionTy::Enum { variants: vec![ IdlEnumVariant { diff --git a/src/sema/variables.rs b/src/sema/variables.rs index 3bb220b800..70468626b6 100644 --- a/src/sema/variables.rs +++ b/src/sema/variables.rs @@ -21,6 +21,7 @@ use solang_parser::{ doccomment::DocComment, pt::{self, CodeLocation, OptionalCodeLocation}, }; +use std::sync::Arc; pub struct DelayedResolveInitializer<'a> { var_no: usize, @@ -460,6 +461,143 @@ pub fn variable_decl<'a>( )); } + let body; + let returns; + + // If the return value of an accessor function is a struct, return the members rather than + // the struct. This is a particular inconsistency in Solidity which does not apply recursively, + // i.e. if the struct contains structs, then those values are returned as structs. + if let Type::Struct(str) = &ty { + let expr = Arc::new(expr); + + if constant { + let var_no = symtable + .add( + &pt::Identifier { + loc: pt::Loc::Implicit, + name: "_".into(), + }, + ty.clone(), + ns, + VariableInitializer::Solidity(Some(expr.clone())), + VariableUsage::LocalVariable, + Some(pt::StorageLocation::Storage(pt::Loc::Implicit)), + ) + .unwrap(); + + symtable.vars[&var_no].read = true; + symtable.vars[&var_no].assigned = true; + + let def = str.definition(ns); + + let list = def + .fields + .iter() + .enumerate() + .map(|(i, param)| Expression::Load { + loc: pt::Loc::Implicit, + ty: param.ty.clone(), + expr: Expression::StructMember { + loc: pt::Loc::Implicit, + ty: Type::Ref(def.fields[i].ty.clone().into()), + expr: Expression::Variable { + loc: pt::Loc::Implicit, + ty: ty.clone(), + var_no, + } + .into(), + field: i, + } + .into(), + }) + .collect(); + + body = vec![ + Statement::VariableDecl(pt::Loc::Implicit, var_no, param, Some(expr)), + Statement::Return( + pt::Loc::Implicit, + Some(Expression::List { + loc: pt::Loc::Implicit, + list, + }), + ), + ]; + + returns = def.fields.clone(); + } else { + let ty = Type::StorageRef(false, ty.clone().into()); + + let var_no = symtable + .add( + &pt::Identifier { + loc: pt::Loc::Implicit, + name: "_".into(), + }, + ty.clone(), + ns, + VariableInitializer::Solidity(Some(expr.clone())), + VariableUsage::LocalVariable, + Some(pt::StorageLocation::Storage(pt::Loc::Implicit)), + ) + .unwrap(); + + symtable.vars[&var_no].read = true; + symtable.vars[&var_no].assigned = true; + + let def = str.definition(ns); + + let list = def + .fields + .iter() + .enumerate() + .map(|(i, param)| Expression::StorageLoad { + loc: pt::Loc::Implicit, + ty: param.ty.clone(), + expr: Expression::StructMember { + loc: pt::Loc::Implicit, + ty: Type::StorageRef(false, def.fields[i].ty.clone().into()), + expr: Expression::Variable { + loc: pt::Loc::Implicit, + ty: ty.clone(), + var_no, + } + .into(), + field: i, + } + .into(), + }) + .collect(); + + body = vec![ + Statement::VariableDecl(pt::Loc::Implicit, var_no, param, Some(expr)), + Statement::Return( + pt::Loc::Implicit, + Some(Expression::List { + loc: pt::Loc::Implicit, + list, + }), + ), + ]; + + returns = def.fields.clone(); + } + } else { + body = vec![Statement::Return( + pt::Loc::Implicit, + Some(if constant { + expr + } else { + Expression::StorageLoad { + loc: pt::Loc::Implicit, + ty, + expr: Box::new(expr), + } + }), + )]; + + returns = vec![param]; + } + let mut func = Function::new( def.name.as_ref().unwrap().loc, def.name.as_ref().unwrap().name.to_owned(), @@ -468,25 +606,14 @@ pub fn variable_decl<'a>( pt::FunctionTy::Function, // accessors for constant variables have view mutability Some(pt::Mutability::View(def.name.as_ref().unwrap().loc)), - visibility, + pt::Visibility::External(None), params, - vec![param], + returns, ns, ); // Create the implicit body - just return the value - func.body = vec![Statement::Return( - pt::Loc::Implicit, - Some(if constant { - expr - } else { - Expression::StorageLoad { - loc: pt::Loc::Implicit, - ty, - expr: Box::new(expr), - } - }), - )]; + func.body = body; func.is_accessor = true; func.has_body = true; func.is_override = is_override; diff --git a/tests/contract_testcases/solana/accessor/accessor_is_external.sol b/tests/contract_testcases/solana/accessor/accessor_is_external.sol new file mode 100644 index 0000000000..43c6cbda7f --- /dev/null +++ b/tests/contract_testcases/solana/accessor/accessor_is_external.sol @@ -0,0 +1,10 @@ +contract C { + int constant public c = 102; + + function f() public { + int x = c(); + } +} +// ---- Expect: diagnostics ---- +// error: 5:11-14: functions declared external cannot be called via an internal function call +// note 2:22-23: declaration of function 'c' diff --git a/tests/evm.rs b/tests/evm.rs index 0783ed7681..79d95d4129 100644 --- a/tests/evm.rs +++ b/tests/evm.rs @@ -248,7 +248,7 @@ fn ethereum_solidity_tests() { }) .sum(); - assert_eq!(errors, 1042); + assert_eq!(errors, 1039); } fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec) { diff --git a/tests/solana_tests/accessor.rs b/tests/solana_tests/accessor.rs index 991e59b83e..366866a1b4 100644 --- a/tests/solana_tests/accessor.rs +++ b/tests/solana_tests/accessor.rs @@ -205,3 +205,44 @@ fn constant() { ]) ); } + +#[test] +fn struct_accessor() { + let mut vm = build_solidity( + r#" + contract C { + struct E { + bytes4 b4; + } + struct S { + int64 f1; + bool f2; + E f3; + } + S public a = S({f1: -63, f2: false, f3: E("nuff")}); + S[100] public s; + mapping(int => S) public m; + E public constant e = E("cons"); + + constructor() { + s[99] = S({f1: 65535, f2: true, f3: E("naff")}); + m[1023413412] = S({f1: 414243, f2: true, f3: E("niff")}); + } + + function f() public view { + (int64 a1, bool b, E memory c) = this.a(); + require(a1 == -63 && !b && c.b4 == "nuff", "a"); + (a1, b, c) = this.s(99); + require(a1 == 65535 && b && c.b4 == "naff", "b"); + (a1, b, c) = this.m(1023413412); + require(a1 == 414243 && b && c.b4 == "niff", "c"); + c.b4 = this.e(); + require(a1 == 414243 && b && c.b4 == "cons", "E"); + } + }"#, + ); + + vm.constructor(&[]); + + vm.function("f", &[]); +} diff --git a/tests/substrate_tests/inheritance.rs b/tests/substrate_tests/inheritance.rs index fb1389fa79..afa42eb55c 100644 --- a/tests/substrate_tests/inheritance.rs +++ b/tests/substrate_tests/inheritance.rs @@ -666,11 +666,11 @@ fn var_or_function() { r##" contract x is c { function f1() public returns (int64) { - return c.selector(); + return selector; } function f2() public returns (int64) { - function() internal returns (int64) a = c.selector; + function() external returns (int64) a = this.selector; return a(); } }