Implicit accessor function should return struct members, not struct (#1374)

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 <sean@mess.org>

src/abi/tests.rs

@@ -84,30 +84,10 @@ fn constants_and_types() {
 
     assert_eq!(idl.types.len(), 2);
 
-    assert_eq!(idl.types[0].name, "MyStruct");
+    assert_eq!(idl.types[0].name, "Week");
     assert!(idl.types[0].docs.is_none());
     assert_eq!(
         idl.types[0].ty,
-        IdlTypeDefinitionTy::Struct {
-            fields: vec![
-                IdlField {
-                    name: "g".to_string(),
-                    docs: None,
-                    ty: IdlType::U8,
-                },
-                IdlField {
-                    name: "d".to_string(),
-                    docs: None,
-                    ty: IdlType::Array(IdlType::U8.into(), 2)
-                }
-            ]
-        }
-    );
-
-    assert_eq!(idl.types[1].name, "Week");
-    assert!(idl.types[1].docs.is_none());
-    assert_eq!(
-        idl.types[1].ty,
         IdlTypeDefinitionTy::Enum {
             variants: vec![
                 IdlEnumVariant {
@@ -125,6 +105,31 @@ fn constants_and_types() {
             ]
         }
     );
+
+    assert_eq!(idl.types[1].name, "cte5_returns");
+    assert_eq!(
+        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 {
+                    name: "g".to_string(),
+                    docs: None,
+                    ty: IdlType::U8,
+                },
+                IdlField {
+                    name: "d".to_string(),
+                    docs: None,
+                    ty: IdlType::Array(IdlType::U8.into(), 2)
+                }
+            ]
+        }
+    );
 }
 
 #[test]

src/sema/expression/function_call.rs

@@ -2665,13 +2665,22 @@ fn resolve_internal_call(
         return None;
     } else if let (Some(base_no), Some(derived_no)) = (func.contract_no, context.contract_no) {
         if is_base(base_no, derived_no, ns) && matches!(func.visibility, Visibility::External(_)) {
-            errors.push(Diagnostic::error_with_note(
-                *loc,
-                "functions declared external cannot be called via an internal function call"
-                    .to_string(),
-                func.loc,
-                format!("declaration of {} '{}'", func.ty, func.name),
-            ));
+            if func.is_accessor {
+                errors.push(Diagnostic::error_with_note(
+                    *loc,
+                    "accessor function cannot be called via an internal function call".to_string(),
+                    func.loc,
+                    format!("declaration of '{}'", func.name),
+                ));
+            } else {
+                errors.push(Diagnostic::error_with_note(
+                    *loc,
+                    "functions declared external cannot be called via an internal function call"
+                        .to_string(),
+                    func.loc,
+                    format!("declaration of {} '{}'", func.ty, func.name),
+                ));
+            }
             return None;
         }
     }

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,
@@ -450,9 +451,8 @@ pub fn variable_decl<'a>(
             let mut params = Vec::new();
             let param =
                 collect_parameters(&ty, &def.name, &mut symtable, &mut params, &mut expr, ns)?;
-            let ty = param.ty.clone();
 
-            if ty.contains_mapping(ns) {
+            if param.ty.contains_mapping(ns) {
                 // we can't return a mapping
                 ns.diagnostics.push(Diagnostic::decl_error(
                     def.loc,
@@ -460,6 +460,8 @@ pub fn variable_decl<'a>(
                 ));
             }
 
+            let (body, returns) = accessor_body(expr, param, constant, &mut symtable, ns);
+
             let mut func = Function::new(
                 def.name.as_ref().unwrap().loc,
                 def.name.as_ref().unwrap().name.to_owned(),
@@ -468,25 +470,13 @@ 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;
@@ -652,6 +642,127 @@ fn collect_parameters(
     }
 }
 
+/// Build up an ast for the implict accessor function for public state variables.
+fn accessor_body(
+    expr: Expression,
+    param: Parameter,
+    constant: bool,
+    symtable: &mut Symtable,
+    ns: &mut Namespace,
+) -> (Vec<Statement>, Vec<Parameter>) {
+    // This could be done in codegen rather than sema, however I am not sure how we would implement
+    // that. After building up the parameter/returns list for the implicit function, we have done almost
+    // all the work already for building the body (see `expr` and `param` above). So, we would need to
+    // share some code between codegen and sema for this.
+
+    let mut ty = param.ty.clone();
+
+    // 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) = &param.ty {
+        let expr = Arc::new(expr);
+
+        if !constant {
+            ty = Type::StorageRef(false, ty.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 returns = def.fields.clone();
+
+        let list = if constant {
+            def.fields
+                .iter()
+                .enumerate()
+                .map(|(field, param)| Expression::Load {
+                    loc: pt::Loc::Implicit,
+                    ty: param.ty.clone(),
+                    expr: Expression::StructMember {
+                        loc: pt::Loc::Implicit,
+                        ty: Type::Ref(def.fields[field].ty.clone().into()),
+                        expr: Expression::Variable {
+                            loc: pt::Loc::Implicit,
+                            ty: ty.clone(),
+                            var_no,
+                        }
+                        .into(),
+                        field,
+                    }
+                    .into(),
+                })
+                .collect()
+        } else {
+            def.fields
+                .iter()
+                .enumerate()
+                .map(|(field, param)| Expression::StorageLoad {
+                    loc: pt::Loc::Implicit,
+                    ty: param.ty.clone(),
+                    expr: Expression::StructMember {
+                        loc: pt::Loc::Implicit,
+                        ty: Type::StorageRef(false, def.fields[field].ty.clone().into()),
+                        expr: Expression::Variable {
+                            loc: pt::Loc::Implicit,
+                            ty: ty.clone(),
+                            var_no,
+                        }
+                        .into(),
+                        field,
+                    }
+                    .into(),
+                })
+                .collect()
+        };
+
+        let 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,
+                }),
+            ),
+        ];
+
+        (body, returns)
+    } else {
+        let body = vec![Statement::Return(
+            pt::Loc::Implicit,
+            Some(if constant {
+                expr
+            } else {
+                Expression::StorageLoad {
+                    loc: pt::Loc::Implicit,
+                    ty,
+                    expr: Box::new(expr),
+                }
+            }),
+        )];
+
+        let returns = vec![param];
+
+        (body, returns)
+    }
+}
+
 pub fn resolve_initializers(
     initializers: &[DelayedResolveInitializer],
     file_no: usize,

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: accessor function cannot be called via an internal function call
+// 	note 2:22-23: declaration of 'c'

tests/evm.rs

@@ -248,7 +248,7 @@ fn ethereum_solidity_tests() {
         })
         .sum();
 
-    assert_eq!(errors, 1032);
+    assert_eq!(errors, 1029);
 }
 
 fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec<String>) {

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", &[]);
+}

tests/substrate_tests/inheritance.rs

@@ -666,12 +666,12 @@ 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;
-                return a();
+                function() external returns (int64) a = this.selector;
+                return a{flags: 8}();
             }
         }