From 3641eb9f435f6fa164cd70cb86ee7bad0755f657 Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Wed, 18 Aug 2021 11:18:07 -0300 Subject: [PATCH] Implement undefined variable detection and prettify warning messages Signed-off-by: Lucas Steuernagel --- examples/full_example.sol | 2 +- src/bin/solang.rs | 6 +- src/codegen/cfg.rs | 131 ++++- src/codegen/constant_folding.rs | 1 + src/codegen/mod.rs | 5 +- src/codegen/statements.rs | 32 +- src/codegen/strength_reduce.rs | 8 + src/codegen/undefined_variable.rs | 137 +++++ src/codegen/unused_variable.rs | 33 +- src/emit/mod.rs | 14 +- src/file_cache.rs | 40 +- src/sema/ast.rs | 86 ++- src/sema/diagnostics.rs | 43 +- src/sema/expression.rs | 80 +-- src/sema/file.rs | 8 +- src/sema/mod.rs | 9 +- src/sema/printer.rs | 1 + src/sema/symtable.rs | 25 + src/sema/unused_variable.rs | 13 +- tests/ewasm.rs | 2 +- tests/solana.rs | 2 +- tests/substrate.rs | 4 +- tests/substrate_tests/enums.rs | 1 + tests/substrate_tests/structs.rs | 2 +- tests/undefined_variable_detection.rs | 746 ++++++++++++++++++++++++++ tests/unused_variable_detection.rs | 20 +- 26 files changed, 1267 insertions(+), 184 deletions(-) create mode 100644 src/codegen/undefined_variable.rs create mode 100644 tests/undefined_variable_detection.rs diff --git a/examples/full_example.sol b/examples/full_example.sol index 956269820..35a393756 100644 --- a/examples/full_example.sol +++ b/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) { diff --git a/src/bin/solang.rs b/src/bin/solang.rs index 8283204df..f4cc4d809 100644 --- a/src/bin/solang.rs +++ b/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) { diff --git a/src/codegen/cfg.rs b/src/codegen/cfg.rs index acb88f61e..3e44ff2cb 100644 --- a/src/codegen/cfg.rs +++ b/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( + &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::>() .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, + 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); } diff --git a/src/codegen/constant_folding.rs b/src/codegen/constant_folding.rs index b9c0f10a5..1bd6ec4c9 100644 --- a/src/codegen/constant_folding.rs +++ b/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 diff --git a/src/codegen/mod.rs b/src/codegen/mod.rs index 9b73c9137..8211767f9 100644 --- a/src/codegen/mod.rs +++ b/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 } diff --git a/src/codegen/statements.rs b/src/codegen/statements.rs index 419c60a6e..eaa68a4a6 100644 --- a/src/codegen/statements.rs +++ b/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(¶m.loc, right, ¶m.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!(), } } diff --git a/src/codegen/strength_reduce.rs b/src/codegen/strength_reduce.rs index 5336490b4..6faa3d74d 100644 --- a/src/codegen/strength_reduce.rs +++ b/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(); diff --git a/src/codegen/undefined_variable.rs b/src/codegen/undefined_variable.rs new file mode 100644 index 000000000..90206eddd --- /dev/null +++ b/src/codegen/undefined_variable.rs @@ -0,0 +1,137 @@ +use crate::codegen::cfg::{ControlFlowGraph, Instr}; +use crate::codegen::reaching_definitions::{apply_transfers, VarDefs}; +use crate::parser::pt::{Loc, StorageLocation}; +use crate::sema::ast::{Diagnostic, ErrorType, Expression, Level, Namespace, Note}; +use crate::sema::symtable; +use std::collections::HashMap; + +/// We use this struct in expression.recurse function to provide all the +/// parameters for detecting undefined variables +pub struct FindUndefinedVariablesParams<'a> { + pub func_no: usize, + pub defs: &'a VarDefs, + pub ns: &'a mut Namespace, + pub cfg: &'a ControlFlowGraph, + pub diagnostics: &'a mut HashMap, +} + +/// This function traverses all the instructions of each block, apply transfers and +/// look for undefined variables. Returns true if undefined variables have been detected +pub fn find_undefined_variables( + cfg: &ControlFlowGraph, + ns: &mut Namespace, + func_no: usize, +) -> bool { + let mut diagnostics: HashMap = HashMap::new(); + for block in &cfg.blocks { + let mut var_defs: VarDefs = block.defs.clone(); + for (instr_no, instruction) in block.instr.iter().enumerate() { + check_variables_in_expression( + func_no, + instruction, + &var_defs, + ns, + cfg, + &mut diagnostics, + ); + apply_transfers(&block.transfers[instr_no], &mut var_defs); + } + } + + let mut all_diagnostics: Vec = + diagnostics.into_iter().map(|(_, diag)| diag).collect(); + ns.diagnostics.append(&mut all_diagnostics); + + !all_diagnostics.is_empty() +} + +/// Checks for undefined variables in an expression associated to an instruction +pub fn check_variables_in_expression( + func_no: usize, + instr: &Instr, + defs: &VarDefs, + ns: &mut Namespace, + cfg: &ControlFlowGraph, + diagnostics: &mut HashMap, +) { + if matches!(instr, Instr::Store { .. }) { + return; + } + + let mut params = FindUndefinedVariablesParams { + func_no, + defs, + ns, + cfg, + diagnostics, + }; + instr.recurse_expressions(&mut params, find_undefined_variables_in_expression); +} + +/// Auxiliar function for expression.recurse. It checks if a variable is read before being defined +pub fn find_undefined_variables_in_expression( + exp: &Expression, + ctx: &mut FindUndefinedVariablesParams, +) -> bool { + match &exp { + Expression::Variable(_, _, pos) => { + if let (Some(def_map), Some(var)) = ( + ctx.defs.get(pos), + ctx.ns.functions[ctx.func_no].symtable.vars.get(pos), + ) { + for (def, modified) in def_map { + if let Instr::Set { + expr: instr_expr, .. + } = &ctx.cfg.blocks[def.block_no].instr[def.instr_no] + { + // If an undefined definition reaches this read and the variable + // has not been modified since its definition, it is undefined + if matches!(instr_expr, Expression::Undefined(_)) && !*modified { + add_diagnostic(var, pos, &exp.loc(), ctx.diagnostics); + } + } + } + } + false + } + + // This is a method call whose array will never be undefined + Expression::DynamicArrayLength(..) => false, + + _ => true, + } +} + +/// Add a diagnostic or a note to the hashmap. This function prevents duplicate +/// error messages in Diagnotics +fn add_diagnostic( + var: &symtable::Variable, + var_no: &usize, + expr_loc: &Loc, + diagnostics: &mut HashMap, +) { + if matches!(var.usage_type, symtable::VariableUsage::ReturnVariable) + && !matches!(var.storage_location, Some(StorageLocation::Storage(_))) + { + return; + } + + if !diagnostics.contains_key(var_no) { + diagnostics.insert( + *var_no, + Diagnostic { + level: Level::Error, + ty: ErrorType::TypeError, + pos: Some(var.id.loc), + message: format!("Variable '{}' is undefined", var.id.name), + notes: vec![], + }, + ); + } + + let diag = diagnostics.get_mut(var_no).unwrap(); + diag.notes.push(Note { + pos: *expr_loc, + message: "Variable read before being defined".to_string(), + }); +} diff --git a/src/codegen/unused_variable.rs b/src/codegen/unused_variable.rs index be9cefdaf..78a760bd1 100644 --- a/src/codegen/unused_variable.rs +++ b/src/codegen/unused_variable.rs @@ -1,5 +1,4 @@ use crate::codegen::cfg::{ControlFlowGraph, Vartable}; -use crate::parser::pt; use crate::sema::ast::{Expression, Function, Namespace}; use crate::sema::symtable::VariableUsage; @@ -23,11 +22,7 @@ pub fn should_remove_assignment(ns: &Namespace, exp: &Expression, func: &Functio !var.read } - Expression::Variable(_, _, offset) => should_remove_variable( - offset, - func, - func.symtable.vars.get(offset).unwrap().initializer.as_ref(), - ), + Expression::Variable(_, _, offset) => should_remove_variable(offset, func), Expression::StructMember(_, _, str, _) => should_remove_assignment(ns, str, func), @@ -48,11 +43,7 @@ pub fn should_remove_assignment(ns: &Namespace, exp: &Expression, func: &Functio } /// Checks if we should remove a variable -pub fn should_remove_variable( - pos: &usize, - func: &Function, - initializer: Option<&Expression>, -) -> bool { +pub fn should_remove_variable(pos: &usize, func: &Function) -> bool { let var = &func.symtable.vars[pos]; //If the variable has never been read nor assigned, we can remove it right away. @@ -69,24 +60,8 @@ pub fn should_remove_variable( VariableUsage::DestructureVariable | VariableUsage::LocalVariable ) { - // If the variable has the memory or storage keyword, it can be a reference to another variable. - // In this case, an assigment may change the value of the variable it is referencing. - if !matches!( - var.storage_location, - Some(pt::StorageLocation::Memory(_)) | Some(pt::StorageLocation::Storage(_)) - ) { - return true; - } else if let Some(expr) = initializer { - // We can only remove variable with memory and storage keywords if the initializer is - // an array allocation, a constructor and a struct literal (only available in the local scope), - return matches!( - expr, - Expression::AllocDynamicArray(..) - | Expression::ArrayLiteral(..) - | Expression::Constructor { .. } - | Expression::StructLiteral(..) - ); - } + // Variables that are reference to other cannot be removed + return !var.is_reference(); } false diff --git a/src/emit/mod.rs b/src/emit/mod.rs index 63eb463d7..2e246dd02 100644 --- a/src/emit/mod.rs +++ b/src/emit/mod.rs @@ -3108,9 +3108,17 @@ pub trait TargetRuntime<'a> { .build_return(Some(&bin.return_values[&ReturnCode::Success])); } Instr::Set { res, expr, .. } => { - let value_ref = self.expression(bin, expr, &w.vars, function, ns); - - w.vars.get_mut(res).unwrap().value = value_ref; + if let Expression::Undefined(expr_type) = expr { + // If the variable has been declared as undefined, but we can + // initialize it with a default value + if let Some(default_expr) = expr_type.default(ns) { + w.vars.get_mut(res).unwrap().value = + self.expression(bin, &default_expr, &w.vars, function, ns); + } + } else { + w.vars.get_mut(res).unwrap().value = + self.expression(bin, expr, &w.vars, function, ns); + } } Instr::Branch { block: dest } => { let pos = bin.builder.get_insert_block().unwrap(); diff --git a/src/file_cache.rs b/src/file_cache.rs index 126390d69..cf905fc76 100644 --- a/src/file_cache.rs +++ b/src/file_cache.rs @@ -1,3 +1,5 @@ +use crate::parser::pt::Loc; +use crate::sema::ast; use std::collections::HashMap; use std::fs::File; use std::io::prelude::*; @@ -61,10 +63,10 @@ impl FileCache { /// Get file with contents. This must be a file which was previously /// add to the cache - pub fn get_file_contents(&mut self, file: &Path) -> Arc { + pub fn get_file_contents_and_number(&mut self, file: &Path) -> (Arc, usize) { let file_no = self.cached_paths[file]; - self.files[file_no].clone() + (self.files[file_no].clone(), file_no) } /// Populate the cache with absolute file path @@ -196,4 +198,38 @@ impl FileCache { Err(format!("file not found ‘{}’", filename)) } + + /// Get line and the target symbol's offset from loc + pub fn get_line_and_offset_from_loc( + &self, + file: &ast::File, + loc: &Loc, + ) -> (String, usize, usize, usize) { + let (beg_line_no, mut beg_offset) = file.offset_to_line_column(loc.1); + let (end_line_no, mut end_offset) = file.offset_to_line_column(loc.2); + let mut full_line = self.files[file.cache_no] + .lines() + .nth(beg_line_no) + .unwrap() + .to_owned(); + // If the loc spans across multiple lines, we concatenate them + if beg_line_no != end_line_no { + for i in beg_offset + 1..end_offset + 1 { + let line = self.files[file.cache_no].lines().nth(i).unwrap(); + if i == end_offset { + end_offset += full_line.len(); + } + full_line.push_str(line); + } + } + + let old_size = full_line.len(); + full_line = full_line.trim_start().parse().unwrap(); + // Calculate the size of the symbol we want to highlight + let size = end_offset - beg_offset; + // Update the offset after trimming the line + beg_offset -= old_size - full_line.len(); + + (full_line, beg_line_no, beg_offset, size) + } } diff --git a/src/sema/ast.rs b/src/sema/ast.rs index a65a3b3f1..fcbc3b516 100644 --- a/src/sema/ast.rs +++ b/src/sema/ast.rs @@ -368,6 +368,8 @@ pub struct File { pub path: PathBuf, /// Used for offset to line-column conversions pub line_starts: Vec, + /// Indicates the file number in FileCache.files + pub cache_no: usize, } /// When resolving a Solidity file, this holds all the resolved items @@ -628,12 +630,13 @@ pub enum Expression { args: Vec, }, InternalFunctionCfg(usize), + Undefined(Type), Poison, } impl Expression { - // Recurse over expression and copy each element through a filter. This allows the optimizer passes to create - // copies of expressions while modifying the results slightly + /// Recurse over expression and copy each element through a filter. This allows the optimizer passes to create + /// copies of expressions while modifying the results slightly pub fn copy_filter(&self, ctx: &mut T, filter: F) -> Expression where F: Fn(&Expression, &mut T) -> Expression, @@ -1180,6 +1183,85 @@ impl Expression { } } } + + /// Return the location for this expression + pub fn loc(&self) -> pt::Loc { + match self { + Expression::FunctionArg(loc, _, _) + | Expression::BoolLiteral(loc, _) + | Expression::BytesLiteral(loc, _, _) + | Expression::CodeLiteral(loc, _, _) + | Expression::NumberLiteral(loc, _, _) + | Expression::StructLiteral(loc, _, _) + | Expression::ArrayLiteral(loc, _, _, _) + | Expression::ConstArrayLiteral(loc, _, _, _) + | Expression::Add(loc, _, _, _, _) + | Expression::Subtract(loc, _, _, _, _) + | Expression::Multiply(loc, _, _, _, _) + | Expression::Divide(loc, _, _, _) + | Expression::Modulo(loc, _, _, _) + | Expression::Power(loc, _, _, _, _) + | Expression::BitwiseOr(loc, _, _, _) + | Expression::BitwiseAnd(loc, _, _, _) + | Expression::BitwiseXor(loc, _, _, _) + | Expression::ShiftLeft(loc, _, _, _) + | Expression::ShiftRight(loc, _, _, _, _) + | Expression::Variable(loc, _, _) + | Expression::ConstantVariable(loc, _, _, _) + | Expression::StorageVariable(loc, _, _, _) + | Expression::Load(loc, _, _) + | Expression::StorageLoad(loc, _, _) + | Expression::ZeroExt(loc, _, _) + | Expression::SignExt(loc, _, _) + | Expression::Trunc(loc, _, _) + | Expression::Cast(loc, _, _) + | Expression::BytesCast(loc, _, _, _) + | Expression::More(loc, _, _) + | Expression::Less(loc, _, _) + | Expression::MoreEqual(loc, _, _) + | Expression::LessEqual(loc, _, _) + | Expression::Equal(loc, _, _) + | Expression::NotEqual(loc, _, _) + | Expression::Not(loc, _) + | Expression::Complement(loc, _, _) + | Expression::UnaryMinus(loc, _, _) + | Expression::Ternary(loc, _, _, _, _) + | Expression::Subscript(loc, _, _, _) + | Expression::StructMember(loc, _, _, _) + | Expression::Or(loc, _, _) + | Expression::AllocDynamicArray(loc, _, _, _) + | Expression::DynamicArrayLength(loc, _) + | Expression::DynamicArraySubscript(loc, _, _, _) + | Expression::DynamicArrayPush(loc, _, _, _) + | Expression::DynamicArrayPop(loc, _, _) + | Expression::StorageBytesSubscript(loc, _, _) + | Expression::StorageArrayLength { loc, .. } + | Expression::StringCompare(loc, _, _) + | Expression::StringConcat(loc, _, _, _) + | Expression::Keccak256(loc, _, _) + | Expression::ReturnData(loc) + | Expression::InternalFunction { loc, .. } + | Expression::ExternalFunction { loc, .. } + | Expression::InternalFunctionCall { loc, .. } + | Expression::ExternalFunctionCall { loc, .. } + | Expression::ExternalFunctionCallRaw { loc, .. } + | Expression::Constructor { loc, .. } + | Expression::PreIncrement(loc, _, _, _) + | Expression::PreDecrement(loc, _, _, _) + | Expression::PostIncrement(loc, _, _, _) + | Expression::PostDecrement(loc, _, _, _) + | Expression::Builtin(loc, _, _, _) + | Expression::Assign(loc, _, _, _) + | Expression::List(loc, _) + | Expression::FormatString(loc, _) + | Expression::AbiEncode { loc, .. } + | Expression::InterfaceId(loc, ..) + | Expression::And(loc, _, _) => *loc, + Expression::InternalFunctionCfg(_) | Expression::Undefined(_) | Expression::Poison => { + unreachable!() + } + } + } } #[derive(PartialEq, Clone, Copy, Debug)] diff --git a/src/sema/diagnostics.rs b/src/sema/diagnostics.rs index 9d05202de..9af893044 100644 --- a/src/sema/diagnostics.rs +++ b/src/sema/diagnostics.rs @@ -1,4 +1,5 @@ use super::ast::{Diagnostic, ErrorType, Level, Namespace, Note}; +use crate::file_cache::FileCache; use crate::parser::pt::Loc; use serde::Serialize; @@ -130,11 +131,25 @@ impl Diagnostic { } } - fn formated_message(&self, ns: &Namespace) -> String { + fn formatted_message(&self, ns: &Namespace, cache: &FileCache) -> String { let mut s = if let Some(pos) = self.pos { let loc = ns.files[pos.0].loc_to_string(&pos); - format!("{}: {}: {}", loc, self.level.to_string(), self.message) + let (full_line, beg_line_no, beg_offset, type_size) = + cache.get_line_and_offset_from_loc(&ns.files[pos.0], &pos); + + format!( + "{}: {}: {}\nLine {}:\n\t{}\n\t{:-<7$}{:^<8$}", + loc, + self.level.to_string(), + self.message, + beg_line_no + 1, + full_line, + "", + "", + beg_offset, + type_size + ) } else { format!("solang: {}: {}", self.level.to_string(), self.message) }; @@ -142,20 +157,34 @@ impl Diagnostic { for note in &self.notes { let loc = ns.files[note.pos.0].loc_to_string(¬e.pos); - s.push_str(&format!("\n\t{}: {}: {}", loc, "note", note.message)); + let (full_line, beg_line_no, beg_offset, type_size) = + cache.get_line_and_offset_from_loc(&ns.files[note.pos.0], ¬e.pos); + + s.push_str(&format!( + "\n\t{}: {}: {}\n\tLine {}:\n\t\t{}\n\t\t{:-<7$}{:^<8$}", + loc, + "note", + note.message, + beg_line_no + 1, + full_line, + "", + "", + beg_offset, + type_size + )); } s } } -pub fn print_messages(ns: &Namespace, debug: bool) { +pub fn print_messages(cache: &FileCache, ns: &Namespace, debug: bool) { for msg in &ns.diagnostics { if !debug && msg.level == Level::Debug { continue; } - eprintln!("{}", msg.formated_message(ns)); + eprintln!("{}", msg.formatted_message(ns, cache)); } } @@ -183,7 +212,7 @@ pub struct OutputJson { pub formattedMessage: String, } -pub fn message_as_json(ns: &Namespace) -> Vec { +pub fn message_as_json(ns: &Namespace, cache: &FileCache) -> Vec { let mut json = Vec::new(); for msg in &ns.diagnostics { @@ -203,7 +232,7 @@ pub fn message_as_json(ns: &Namespace) -> Vec { component: "general".to_owned(), severity: msg.level.to_string().to_owned(), message: msg.message.to_owned(), - formattedMessage: msg.formated_message(ns), + formattedMessage: msg.formatted_message(ns, cache), }); } diff --git a/src/sema/expression.rs b/src/sema/expression.rs index 83a2cfae0..64ebfc7ba 100644 --- a/src/sema/expression.rs +++ b/src/sema/expression.rs @@ -30,83 +30,6 @@ use crate::Target; use base58::{FromBase58, FromBase58Error}; impl Expression { - /// Return the location for this expression - pub fn loc(&self) -> pt::Loc { - match self { - Expression::FunctionArg(loc, _, _) - | Expression::BoolLiteral(loc, _) - | Expression::BytesLiteral(loc, _, _) - | Expression::CodeLiteral(loc, _, _) - | Expression::NumberLiteral(loc, _, _) - | Expression::StructLiteral(loc, _, _) - | Expression::ArrayLiteral(loc, _, _, _) - | Expression::ConstArrayLiteral(loc, _, _, _) - | Expression::Add(loc, _, _, _, _) - | Expression::Subtract(loc, _, _, _, _) - | Expression::Multiply(loc, _, _, _, _) - | Expression::Divide(loc, _, _, _) - | Expression::Modulo(loc, _, _, _) - | Expression::Power(loc, _, _, _, _) - | Expression::BitwiseOr(loc, _, _, _) - | Expression::BitwiseAnd(loc, _, _, _) - | Expression::BitwiseXor(loc, _, _, _) - | Expression::ShiftLeft(loc, _, _, _) - | Expression::ShiftRight(loc, _, _, _, _) - | Expression::Variable(loc, _, _) - | Expression::ConstantVariable(loc, _, _, _) - | Expression::StorageVariable(loc, _, _, _) - | Expression::Load(loc, _, _) - | Expression::StorageLoad(loc, _, _) - | Expression::ZeroExt(loc, _, _) - | Expression::SignExt(loc, _, _) - | Expression::Trunc(loc, _, _) - | Expression::Cast(loc, _, _) - | Expression::BytesCast(loc, _, _, _) - | Expression::More(loc, _, _) - | Expression::Less(loc, _, _) - | Expression::MoreEqual(loc, _, _) - | Expression::LessEqual(loc, _, _) - | Expression::Equal(loc, _, _) - | Expression::NotEqual(loc, _, _) - | Expression::Not(loc, _) - | Expression::Complement(loc, _, _) - | Expression::UnaryMinus(loc, _, _) - | Expression::Ternary(loc, _, _, _, _) - | Expression::Subscript(loc, _, _, _) - | Expression::StructMember(loc, _, _, _) - | Expression::Or(loc, _, _) - | Expression::AllocDynamicArray(loc, _, _, _) - | Expression::DynamicArrayLength(loc, _) - | Expression::DynamicArraySubscript(loc, _, _, _) - | Expression::DynamicArrayPush(loc, _, _, _) - | Expression::DynamicArrayPop(loc, _, _) - | Expression::StorageBytesSubscript(loc, _, _) - | Expression::StorageArrayLength { loc, .. } - | Expression::StringCompare(loc, _, _) - | Expression::StringConcat(loc, _, _, _) - | Expression::Keccak256(loc, _, _) - | Expression::ReturnData(loc) - | Expression::InternalFunction { loc, .. } - | Expression::ExternalFunction { loc, .. } - | Expression::InternalFunctionCall { loc, .. } - | Expression::ExternalFunctionCall { loc, .. } - | Expression::ExternalFunctionCallRaw { loc, .. } - | Expression::Constructor { loc, .. } - | Expression::PreIncrement(loc, ..) - | Expression::PreDecrement(loc, ..) - | Expression::PostIncrement(loc, ..) - | Expression::PostDecrement(loc, ..) - | Expression::Builtin(loc, _, _, _) - | Expression::Assign(loc, _, _, _) - | Expression::List(loc, _) - | Expression::FormatString(loc, _) - | Expression::AbiEncode { loc, .. } - | Expression::InterfaceId(loc, ..) - | Expression::And(loc, _, _) => *loc, - Expression::InternalFunctionCfg(_) | Expression::Poison => unreachable!(), - } - } - /// Return the type for this expression. This assumes the expression has a single value, /// panics will occur otherwise pub fn ty(&self) -> Type { @@ -201,7 +124,7 @@ impl Expression { Expression::ReturnData(_) => Type::DynamicBytes, Expression::InternalFunction { ty, .. } => ty.clone(), Expression::ExternalFunction { ty, .. } => ty.clone(), - Expression::InternalFunctionCfg(_) => unreachable!(), + Expression::InternalFunctionCfg(_) | Expression::Undefined(_) => unreachable!(), } } @@ -4405,6 +4328,7 @@ pub fn assign_single( None, )?; assigned_variable(ns, &var, symtable); + let var_ty = var.ty(); let val = expression( right, diff --git a/src/sema/file.rs b/src/sema/file.rs index ff47d4c25..fb2c24202 100644 --- a/src/sema/file.rs +++ b/src/sema/file.rs @@ -3,7 +3,7 @@ use crate::parser::pt::Loc; use std::path::PathBuf; impl File { - pub fn new(path: PathBuf, contents: &str) -> Self { + pub fn new(path: PathBuf, contents: &str, cache_no: usize) -> Self { let mut line_starts = Vec::new(); for (ind, c) in contents.char_indices() { @@ -12,7 +12,11 @@ impl File { } } - File { path, line_starts } + File { + path, + line_starts, + cache_no, + } } /// Give a position as a human readable position diff --git a/src/sema/mod.rs b/src/sema/mod.rs index 4c0c22857..4aa27d8cc 100644 --- a/src/sema/mod.rs +++ b/src/sema/mod.rs @@ -54,10 +54,13 @@ pub fn sema(file: &ResolvedFile, cache: &mut FileCache, ns: &mut ast::Namespace) fn sema_file(file: &ResolvedFile, cache: &mut FileCache, ns: &mut ast::Namespace) { let file_no = ns.files.len(); - let source_code = cache.get_file_contents(&file.full_path); + let (source_code, file_cache_no) = cache.get_file_contents_and_number(&file.full_path); - ns.files - .push(ast::File::new(file.full_path.clone(), &source_code)); + ns.files.push(ast::File::new( + file.full_path.clone(), + &source_code, + file_cache_no, + )); let pt = match parse(&source_code, file_no) { Ok(s) => s, diff --git a/src/sema/printer.rs b/src/sema/printer.rs index 44fc606eb..ea3abe758 100644 --- a/src/sema/printer.rs +++ b/src/sema/printer.rs @@ -507,6 +507,7 @@ fn print_expr(e: &Expression, func: Option<&Function>, ns: &Namespace) -> Tree { Expression::InternalFunctionCfg(..) | Expression::ReturnData(..) | Expression::Poison + | Expression::Undefined(_) | Expression::AbiEncode { .. } | Expression::Keccak256(..) => { panic!("should not present in ast"); diff --git a/src/sema/symtable.rs b/src/sema/symtable.rs index adebf80e3..4ea652977 100644 --- a/src/sema/symtable.rs +++ b/src/sema/symtable.rs @@ -20,6 +20,31 @@ pub struct Variable { pub storage_location: Option, } +impl Variable { + pub fn is_reference(&self) -> bool { + // If the variable has the memory or storage keyword, it can be a reference to another variable. + // In this case, an assigment may change the value of the variable it is referencing. + if matches!( + self.storage_location, + Some(pt::StorageLocation::Memory(_)) | Some(pt::StorageLocation::Storage(_)) + ) { + if let Some(expr) = &self.initializer { + // If the initializer is an array allocation, a constructor or a struct literal, + // the variable is not a reference to another. + return !matches!( + expr, + Expression::AllocDynamicArray(..) + | Expression::ArrayLiteral(..) + | Expression::Constructor { .. } + | Expression::StructLiteral(..) + ); + } + } + + false + } +} + #[derive(Clone)] pub enum VariableUsage { Parameter, diff --git a/src/sema/unused_variable.rs b/src/sema/unused_variable.rs index b63caa4c6..fb02bba2a 100644 --- a/src/sema/unused_variable.rs +++ b/src/sema/unused_variable.rs @@ -296,7 +296,9 @@ pub fn emit_warning_local_variable(variable: &symtable::Variable) -> Option Option TestRuntime { false, ); - diagnostics::print_messages(&ns, false); + diagnostics::print_messages(&cache, &ns, false); for v in &res { println!("contract size:{}", v.0.len()); diff --git a/tests/solana.rs b/tests/solana.rs index ac0100b16..d9a03760f 100644 --- a/tests/solana.rs +++ b/tests/solana.rs @@ -119,7 +119,7 @@ fn build_solidity(src: &str) -> VirtualMachine { codegen(contract_no, &mut ns, &Options::default()); } - diagnostics::print_messages(&ns, false); + diagnostics::print_messages(&cache, &ns, false); let context = inkwell::context::Context::create(); diff --git a/tests/substrate.rs b/tests/substrate.rs index 99e071641..c697d98e5 100644 --- a/tests/substrate.rs +++ b/tests/substrate.rs @@ -1201,7 +1201,7 @@ pub fn build_solidity(src: &'static str) -> TestRuntime { false, ); - diagnostics::print_messages(&ns, false); + diagnostics::print_messages(&cache, &ns, false); no_errors(ns.diagnostics); assert!(!res.is_empty()); @@ -1238,7 +1238,7 @@ pub fn build_solidity_with_overflow_check(src: &'static str) -> TestRuntime { true, ); - diagnostics::print_messages(&ns, false); + diagnostics::print_messages(&cache, &ns, false); assert!(!res.is_empty()); diff --git a/tests/substrate_tests/enums.rs b/tests/substrate_tests/enums.rs index ce12c0552..10b2aa252 100644 --- a/tests/substrate_tests/enums.rs +++ b/tests/substrate_tests/enums.rs @@ -29,6 +29,7 @@ fn weekdays() { assert(int8(Weekday.Sunday) == 6); Weekday x; + x = Weekday.Monday; assert(uint(x) == 0); diff --git a/tests/substrate_tests/structs.rs b/tests/substrate_tests/structs.rs index 1d9c8c940..53f286623 100644 --- a/tests/substrate_tests/structs.rs +++ b/tests/substrate_tests/structs.rs @@ -311,7 +311,7 @@ fn structs_as_ref_args() { } function test() public { - foo f; + foo f = foo(false, 2); func(f); diff --git a/tests/undefined_variable_detection.rs b/tests/undefined_variable_detection.rs new file mode 100644 index 000000000..6631be79e --- /dev/null +++ b/tests/undefined_variable_detection.rs @@ -0,0 +1,746 @@ +use solang::codegen::{codegen, Options}; +use solang::file_cache::FileCache; +use solang::sema::ast::Diagnostic; +use solang::sema::ast::{Level, Namespace}; +use solang::{parse_and_resolve, Target}; + +fn parse_and_codegen(src: &'static str) -> Namespace { + let mut cache = FileCache::new(); + cache.set_file_contents("test.sol", src.to_string()); + let mut ns = parse_and_resolve("test.sol", &mut cache, Target::Generic); + + let opt = Options { + dead_storage: false, + constant_folding: false, + strength_reduce: false, + vector_to_slice: false, + }; + + for contract_no in 0..ns.contracts.len() { + codegen(contract_no, &mut ns, &opt); + } + + ns +} + +fn get_errors(ns: &Namespace) -> Vec<&Diagnostic> { + let mut vec = Vec::new(); + for diag in &ns.diagnostics { + if matches!(diag.level, Level::Error) { + vec.push(diag); + } + } + vec +} + +fn contains_error_message_and_notes( + errors: &[&Diagnostic], + message: &str, + notes_no: usize, +) -> bool { + for error in errors { + if error.message == message { + return error.notes.len() == notes_no; + } + } + + false +} + +#[test] +fn used_before_being_defined() { + let file = r#" + contract Test { + bytes byteArr; + bytes32 baRR; + + function get() public { + string memory s = "Test"; + byteArr = bytes(s); + uint16 a = 1; + uint8 b; + b = uint8(a); + + uint256 c; + c = b; + bytes32 b32; + bytes memory char = bytes(bytes32(uint(a) * 2 ** (8 * b))); + baRR = bytes32(c); + bytes32 cdr = bytes32(char); + assert(b32 == baRR); + if(b32 != cdr) { + + } + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 'b32' is undefined"); + assert_eq!(errors[0].notes.len(), 2); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + assert_eq!( + errors[0].notes[1].message, + "Variable read before being defined" + ); +} + +#[test] +fn struct_as_ref() { + let file = r#" + contract test_struct_parsing { + struct foo { + bool x; + uint32 y; + } + + function func(foo f) private { + // assigning to f members dereferences f + f.x = true; + f.y = 64; + + // assigning to f changes the reference + f = foo({ x: false, y: 256 }); + + // f no longer point to f in caller function + f.x = false; + f.y = 98123; + } + + function test() public { + foo f; + + func(f); + + assert(f.x == true); + assert(f.y == 64); + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 'f' is undefined"); + assert_eq!(errors[0].notes.len(), 3); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + assert_eq!( + errors[0].notes[1].message, + "Variable read before being defined" + ); + assert_eq!( + errors[0].notes[2].message, + "Variable read before being defined" + ); + + let file = r#" + contract test_struct_parsing { + struct foo { + bool x; + uint32 y; + } + + function func(foo f) private { + // assigning to f members dereferences f + f.x = true; + f.y = 64; + + // assigning to f changes the reference + f = foo({ x: false, y: 256 }); + + // f no longer point to f in caller function + f.x = false; + f.y = 98123; + } + + function test() public { + foo f = foo(false, 2); + + func(f); + + assert(f.x == true); + assert(f.y == 64); + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 0); +} + +#[test] +fn while_loop() { + let file = r#" + contract testing { + function test(int x) public pure returns (string) { + string s; + while(x > 0){ + s = "testing_string"; + x--; + } + + return s; + } + } + "#; + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 's' is undefined"); + assert_eq!(errors[0].notes.len(), 1); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + + let file = r#" + contract testing { + function test(int x) public pure returns (string) { + string s; + while(x > 0){ + s = "testing_string"; + x--; + } + + if(x < 0) { + s = "another_test"; + } + + return s; + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 's' is undefined"); + assert_eq!(errors[0].notes.len(), 1); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + + let file = r#" + contract testing { + function test(int x) public pure returns (string) { + string s; + while(x > 0){ + s = "testing_string"; + x--; + } + + if(x < 0) { + s = "another_test"; + } else { + s = "should_work"; + } + + return s; + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 0); +} + +#[test] +fn for_loop() { + let file = r#" + contract testing { + function test(int x) public pure returns (int) { + int s; + for(int i=0; i 0); + + + return o.a; + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 0); +} + +#[test] +fn if_else_condition() { + let file = r#" + contract testing { + struct other { + int a; + } + function test(int x) public pure returns (int) { + other o; + if(x > 0) { + o = other(2); + } + + return o.a; + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 'o' is undefined"); + assert_eq!(errors[0].notes.len(), 1); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + + let file = r#" + contract testing { + struct other { + int a; + } + function test(int x) public pure returns (int) { + other o; + if(x > 0) { + x += 2; + } else if(x < 0) { + o = other(2); + } else { + x++; + } + + return o.a; + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 'o' is undefined"); + assert_eq!(errors[0].notes.len(), 1); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + + let file = r#" + contract testing { + struct other { + int a; + } + function test(int x) public pure returns (int) { + other o; + if(x > 0) { + o = other(2); + } else { + o = other(2); + } + + return o.a; + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 0); +} + +#[test] +fn array() { + let file = r#" + contract test { + + function testing(int x) public pure returns (int) { + int[] vec; + + return vec[0]; + } + } + "#; + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 'vec' is undefined"); + assert_eq!(errors[0].notes.len(), 1); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + + let file = r#" + contract test { + + function testing(int x) public pure returns (int) { + int[] vec; + + if(x > 0) { + vec.push(2); + } + + return vec[0]; + } + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 0); +} + +#[test] +fn contract_and_enum() { + let file = r#" +contract other { + int public a; + + function testing() public returns (int) { + return 2; + } +} + + +contract test { + enum FreshJuiceSize{ SMALL, MEDIUM, LARGE } + function testing(int x) public returns (int) { + other o; + FreshJuiceSize choice; + if(x > 0 && o.testing() < 5) { + o = new other(); + } + + assert(choice == FreshJuiceSize.LARGE); + + return o.a(); + } +} + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'o' is undefined", + 2 + )); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'choice' is undefined", + 1 + )); +} + +#[test] +fn basic_types() { + let file = r#" + contract test { + + function testing(int x) public returns (address, int, uint, bool, bytes, int) { + address a; + int i; + uint u; + bool b; + bytes bt; + int[5] vec; + + while(x > 0) { + x--; + a = address(this); + i = -2; + u = 2; + b = true; + bt = hex"1234"; + vec[0] = 2; + } + return (a, i, u, b, bt, vec[1]); + } +} + "#; + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'bt' is undefined", + 1 + )); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'b' is undefined", + 1 + )); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'a' is undefined", + 1 + )); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'i' is undefined", + 1 + )); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'u' is undefined", + 1 + )); +} + +#[test] +fn nested_branches() { + let file = r#" + +contract test { + + function testing(int x) public returns (int) { + int i; + + while(x > 0) { + int b; + if(x > 5) { + b = 2; + } + + i = b; + } + int a; + if(x < 5) { + if(x < 2) { + a = 2; + } else { + a = 1; + } + } else if(x < 4) { + a = 5; + } + + return i + a; + } +} + "#; + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'b' is undefined", + 1 + )); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'a' is undefined", + 1 + )); + assert!(contains_error_message_and_notes( + &errors, + "Variable 'i' is undefined", + 2 + )); +} + +#[test] +fn try_catch() { + //TODO: Fix this test case + + // let file = r#" + // contract AddNumbers { function add(uint256 a, uint256 b) external pure returns (uint256 c) {c = b;} } + // contract Example { + // AddNumbers addContract; + // event StringFailure(string stringFailure); + // event BytesFailure(bytes bytesFailure); + // + // function exampleFunction(uint256 _a, uint256 _b) public returns (bytes c) { + // bytes r; + // try addContract.add(_a, _b) returns (uint256 _value) { + // r = hex"ABCD"; + // return r; + // } catch Error(string memory _err) { + // r = hex"ABCD"; + // emit StringFailure(_err); + // } catch (bytes memory _err) { + // emit BytesFailure(_err); + // } + // + // return r; + // } + // + // } + // "#; + // + // let ns = parse_and_codegen(file); + // let errors = get_errors(&ns); + // assert_eq!(errors.len(), 1); + // assert_eq!(errors[0].message, "Variable 'r' is undefined"); + // assert_eq!(errors[0].notes.len(), 1); + // assert_eq!(errors[0].notes[0].message, "Variable read before being defined"); + + let file = r#" + contract AddNumbers { function add(uint256 a, uint256 b) external pure returns (uint256 c) {c = b;} } + contract Example { + AddNumbers addContract; + event StringFailure(string stringFailure); + event BytesFailure(bytes bytesFailure); + + function exampleFunction(uint256 _a, uint256 _b) public returns (bytes c) { + bytes r; + try addContract.add(_a, _b) returns (uint256 _value) { + r = hex"ABCD"; + return r; + } catch Error(string memory _err) { + r = hex"ABCD"; + emit StringFailure(_err); + } catch (bytes memory _err) { + r = hex"ABCD"; + emit BytesFailure(_err); + } + + return r; + } + + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 0); + + let file = r#" + contract AddNumbers { function add(uint256 a, uint256 b) external pure returns (uint256 c) {c = b;} } + contract Example { + AddNumbers addContract; + event StringFailure(string stringFailure); + event BytesFailure(bytes bytesFailure); + + function exampleFunction(uint256 _a, uint256 _b) public returns (bytes c) { + bytes r; + try addContract.add(_a, _b) returns (uint256 _value) { + return r; + } catch Error(string memory _err) { + r = hex"ABCD"; + emit StringFailure(_err); + } catch (bytes memory _err) { + emit BytesFailure(_err); + } + + return r; + } + + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 'r' is undefined"); + assert_eq!(errors[0].notes.len(), 1); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); + + let file = r#" + contract AddNumbers { function add(uint256 a, uint256 b) external pure returns (uint256 c) {c = b;} } + contract Example { + AddNumbers addContract; + event StringFailure(string stringFailure); + event BytesFailure(bytes bytesFailure); + + function exampleFunction(uint256 _a, uint256 _b) public returns (bytes c) { + bytes r; + try addContract.add(_a, _b) returns (uint256 _value) { + r = hex"ABCD"; + return r; + } catch Error(string memory _err) { + emit StringFailure(_err); + } catch (bytes memory _err) { + emit BytesFailure(_err); + } + + return r; + } + + } + "#; + + let ns = parse_and_codegen(file); + let errors = get_errors(&ns); + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].message, "Variable 'r' is undefined"); + assert_eq!(errors[0].notes.len(), 1); + assert_eq!( + errors[0].notes[0].message, + "Variable read before being defined" + ); +} diff --git a/tests/unused_variable_detection.rs b/tests/unused_variable_detection.rs index 08e6ee612..316ac0c6d 100644 --- a/tests/unused_variable_detection.rs +++ b/tests/unused_variable_detection.rs @@ -376,15 +376,11 @@ fn subscript() { "#; let ns = generic_target_parse(file); - assert_eq!(count_warnings(&ns.diagnostics), 5); + assert_eq!(count_warnings(&ns.diagnostics), 4); assert!(assert_message_in_warnings( &ns.diagnostics, "local variable 'arr4' has been assigned, but never read" )); - assert!(assert_message_in_warnings( - &ns.diagnostics, - "local variable 'arr5' has been assigned, but never read" - )); assert!(assert_message_in_warnings( &ns.diagnostics, "storage variable 'arr1' has been assigned, but never read" @@ -464,11 +460,7 @@ fn assign_trunc_cast() { "#; let ns = generic_target_parse(file); - assert_eq!(count_warnings(&ns.diagnostics), 2); - assert!(assert_message_in_warnings( - &ns.diagnostics, - "local variable 'b32' has never been assigned a value, but has been read" - )); + assert_eq!(count_warnings(&ns.diagnostics), 1); assert!(assert_message_in_warnings( &ns.diagnostics, "storage variable 'byteArr' has been assigned, but never read" @@ -966,7 +958,7 @@ fn builtin_call_destructure() { uint128 b = 1; uint64 g = 2; address payable ad = payable(address(this)); - bytes memory by; + bytes memory by = hex"AB2"; (p, ) = ad.call{value: b, gas: g}(by); uint c = 1; abi.encodeWithSignature("hey", c); @@ -980,11 +972,7 @@ fn builtin_call_destructure() { "#; let ns = generic_target_parse(file); - assert_eq!(count_warnings(&ns.diagnostics), 1); - assert!(assert_message_in_warnings( - &ns.diagnostics, - "local variable 'by' has never been assigned a value, but has been read" - )); + assert_eq!(count_warnings(&ns.diagnostics), 0); } #[test]