Implement undefined variable detection and prettify warning messages

Signed-off-by: Lucas Steuernagel <[email protected]>

examples/full_example.sol

@@ -126,7 +126,7 @@ contract full_example {
 	function run_queue() public pure returns (uint16) {
 		uint16 count = 0;
 		// no initializer means its 0.
-		int32 n;
+		int32 n=0;
 
 		do {
 			if (get_pid_state(n) == State.Waiting) {

src/bin/solang.rs

@@ -200,7 +200,7 @@ fn main() {
         for filename in matches.values_of("INPUT").unwrap() {
             let ns = solang::parse_and_resolve(filename, &mut cache, target);
 
-            diagnostics::print_messages(&ns, verbose);
+            diagnostics::print_messages(&cache, &ns, verbose);
 
             if ns.contracts.is_empty() {
                 eprintln!("{}: error: no contracts found", filename);
@@ -381,10 +381,10 @@ fn process_filename(
     }
 
     if matches.is_present("STD-JSON") {
-        let mut out = diagnostics::message_as_json(&ns);
+        let mut out = diagnostics::message_as_json(&ns, &cache);
         json.errors.append(&mut out);
     } else {
-        diagnostics::print_messages(&ns, verbose);
+        diagnostics::print_messages(&cache, &ns, verbose);
     }
 
     if ns.contracts.is_empty() || diagnostics::any_errors(&ns.diagnostics) {

src/codegen/cfg.rs

@@ -9,6 +9,7 @@ use super::{
     constant_folding, dead_storage, expression::expression, reaching_definitions, strength_reduce,
     vector_to_slice, Options,
 };
+use crate::codegen::undefined_variable;
 use crate::parser::pt;
 use crate::sema::ast::{
     CallTy, Contract, Expression, Function, Namespace, Parameter, StringLocation, Type,
@@ -148,7 +149,117 @@ pub enum Instr {
     Nop,
 }
 
-#[derive(Clone)]
+impl Instr {
+    pub fn recurse_expressions<T>(
+        &self,
+        cx: &mut T,
+        f: fn(expr: &Expression, ctx: &mut T) -> bool,
+    ) {
+        match self {
+            Instr::BranchCond { cond: expr, .. }
+            | Instr::Store { dest: expr, .. }
+            | Instr::LoadStorage { storage: expr, .. }
+            | Instr::ClearStorage { storage: expr, .. }
+            | Instr::Print { expr }
+            | Instr::AssertFailure { expr: Some(expr) }
+            | Instr::PopStorage { storage: expr, .. }
+            | Instr::AbiDecode { data: expr, .. }
+            | Instr::SelfDestruct { recipient: expr }
+            | Instr::Set { expr, .. } => {
+                expr.recurse(cx, f);
+            }
+
+            Instr::PushMemory { value: expr, .. } => {
+                expr.recurse(cx, f);
+            }
+
+            Instr::SetStorage { value, storage, .. }
+            | Instr::PushStorage { value, storage, .. } => {
+                value.recurse(cx, f);
+                storage.recurse(cx, f);
+            }
+
+            Instr::SetStorageBytes {
+                value,
+                storage,
+                offset,
+            } => {
+                value.recurse(cx, f);
+                storage.recurse(cx, f);
+                offset.recurse(cx, f);
+            }
+
+            Instr::Return { value: exprs } | Instr::Call { args: exprs, .. } => {
+                for expr in exprs {
+                    expr.recurse(cx, f);
+                }
+            }
+
+            Instr::Constructor {
+                args,
+                value,
+                gas,
+                salt,
+                space,
+                ..
+            } => {
+                for arg in args {
+                    arg.recurse(cx, f);
+                }
+                if let Some(expr) = value {
+                    expr.recurse(cx, f);
+                }
+                gas.recurse(cx, f);
+
+                if let Some(expr) = salt {
+                    expr.recurse(cx, f);
+                }
+
+                if let Some(expr) = space {
+                    expr.recurse(cx, f);
+                }
+            }
+
+            Instr::ExternalCall {
+                address,
+                payload,
+                value,
+                gas,
+                ..
+            } => {
+                if let Some(expr) = address {
+                    expr.recurse(cx, f);
+                }
+                payload.recurse(cx, f);
+                value.recurse(cx, f);
+                gas.recurse(cx, f);
+            }
+
+            Instr::ValueTransfer { address, value, .. } => {
+                address.recurse(cx, f);
+                value.recurse(cx, f);
+            }
+
+            Instr::EmitEvent { data, topics, .. } => {
+                for expr in data {
+                    expr.recurse(cx, f);
+                }
+
+                for expr in topics {
+                    expr.recurse(cx, f);
+                }
+            }
+
+            Instr::AssertFailure { expr: None }
+            | Instr::Unreachable
+            | Instr::Nop
+            | Instr::Branch { .. }
+            | Instr::PopMemory { .. } => {}
+        }
+    }
+}
+
+#[derive(Clone, Debug)]
 #[allow(clippy::large_enum_variant)]
 pub enum InternalCallTy {
     Static(usize),
@@ -649,6 +760,7 @@ impl ControlFlowGraph {
                     .collect::<Vec<String>>()
                     .join(", ")
             ),
+            Expression::Undefined(_) => "undef".to_string(),
             _ => panic!("{:?}", expr),
         }
     }
@@ -1061,7 +1173,7 @@ pub fn generate_cfg(
         cfg.selector = func.selector();
     }
 
-    optimize(&mut cfg, ns, opt);
+    optimize_and_check_cfg(&mut cfg, ns, function_no, opt);
 
     all_cfgs[cfg_no] = cfg;
 }
@@ -1092,9 +1204,20 @@ fn resolve_modifier_call<'a>(
     panic!("modifier should resolve to internal call");
 }
 
-/// Run codegen optimizer passess
-pub fn optimize(cfg: &mut ControlFlowGraph, ns: &mut Namespace, opt: &Options) {
+/// Detect undefined variables and run codegen optimizer passess
+pub fn optimize_and_check_cfg(
+    cfg: &mut ControlFlowGraph,
+    ns: &mut Namespace,
+    func_no: Option<usize>,
+    opt: &Options,
+) {
     reaching_definitions::find(cfg);
+    if let Some(function) = func_no {
+        // If there are undefined variables, we raise an error and don't run optimizations
+        if undefined_variable::find_undefined_variables(cfg, ns, function) {
+            return;
+        }
+    }
     if opt.constant_folding {
         constant_folding::constant_folding(cfg, ns);
     }

src/codegen/constant_folding.rs

@@ -1083,6 +1083,7 @@ fn expression(
         | Expression::FunctionArg(_, _, _) => (expr.clone(), true),
         Expression::AllocDynamicArray(_, _, _, _)
         | Expression::ReturnData(_)
+        | Expression::Undefined(_)
         | Expression::FormatString { .. }
         | Expression::InternalFunctionCfg(_) => (expr.clone(), false),
         // nothing else is permitted in cfg

src/codegen/mod.rs

@@ -7,10 +7,11 @@ mod reaching_definitions;
 mod statements;
 mod storage;
 mod strength_reduce;
+mod undefined_variable;
 mod unused_variable;
 mod vector_to_slice;
 
-use self::cfg::{optimize, ControlFlowGraph, Instr, Vartable};
+use self::cfg::{optimize_and_check_cfg, ControlFlowGraph, Instr, Vartable};
 use self::expression::expression;
 use crate::sema::ast::{Layout, Namespace};
 use crate::sema::contracts::visit_bases;
@@ -132,7 +133,7 @@ fn storage_initializer(contract_no: usize, ns: &mut Namespace, opt: &Options) ->
 
     cfg.vars = vartab.drain();
 
-    optimize(&mut cfg, ns, opt);
+    optimize_and_check_cfg(&mut cfg, ns, None, opt);
 
     cfg
 }

src/codegen/statements.rs

@@ -42,7 +42,7 @@ pub fn statement(
             }
         }
         Statement::VariableDecl(loc, pos, _, Some(init)) => {
-            if should_remove_variable(pos, func, Some(init)) {
+            if should_remove_variable(pos, func) {
                 let mut params = SideEffectsCheckParameters {
                     cfg,
                     contract_no,
@@ -57,7 +57,6 @@ pub fn statement(
             }
 
             let expr = expression(init, cfg, contract_no, Some(func), ns, vartab);
-
             cfg.add(
                 vartab,
                 Instr::Set {
@@ -68,21 +67,19 @@ pub fn statement(
             );
         }
         Statement::VariableDecl(loc, pos, param, None) => {
-            if should_remove_variable(pos, func, None) {
+            if should_remove_variable(pos, func) {
                 return;
             }
 
-            // Set it to a default value
-            if let Some(expr) = param.ty.default(ns) {
-                cfg.add(
-                    vartab,
-                    Instr::Set {
-                        loc: *loc,
-                        res: *pos,
-                        expr,
-                    },
-                );
-            }
+            // Add variable as undefined
+            cfg.add(
+                vartab,
+                Instr::Set {
+                    loc: *loc,
+                    res: *pos,
+                    expr: Expression::Undefined(param.ty.clone()),
+                },
+            );
         }
         Statement::Return(_, values) => {
             if let Some(return_instr) = return_override {
@@ -787,7 +784,7 @@ fn destructure(
                 let expr = cast(&param.loc, right, &param.ty, true, ns, &mut Vec::new())
                     .expect("sema should have checked cast");
 
-                if should_remove_variable(res, func, Some(&expr)) {
+                if should_remove_variable(res, func) {
                     continue;
                 }
 
@@ -1219,7 +1216,9 @@ impl Type {
                 )),
                 None,
             )),
-            Type::InternalFunction { .. } | Type::ExternalFunction { .. } => None,
+            Type::InternalFunction { .. } | Type::Contract(_) | Type::ExternalFunction { .. } => {
+                None
+            }
             Type::Array(_, dims) => {
                 if dims[0].is_none() {
                     Some(Expression::AllocDynamicArray(
@@ -1241,7 +1240,6 @@ impl Type {
                     ))
                 }
             }
-            Type::Contract(_) => None,
             _ => unreachable!(),
         }
     }

src/codegen/strength_reduce.rs

@@ -1690,6 +1690,14 @@ fn expression_values(expr: &Expression, vars: &Variables, ns: &Namespace) -> Has
             // reference to a function; ignore
             HashSet::new()
         }
+        Expression::Undefined(expr_type) => {
+            // If the variable is undefined, we can return the default value to optimize operations
+            if let Some(default_expr) = expr_type.default(ns) {
+                return expression_values(&default_expr, vars, ns);
+            }
+
+            HashSet::new()
+        }
         e => {
             let ty = e.ty();
             let mut set = HashSet::new();