Update tests to solc v0.8.22 and fix a few tests (#1579)

- Do not modify `ns.diagnostics` directly - ever. We may want to resolve
an expression and drop all the diagnostics for it.
- Ensure all variable errors are surfaced
- Update solc testdata to `v0.8.22`

Signed-off-by: Sean Young <sean@mess.org>

.gitmodules

@@ -1,4 +1,4 @@
-[submodule "solang-parser/testdata/solidity"]
+[submodule "testdata/solidity"]
 	path = testdata/solidity
 	url = https://github.com/ethereum/solidity
 [submodule "integration/polkadot/tornado/tornado-cli"]

solang-parser/README.md

@@ -3,7 +3,7 @@
 This crate is part of [Hyperledger Solang](https://solang.readthedocs.io/). It contains the
 parser for Solidity, including the dialects used by Solang for Solana and Polkadot.
 
-This parser is compatible with Ethereum Solidity v0.8.21.
+This parser is compatible with Ethereum Solidity v0.8.22.
 
 `solang-parser` is still `0.*.*`, so breaking changes [may occur at any time](https://semver.org/#spec-item-4). If you must depend on `solang-parser`, we recommend pinning to a specific version, i.e., `=0.y.z`.
 

src/abi/mod.rs

@@ -13,7 +13,7 @@ pub fn generate_abi(
     ns: &Namespace,
     code: &[u8],
     verbose: bool,
-    default_authors: &Vec<String>,
+    default_authors: &[String],
     version: &str,
 ) -> (String, &'static str) {
     match ns.target {

src/abi/polkadot.rs

@@ -544,7 +544,7 @@ pub fn metadata(
     contract_no: usize,
     code: &[u8],
     ns: &ast::Namespace,
-    default_authors: &Vec<String>,
+    default_authors: &[String],
     contract_version: &str,
 ) -> Value {
     let hash = blake2_rfc::blake2b::blake2b(32, &[], code);

src/bin/solang.rs

@@ -12,7 +12,6 @@ use solang::{
     file_resolver::FileResolver,
     sema::{ast::Namespace, file::PathDisplay},
     standard_json::{EwasmContract, JsonContract, JsonResult},
-    Target,
 };
 use std::{
     collections::{HashMap, HashSet},
@@ -243,8 +242,11 @@ fn compile(compile_args: &Compile) {
         let mut seen_contracts = HashMap::new();
 
         let authors = if let Some(authors) = &compile_args.package.authors {
-            if target == Target::Solana {
-                eprintln!("warning: the `authors` flag will be ignored for solana target")
+            if !target.is_polkadot() {
+                eprintln!(
+                    "warning: the `authors` flag will be ignored for {} target",
+                    target
+                )
             }
             authors.clone()
         } else {
@@ -348,7 +350,7 @@ fn contract_results(
     json_contracts: &mut HashMap<String, JsonContract>,
     seen_contracts: &mut HashMap<String, String>,
     opt: &Options,
-    default_authors: &Vec<String>,
+    default_authors: &[String],
     version: &str,
 ) {
     let verbose = compiler_output.verbose;

src/sema/eval.rs

@@ -3,6 +3,7 @@
 use super::{
     ast::{Diagnostic, Expression, Namespace, Type},
     diagnostics::Diagnostics,
+    Recurse,
 };
 use num_bigint::BigInt;
 use num_bigint::Sign;
@@ -295,9 +296,16 @@ pub fn eval_const_rational(
     }
 }
 
+impl Expression {
+    /// Check the expression for constant overflows, e.g. `uint8 a = 100 + 200;`.
+    pub fn check_constant_overflow(&self, diagnostics: &mut Diagnostics) {
+        self.recurse(diagnostics, check_term_for_constant_overflow);
+    }
+}
+
 /// Function that recurses the expression and folds number literals by calling 'eval_constants_in_expression'.
 /// If the expression is an arithmetic operation of two number literals, overflow_check() will be called on the result.
-pub(super) fn check_term_for_constant_overflow(expr: &Expression, ns: &mut Namespace) -> bool {
+fn check_term_for_constant_overflow(expr: &Expression, diagnostics: &mut Diagnostics) -> bool {
     match expr {
         Expression::Add { .. }
         | Expression::Subtract { .. }
@@ -310,28 +318,27 @@ pub(super) fn check_term_for_constant_overflow(expr: &Expression, ns: &mut Names
         | Expression::BitwiseAnd { .. }
         | Expression::BitwiseOr { .. }
         | Expression::BitwiseXor { .. }
-        | Expression::NumberLiteral { .. } => {
-            match eval_constants_in_expression(expr, &mut ns.diagnostics) {
-                (
-                    Some(Expression::NumberLiteral {
-                        loc,
-                        ty,
-                        value: result,
-                    }),
-                    _,
-                ) => {
-                    if let Some(diagnostic) = overflow_diagnostic(&result, &ty, &loc) {
-                        ns.diagnostics.push(diagnostic);
-                    }
-
-                    return false;
-                }
-                (None, false) => {
-                    return false;
+        | Expression::NumberLiteral { .. } => match eval_constants_in_expression(expr, diagnostics)
+        {
+            (
+                Some(Expression::NumberLiteral {
+                    loc,
+                    ty,
+                    value: result,
+                }),
+                _,
+            ) => {
+                if let Some(diagnostic) = overflow_diagnostic(&result, &ty, &loc) {
+                    diagnostics.push(diagnostic);
                 }
-                _ => {}
+
+                return false;
             }
-        }
+            (None, false) => {
+                return false;
+            }
+            _ => {}
+        },
         _ => {}
     }
 

src/sema/expression/assign.rs

@@ -2,13 +2,11 @@
 
 use crate::sema::ast::{Expression, Namespace, RetrieveType, Type};
 use crate::sema::diagnostics::Diagnostics;
-use crate::sema::eval::check_term_for_constant_overflow;
 use crate::sema::expression::integers::type_bits_and_sign;
 use crate::sema::expression::resolve_expression::expression;
 use crate::sema::expression::{ExprContext, ResolveTo};
 use crate::sema::symtable::Symtable;
 use crate::sema::unused_variable::{assigned_variable, used_variable};
-use crate::sema::Recurse;
 use solang_parser::diagnostics::Diagnostic;
 use solang_parser::pt;
 use solang_parser::pt::CodeLocation;
@@ -46,7 +44,7 @@ pub(super) fn assign_single(
         ResolveTo::Type(var_ty.deref_any()),
     )?;
 
-    val.recurse(ns, check_term_for_constant_overflow);
+    val.check_constant_overflow(diagnostics);
 
     used_variable(ns, &val, symtable);
     match &var {

src/sema/expression/literals.rs

@@ -125,7 +125,7 @@ pub(super) fn hex_literal(
 pub(crate) fn hex_number_literal(
     loc: &pt::Loc,
     n: &str,
-    ns: &mut Namespace,
+    ns: &Namespace,
     diagnostics: &mut Diagnostics,
     resolve_to: ResolveTo,
 ) -> Result<Expression, ()> {
@@ -200,7 +200,7 @@ pub(crate) fn hex_number_literal(
 pub(super) fn address_literal(
     loc: &pt::Loc,
     address: &str,
-    ns: &mut Namespace,
+    ns: &Namespace,
     diagnostics: &mut Diagnostics,
 ) -> Result<Expression, ()> {
     if ns.target.is_polkadot() {
@@ -516,7 +516,7 @@ pub(super) fn struct_literal(
 pub(crate) fn unit_literal(
     loc: &pt::Loc,
     unit: &Option<pt::Identifier>,
-    ns: &mut Namespace,
+    ns: &Namespace,
     diagnostics: &mut Diagnostics,
 ) -> BigInt {
     if let Some(unit) = unit {

src/sema/expression/resolve_expression.rs

@@ -21,11 +21,9 @@ use crate::sema::expression::{
 use crate::sema::{
     symtable::Symtable,
     unused_variable::{check_function_call, check_var_usage_expression, used_variable},
-    Recurse,
     {
         ast::{Expression, Namespace, RetrieveType, Type},
         diagnostics::Diagnostics,
-        eval::check_term_for_constant_overflow,
     },
 };
 use num_bigint::BigInt;
@@ -249,7 +247,7 @@ pub fn expression(
             };
             let expr = assign_expr(loc, var, expr, e, context, ns, symtable, diagnostics);
             if let Ok(expression) = &expr {
-                expression.recurse(ns, check_term_for_constant_overflow);
+                expression.check_constant_overflow(diagnostics);
             }
             expr
         }

src/sema/expression/subscript.rs

@@ -2,11 +2,9 @@
 
 use crate::sema::ast::{Expression, Mapping, Namespace, RetrieveType, Type};
 use crate::sema::diagnostics::Diagnostics;
-use crate::sema::eval::check_term_for_constant_overflow;
 use crate::sema::expression::resolve_expression::expression;
 use crate::sema::expression::{ExprContext, ResolveTo};
 use crate::sema::symtable::Symtable;
-use crate::sema::Recurse;
 use solang_parser::diagnostics::Diagnostic;
 use solang_parser::pt;
 use solang_parser::pt::CodeLocation;
@@ -52,7 +50,7 @@ pub(super) fn array_subscript(
 
     let index_ty = index.ty();
 
-    index.recurse(ns, check_term_for_constant_overflow);
+    index.check_constant_overflow(diagnostics);
 
     match index_ty.deref_any() {
         Type::Uint(_) => (),

src/sema/expression/variable.rs

@@ -12,7 +12,7 @@ use solang_parser::pt;
 pub(super) fn variable(
     id: &pt::Identifier,
     context: &ExprContext,
-    ns: &mut Namespace,
+    ns: &Namespace,
     symtable: &mut Symtable,
     diagnostics: &mut Diagnostics,
     resolve_to: ResolveTo,

src/sema/functions.rs

@@ -904,7 +904,7 @@ pub fn resolve_params(
                             "parameter of type '{}' not alowed in public or external functions",
                             ty.to_string(ns)
                         );
-                        ns.diagnostics.push(Diagnostic::error(p.ty.loc(), message));
+                        diagnostics.push(Diagnostic::error(p.ty.loc(), message));
                         success = false
                     }
                 }
@@ -1032,7 +1032,7 @@ pub fn resolve_returns(
                             "return type '{}' not allowed in public or external functions",
                             ty.to_string(ns)
                         );
-                        ns.diagnostics.push(Diagnostic::error(r.ty.loc(), message));
+                        diagnostics.push(Diagnostic::error(r.ty.loc(), message));
                         success = false
                     }
                 }

src/sema/namespace.rs

@@ -365,7 +365,7 @@ impl Namespace {
 
     /// Resolve a free function name with namespace
     pub(super) fn resolve_function_with_namespace(
-        &mut self,
+        &self,
         file_no: usize,
         contract_no: Option<usize>,
         name: &pt::IdentifierPath,

src/sema/statements.rs

@@ -3,7 +3,6 @@
 use super::ast::*;
 use super::contracts::is_base;
 use super::diagnostics::Diagnostics;
-use super::eval::check_term_for_constant_overflow;
 use super::expression::{
     function_call::{available_functions, call_expr, named_call_expr},
     ExprContext, ResolveTo,
@@ -368,7 +367,8 @@ fn statement(
                     ResolveTo::Type(&var_ty),
                 )?;
 
-                expr.recurse(ns, check_term_for_constant_overflow);
+                expr.check_constant_overflow(diagnostics);
+
                 used_variable(ns, &expr, symtable);
 
                 Some(Arc::new(expr.cast(
@@ -761,7 +761,7 @@ fn statement(
         pt::Statement::Return(loc, Some(returns)) => {
             let expr = return_with_values(returns, loc, context, symtable, ns, diagnostics)?;
 
-            expr.recurse(ns, check_term_for_constant_overflow);
+            expr.check_constant_overflow(diagnostics);
 
             for offset in symtable.returns.iter() {
                 let elem = symtable.vars.get_mut(offset).unwrap();
@@ -821,7 +821,7 @@ fn statement(
                         ResolveTo::Discard,
                     )?;
 
-                    ret.recurse(ns, check_term_for_constant_overflow);
+                    ret.check_constant_overflow(diagnostics);
                     ret
                 }
                 pt::Expression::NamedFunctionCall(loc, ty, args) => {
@@ -836,7 +836,7 @@ fn statement(
                         diagnostics,
                         ResolveTo::Discard,
                     )?;
-                    ret.recurse(ns, check_term_for_constant_overflow);
+                    ret.check_constant_overflow(diagnostics);
                     ret
                 }
                 _ => {

src/sema/variables.rs

@@ -14,10 +14,8 @@ use super::{
     tags::resolve_tags,
     ContractDefinition,
 };
-use crate::sema::eval::check_term_for_constant_overflow;
 use crate::sema::expression::resolve_expression::expression;
 use crate::sema::namespace::ResolveTypeContext;
-use crate::sema::Recurse;
 use solang_parser::{
     doccomment::DocComment,
     pt::{self, CodeLocation, OptionalCodeLocation},
@@ -322,9 +320,10 @@ pub fn variable_decl<'a>(
         return None;
     }
 
+    let mut diagnostics = Diagnostics::default();
+
     let initializer = if constant {
         if let Some(initializer) = &def.initializer {
-            let mut diagnostics = Diagnostics::default();
             let context = ExprContext {
                 file_no,
                 unchecked: false,
@@ -347,22 +346,16 @@ pub fn variable_decl<'a>(
                     // implicitly conversion to correct ty
                     match res.cast(&def.loc, &ty, true, ns, &mut diagnostics) {
                         Ok(res) => {
-                            res.recurse(ns, check_term_for_constant_overflow);
+                            res.check_constant_overflow(&mut diagnostics);
                             Some(res)
                         }
-                        Err(_) => {
-                            ns.diagnostics.extend(diagnostics);
-                            None
-                        }
+                        Err(_) => None,
                     }
                 }
-                Err(()) => {
-                    ns.diagnostics.extend(diagnostics);
-                    None
-                }
+                Err(()) => None,
             }
         } else {
-            ns.diagnostics.push(Diagnostic::decl_error(
+            diagnostics.push(Diagnostic::decl_error(
                 def.loc,
                 "missing initializer for constant".to_string(),
             ));
@@ -373,6 +366,8 @@ pub fn variable_decl<'a>(
         None
     };
 
+    ns.diagnostics.extend(diagnostics);
+
     let bases = contract_no.map(|contract_no| ns.contract_bases(contract_no));
 
     let tags = resolve_tags(
@@ -803,7 +798,7 @@ pub fn resolve_initializers(
             ResolveTo::Type(&ty),
         ) {
             if let Ok(res) = res.cast(&initializer.loc(), &ty, true, ns, &mut diagnostics) {
-                res.recurse(ns, check_term_for_constant_overflow);
+                res.check_constant_overflow(&mut diagnostics);
                 ns.contracts[*contract_no].variables[*var_no].initializer = Some(res);
             }
         }

testdata/solidity

@@ -1 +1 @@
-Subproject commit d9974bed7134e043f7ccc593c0c19c67d2d45dc4
+Subproject commit 4fc1097e8df32897ccee8985019610fa5a869212

tests/cli.rs

@@ -31,24 +31,34 @@ fn create_output_dir() {
     let test2 = tmp.path().join("test2");
     let test2_meta = tmp.path().join("test2_meta");
 
-    cmd.args([
-        "compile",
-        "examples/solana/flipper.sol",
-        "--target",
-        "solana",
-        "--contract",
-        "flipper",
-        "--output",
-    ])
-    .arg(test2.clone())
-    .arg("--output-meta")
-    .arg(test2_meta.clone())
-    .assert()
-    .success();
+    let assert = cmd
+        .args([
+            "compile",
+            "examples/solana/flipper.sol",
+            "--target",
+            "solana",
+            "--contract",
+            "flipper",
+            "--contract-authors",
+            "itchy and cratchy",
+            "--output",
+        ])
+        .arg(test2.clone())
+        .arg("--output-meta")
+        .arg(test2_meta.clone())
+        .assert()
+        .success();
 
     File::open(test2.join("flipper.so")).expect("should exist");
     File::open(test2_meta.join("flipper.json")).expect("should exist");
 
+    let output = assert.get_output();
+
+    assert_eq!(
+        String::from_utf8_lossy(&output.stderr),
+        "warning: the `authors` flag will be ignored for Solana target\n"
+    );
+
     let mut cmd = Command::cargo_bin("solang").unwrap();
 
     cmd.args([

tests/contract.rs

@@ -3,6 +3,7 @@
 use path_slash::PathExt;
 use rayon::prelude::*;
 use solang::{
+    abi::generate_abi,
     codegen,
     file_resolver::FileResolver,
     parse_and_resolve,
@@ -92,16 +93,18 @@ fn parse_file(path: PathBuf, target: Target) -> io::Result<()> {
 
     if !ns.diagnostics.any_errors() {
         // let's try and emit
-        match ns.target {
-            Target::Solana | Target::Polkadot { .. } => {
-                for contract in &ns.contracts {
-                    if contract.instantiable {
-                        let _ = contract.emit(&ns, &Default::default());
+        for contract_no in 0..ns.contracts.len() {
+            let contract = &ns.contracts[contract_no];
+
+            if contract.instantiable {
+                let code = match ns.target {
+                    Target::Solana | Target::Polkadot { .. } => {
+                        contract.emit(&ns, &Default::default())
                     }
-                }
-            }
-            Target::EVM => {
-                // not implemented yet
+                    Target::EVM => b"beep".to_vec(),
+                };
+
+                let _ = generate_abi(contract_no, &ns, &code, false, &["unknown".into()], "0.1.0");
             }
         }
     }

tests/contract_testcases/polkadot/contracts/contract_name_08.sol

@@ -15,9 +15,14 @@
             function x() public {
                 a y = new a();
             }
+
+	    function y() public {
+                a y = new a({});
+	    }
         }
         
 // ---- Expect: diagnostics ----
 // warning: 4:19-20: local variable 'y' is unused
 // warning: 10:19-20: local variable 'y' is unused
 // error: 16:23-30: circular reference creating contract 'a'
+// error: 20:23-32: circular reference creating contract 'a'

tests/contract_testcases/polkadot/tags/functions_10.sol

@@ -0,0 +1,10 @@
+contract c {
+	/// @return meh
+	/// @return feh
+	function foo() public returns (int) {
+		return 1;
+	}
+}
+
+// ---- Expect: diagnostics ----
+// error: 3:7-14: duplicate tag '@return'

tests/evm.rs

@@ -176,7 +176,8 @@ contract testing  {
 #[test]
 fn ethereum_solidity_tests() {
     let error_matcher =
-        regex::Regex::new(r"// ----\r?\n(// Warning \d+: .*\n)*// \w+Error( \d+)?: (.*)").unwrap();
+        regex::Regex::new(r"// ----\r?\n(//\s+Warning \d+: .*\n)*//\s+\w+Error( \d+)?: (.*)")
+            .unwrap();
 
     let entries = WalkDir::new(
         Path::new(env!("CARGO_MANIFEST_DIR"))
@@ -252,7 +253,7 @@ fn ethereum_solidity_tests() {
         })
         .sum();
 
-    assert_eq!(errors, 1003);
+    assert_eq!(errors, 993);
 }
 
 fn set_file_contents(source: &str, path: &Path) -> (FileResolver, Vec<String>) {