From 57071d7544d5db16e9e08348f90fe75b11a11e21 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Thu, 28 Nov 2024 19:22:18 +0100 Subject: [PATCH] feat: validate variable data locations (#149) * feat: add VarKind * feat: validate variable data locations * tests * note * tests --- crates/ast/src/ast/item.rs | 8 + crates/ast/src/ast/ty.rs | 6 + crates/data-structures/src/fmt.rs | 82 ++++++++ crates/data-structures/src/lib.rs | 39 +--- crates/parse/src/parser/item.rs | 10 +- crates/parse/src/parser/mod.rs | 47 +---- crates/sema/src/ast_lowering/lower.rs | 29 ++- crates/sema/src/ast_lowering/resolve.rs | 128 ++++++++---- crates/sema/src/hir.rs | 185 ++++++++++++++++-- crates/sema/src/ty/mod.rs | 166 +++++++++++++--- crates/sema/src/ty/ty.rs | 10 + tests/ui/parser/transient.sol | 6 +- tests/ui/parser/transient.stderr | 14 +- tests/ui/typeck/issue_128_library_mapping.sol | 15 ++ ...ue_129.sol => issue_129_nested_struct.sol} | 0 tests/ui/typeck/mapping_structs.sol | 4 + tests/ui/typeck/mapping_structs.stderr | 50 ++++- .../typeck/recursive_function_parameter.sol | 12 +- .../recursive_function_parameter.stderr | 4 +- tests/ui/typeck/recursive_types.sol | 12 +- tests/ui/typeck/recursive_types.stderr | 6 +- tests/ui/typeck/var_loc_contract_fns.sol | 75 +++++++ tests/ui/typeck/var_loc_contract_fns.stderr | 114 +++++++++++ tests/ui/typeck/var_loc_file_level.sol | 8 + tests/ui/typeck/var_loc_file_level.stderr | 30 +++ tests/ui/typeck/var_loc_state.sol | 10 + tests/ui/typeck/var_loc_state.stderr | 33 ++++ tools/tester/src/solc/solidity.rs | 8 + 28 files changed, 897 insertions(+), 214 deletions(-) create mode 100644 crates/data-structures/src/fmt.rs create mode 100644 tests/ui/typeck/issue_128_library_mapping.sol rename tests/ui/typeck/{nested_struct_issue_129.sol => issue_129_nested_struct.sol} (100%) create mode 100644 tests/ui/typeck/var_loc_contract_fns.sol create mode 100644 tests/ui/typeck/var_loc_contract_fns.stderr create mode 100644 tests/ui/typeck/var_loc_file_level.sol create mode 100644 tests/ui/typeck/var_loc_file_level.stderr create mode 100644 tests/ui/typeck/var_loc_state.sol create mode 100644 tests/ui/typeck/var_loc_state.stderr diff --git a/crates/ast/src/ast/item.rs b/crates/ast/src/ast/item.rs index a85c7338..d31768c3 100644 --- a/crates/ast/src/ast/item.rs +++ b/crates/ast/src/ast/item.rs @@ -499,6 +499,14 @@ impl DataLocation { Self::Calldata => "calldata", } } + + /// Returns the string representation of the storage location, or `"none"` if `None`. + pub const fn opt_to_str(this: Option) -> &'static str { + match this { + Some(location) => location.to_str(), + None => "none", + } + } } // How a function can mutate the EVM state. diff --git a/crates/ast/src/ast/ty.rs b/crates/ast/src/ast/ty.rs index f843bb2c..cd32a61c 100644 --- a/crates/ast/src/ast/ty.rs +++ b/crates/ast/src/ast/ty.rs @@ -188,6 +188,12 @@ impl ElementaryType { | Self::FixedBytes(..) ) } + + /// Returns `true` if the type is a reference type. + #[inline] + pub const fn is_reference_type(self) -> bool { + matches!(self, Self::String | Self::Bytes) + } } /// Byte size of a fixed-bytes, integer, or fixed-point number (M) type. Valid values: 0..=32. diff --git a/crates/data-structures/src/fmt.rs b/crates/data-structures/src/fmt.rs new file mode 100644 index 00000000..05daf4bc --- /dev/null +++ b/crates/data-structures/src/fmt.rs @@ -0,0 +1,82 @@ +use std::{cell::Cell, fmt}; + +/// Wrapper for [`fmt::from_fn`]. +#[cfg(feature = "nightly")] +pub fn from_fn) -> fmt::Result>( + f: F, +) -> impl fmt::Debug + fmt::Display { + fmt::from_fn(f) +} + +/// Polyfill for [`fmt::from_fn`]. +#[cfg(not(feature = "nightly"))] +pub fn from_fn) -> fmt::Result>( + f: F, +) -> impl fmt::Debug + fmt::Display { + struct FromFn(F); + + impl fmt::Debug for FromFn + where + F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, + { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (self.0)(f) + } + } + + impl fmt::Display for FromFn + where + F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, + { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + (self.0)(f) + } + } + + FromFn(f) +} + +/// Returns `list` formatted as a comma-separated list with "or" before the last item. +pub fn or_list(list: I) -> impl fmt::Display +where + I: IntoIterator, +{ + let list = Cell::new(Some(list.into_iter())); + from_fn(move |f| { + let list = list.take().expect("or_list called twice"); + let len = list.len(); + for (i, t) in list.enumerate() { + if i > 0 { + let is_last = i == len - 1; + f.write_str(if len > 2 && is_last { + ", or " + } else if len == 2 && is_last { + " or " + } else { + ", " + })?; + } + write!(f, "{t}")?; + } + Ok(()) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_or_list() { + let tests: &[(&[&str], &str)] = &[ + (&[], ""), + (&["``"], "``"), + (&["integer", "identifier"], "integer or identifier"), + (&["path", "string literal", "`&&`"], "path, string literal, or `&&`"), + (&["`&&`", "`||`", "`&&`", "`||`"], "`&&`, `||`, `&&`, or `||`"), + ]; + for &(tokens, expected) in tests { + assert_eq!(or_list(tokens).to_string(), expected, "{tokens:?}"); + } + } +} diff --git a/crates/data-structures/src/lib.rs b/crates/data-structures/src/lib.rs index 4fc220a4..77606548 100644 --- a/crates/data-structures/src/lib.rs +++ b/crates/data-structures/src/lib.rs @@ -10,9 +10,8 @@ #![cfg_attr(feature = "nightly", feature(rustc_attrs))] #![cfg_attr(feature = "nightly", allow(internal_features))] -use std::fmt; - pub mod cycle; +pub mod fmt; pub mod hint; pub mod index; pub mod map; @@ -42,39 +41,3 @@ pub use smallvec; pub fn outline(f: impl FnOnce() -> R) -> R { f() } - -/// Wrapper for [`fmt::from_fn`]. -#[cfg(feature = "nightly")] -pub fn fmt_from_fn) -> fmt::Result>( - f: F, -) -> impl fmt::Debug + fmt::Display { - fmt::from_fn(f) -} - -/// Polyfill for [`fmt::from_fn`]; -#[cfg(not(feature = "nightly"))] -pub fn fmt_from_fn) -> fmt::Result>( - f: F, -) -> impl fmt::Debug + fmt::Display { - struct FromFn(F); - - impl fmt::Debug for FromFn - where - F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, - { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - (self.0)(f) - } - } - - impl fmt::Display for FromFn - where - F: Fn(&mut fmt::Formatter<'_>) -> fmt::Result, - { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - (self.0)(f) - } - } - - FromFn(f) -} diff --git a/crates/parse/src/parser/item.rs b/crates/parse/src/parser/item.rs index 332a8b1c..6235b31a 100644 --- a/crates/parse/src/parser/item.rs +++ b/crates/parse/src/parser/item.rs @@ -738,12 +738,7 @@ impl<'sess, 'ast> Parser<'sess, 'ast> { let mut indexed = false; loop { if let Some(s) = self.parse_data_location() { - let transient = matches!(s, DataLocation::Transient); - let transient_allowed = flags.contains(VarFlags::TRANSIENT); - if transient && !transient_allowed { - let msg = "`transient` data location is not allowed here"; - self.dcx().err(msg).span(self.prev_token.span).emit(); - } else if !(transient || flags.contains(VarFlags::DATALOC)) { + if !flags.contains(VarFlags::DATALOC) { let msg = "data locations are not allowed here"; self.dcx().err(msg).span(self.prev_token.span).emit(); } else if data_location.is_some() { @@ -984,7 +979,6 @@ bitflags::bitflags! { pub(super) struct VarFlags: u16 { // `ty` is always required. `name` is always optional, unless `NAME` is specified. - const TRANSIENT = 1 << 0; const DATALOC = 1 << 1; const INDEXED = 1 << 2; @@ -1015,7 +1009,7 @@ bitflags::bitflags! { const FUNCTION_TY = Self::DATALOC.bits() | Self::NAME_WARN.bits(); // https://docs.soliditylang.org/en/latest/grammar.html#a4.SolidityParser.stateVariableDeclaration - const STATE_VAR = Self::TRANSIENT.bits() + const STATE_VAR = Self::DATALOC.bits() | Self::PRIVATE.bits() | Self::INTERNAL.bits() | Self::PUBLIC.bits() diff --git a/crates/parse/src/parser/mod.rs b/crates/parse/src/parser/mod.rs index 68647fa0..3b4326a1 100644 --- a/crates/parse/src/parser/mod.rs +++ b/crates/parse/src/parser/mod.rs @@ -5,16 +5,13 @@ use solar_ast::{ token::{Delimiter, Token, TokenKind}, AstPath, Box, DocComment, DocComments, PathSlice, }; -use solar_data_structures::BumpExt; +use solar_data_structures::{fmt::or_list, BumpExt}; use solar_interface::{ diagnostics::DiagCtxt, source_map::{FileName, SourceFile}, Ident, Result, Session, Span, Symbol, }; -use std::{ - fmt::{self, Write}, - path::Path, -}; +use std::{fmt, path::Path}; mod expr; mod item; @@ -79,7 +76,7 @@ impl fmt::Display for ExpectedToken { impl ExpectedToken { fn to_string_many(tokens: &[Self]) -> String { - or_list(tokens) + or_list(tokens).to_string() } fn eq_kind(&self, other: &TokenKind) -> bool { @@ -978,41 +975,3 @@ impl<'sess, 'ast> Parser<'sess, 'ast> { self.expected_ident_found(false).unwrap_err() } } - -fn or_list(list: &[T]) -> String { - let len = list.len(); - let mut s = String::with_capacity(16 * len); - for (i, t) in list.iter().enumerate() { - if i > 0 { - let is_last = i == len - 1; - s.push_str(if len > 2 && is_last { - ", or " - } else if len == 2 && is_last { - " or " - } else { - ", " - }); - } - let _ = write!(s, "{t}"); - } - s -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_or_list() { - let tests: &[(&[&str], &str)] = &[ - (&[], ""), - (&["``"], "``"), - (&["integer", "identifier"], "integer or identifier"), - (&["path", "string literal", "`&&`"], "path, string literal, or `&&`"), - (&["`&&`", "`||`", "`&&`", "`||`"], "`&&`, `||`, `&&`, or `||`"), - ]; - for &(tokens, expected) in tests { - assert_eq!(or_list(tokens), expected, "{tokens:?}"); - } - } -} diff --git a/crates/sema/src/ast_lowering/lower.rs b/crates/sema/src/ast_lowering/lower.rs index 7a1a2811..85443bed 100644 --- a/crates/sema/src/ast_lowering/lower.rs +++ b/crates/sema/src/ast_lowering/lower.rs @@ -103,7 +103,14 @@ impl<'ast> super::LoweringContext<'_, 'ast, '_> { } ast::ItemKind::Contract(i) => hir::ItemId::Contract(self.lower_contract(item, i)), ast::ItemKind::Function(i) => hir::ItemId::Function(self.lower_function(item, i)), - ast::ItemKind::Variable(i) => hir::ItemId::Variable(self.lower_variable(i)), + ast::ItemKind::Variable(i) => { + let kind = if self.current_contract_id.is_some() { + hir::VarKind::State + } else { + hir::VarKind::Global + }; + hir::ItemId::Variable(self.lower_variable(i, kind)) + } ast::ItemKind::Struct(i) => hir::ItemId::Struct(self.lower_struct(item, i)), ast::ItemKind::Enum(i) => hir::ItemId::Enum(self.lower_enum(item, i)), ast::ItemKind::Udvt(i) => hir::ItemId::Udvt(self.lower_udvt(item, i)), @@ -154,13 +161,18 @@ impl<'ast> super::LoweringContext<'_, 'ast, '_> { }) } - fn lower_variable(&mut self, i: &ast::VariableDefinition<'_>) -> hir::VariableId { + fn lower_variable( + &mut self, + i: &ast::VariableDefinition<'_>, + kind: hir::VarKind, + ) -> hir::VariableId { lower_variable_partial( &mut self.hir, i, self.current_source_id, self.current_contract_id, - self.current_contract_id.is_some(), + None, + kind, ) } @@ -231,7 +243,8 @@ pub(super) fn lower_variable_partial( i: &ast::VariableDefinition<'_>, source: SourceId, contract: Option, - is_state_variable: bool, + function: Option, + kind: hir::VarKind, ) -> hir::VariableId { // handled later: ty, override_, initializer let ast::VariableDefinition { @@ -248,7 +261,9 @@ pub(super) fn lower_variable_partial( let id = hir.variables.push(hir::Variable { source, contract, + function, span, + kind, ty: hir::Type::DUMMY, name, visibility, @@ -258,7 +273,6 @@ pub(super) fn lower_variable_partial( overrides: &[], indexed, initializer: None, - is_state_variable, getter: None, }); let v = hir.variable(id); @@ -272,7 +286,9 @@ fn generate_partial_getter(hir: &mut hir::Hir<'_>, id: hir::VariableId) -> hir:: let hir::Variable { source, contract, + function: _, span, + kind, ty: _, name, visibility, @@ -282,13 +298,12 @@ fn generate_partial_getter(hir: &mut hir::Hir<'_>, id: hir::VariableId) -> hir:: overrides, indexed, initializer: _, - is_state_variable, getter, } = *hir.variable(id); debug_assert!(!indexed); debug_assert!(data_location.is_none()); debug_assert_eq!(visibility, Some(ast::Visibility::Public)); - debug_assert!(is_state_variable); + debug_assert!(kind.is_state()); debug_assert!(getter.is_none()); hir.functions.push(hir::Function { source, diff --git a/crates/sema/src/ast_lowering/resolve.rs b/crates/sema/src/ast_lowering/resolve.rs index 2ae81c77..5980002e 100644 --- a/crates/sema/src/ast_lowering/resolve.rs +++ b/crates/sema/src/ast_lowering/resolve.rs @@ -236,19 +236,20 @@ impl super::LoweringContext<'_, '_, '_> { } } -impl super::LoweringContext<'_, '_, '_> { +impl<'hir> super::LoweringContext<'_, '_, 'hir> { #[instrument(level = "debug", skip_all)] pub(super) fn resolve_symbols(&mut self) { let next_id = &AtomicUsize::new(0); macro_rules! mk_resolver { ($e:expr) => { - mk_resolver!(@scopes SymbolResolverScopes::new_in($e.source, $e.contract)) + mk_resolver!(@scopes SymbolResolverScopes::new_in($e.source, $e.contract), None) }; - (@scopes $scopes:expr) => { + (@scopes $scopes:expr, $f:expr) => { ResolveContext { scopes: $scopes, + function_id: $f, sess: self.sess, arena: self.arena, hir: &mut self.hir, @@ -271,7 +272,8 @@ impl super::LoweringContext<'_, '_, '_> { let ast::ItemKind::Struct(ast_struct) = &ast_item.kind else { unreachable!() }; let strukt = self.hir.strukt(id); let mut cx = mk_resolver!(strukt); - self.hir.structs[id].fields = cx.lower_variables(ast_struct.fields); + self.hir.structs[id].fields = + cx.lower_variables(ast_struct.fields, hir::VarKind::Struct); } for id in self.hir.error_ids() { @@ -279,7 +281,8 @@ impl super::LoweringContext<'_, '_, '_> { let ast::ItemKind::Error(ast_error) = &ast_item.kind else { unreachable!() }; let error = self.hir.error(id); let mut cx = mk_resolver!(error); - self.hir.errors[id].parameters = cx.lower_variables(ast_error.parameters); + self.hir.errors[id].parameters = + cx.lower_variables(ast_error.parameters, hir::VarKind::Error); } for id in self.hir.event_ids() { @@ -287,7 +290,8 @@ impl super::LoweringContext<'_, '_, '_> { let ast::ItemKind::Event(ast_event) = &ast_item.kind else { unreachable!() }; let event = self.hir.event(id); let mut cx = mk_resolver!(event); - self.hir.events[id].parameters = cx.lower_variables(ast_event.parameters); + self.hir.events[id].parameters = + cx.lower_variables(ast_event.parameters, hir::VarKind::Event); } // Resolve constants and state variables. @@ -368,13 +372,11 @@ impl super::LoweringContext<'_, '_, '_> { self.arena.alloc_smallvec(overrides) }; - let mut cx = ResolveContext::new(self, scopes, next_id); - cx.hir.functions[id].parameters = cx.arena.alloc_slice_fill_iter( - ast_func.header.parameters.iter().map(|param| cx.lower_variable(param).0), - ); - cx.hir.functions[id].returns = cx.arena.alloc_slice_fill_iter( - ast_func.header.returns.iter().map(|ret| cx.lower_variable(ret).0), - ); + let mut cx = ResolveContext::new(self, scopes, next_id, Some(id)); + cx.hir.functions[id].parameters = + cx.lower_variables(ast_func.header.parameters, hir::VarKind::FunctionParam); + cx.hir.functions[id].returns = + cx.lower_variables(ast_func.header.returns, hir::VarKind::FunctionReturn); if let Some(body) = &ast_func.body { cx.hir.functions[id].body = Some(cx.lower_stmts(body)); } @@ -396,7 +398,7 @@ impl super::LoweringContext<'_, '_, '_> { let ast::ItemKind::Variable(ast_var) = &ast_item.kind else { unreachable!() }; let scopes = SymbolResolverScopes::new_in(var.source, var.contract); - let mut cx = ResolveContext::new(self, scopes, next_id); + let mut cx = ResolveContext::new(self, scopes, next_id, None); let init = ast_var.initializer.as_deref().map(|init| cx.lower_expr(init)); let ty = cx.lower_type(&ast_var.ty); self.hir.variables[id].initializer = init; @@ -448,6 +450,9 @@ impl super::LoweringContext<'_, '_, '_> { let mut ret_ty = &self.hir.variable(gettee).ty; let mut ret_name = None; let mut parameters = SmallVec::<[_; 8]>::new(); + let new_param = |this: &mut Self, ty, name| { + this.mk_var(Some(id), span, ty, name, hir::VarKind::FunctionParam) + }; for i in 0usize.. { // let index_name = || Some(Ident::new(Symbol::intern(&format!("index{i}")), span)); let _ = i; @@ -456,8 +461,10 @@ impl super::LoweringContext<'_, '_, '_> { // mapping(k => v) -> arguments += k, ret_ty = v hir::TypeKind::Mapping(map) => { let name = map.key_name.or_else(index_name); - let mut param = hir::Variable::new(map.key.clone(), name); - param.data_location = Some(hir::DataLocation::Calldata); + let mut param = new_param(self, map.key.clone(), name); + if param.ty.kind.is_reference_type() { + param.data_location = Some(hir::DataLocation::Calldata); + } parameters.push(self.hir.variables.push(param)); ret_ty = &map.value; ret_name = map.value_name; @@ -470,8 +477,8 @@ impl super::LoweringContext<'_, '_, '_> { )), span: ret_ty.span, }; - let var = hir::Variable::new(u256, index_name()); - parameters.push(self.hir.variables.push(var)); + let param = new_param(self, u256, index_name()); + parameters.push(self.hir.variables.push(param)); ret_ty = &array.element; } _ => break, @@ -481,8 +488,10 @@ impl super::LoweringContext<'_, '_, '_> { let mut returns = SmallVec::<[_; 8]>::new(); let mut push_return = |this: &mut Self, ty, name| { - let mut ret = hir::Variable::new(ty, name); - ret.data_location = Some(hir::DataLocation::Memory); + let mut ret = this.mk_var(Some(id), span, ty, name, hir::VarKind::FunctionReturn); + if ret.ty.kind.is_reference_type() { + ret.data_location = Some(hir::DataLocation::Memory); + } returns.push(this.hir.variables.push(ret)) }; let mut ret_struct = None; @@ -539,7 +548,7 @@ impl super::LoweringContext<'_, '_, '_> { } else { // `Struct storage = ;` let decl_name = Ident::new(sym::__tmp_struct, ast_var.span); - let mut decl_var = hir::Variable::new(ret_ty.clone(), Some(decl_name)); + let mut decl_var = self.mk_var_stmt(id, span, ret_ty.clone(), decl_name); decl_var.data_location = Some(hir::DataLocation::Storage); let decl_id = self.hir.variables.push(decl_var); let decl_stmt = mk_stmt(hir::StmtKind::DeclSingle(decl_id)); @@ -566,6 +575,32 @@ impl super::LoweringContext<'_, '_, '_> { } } + fn mk_var( + &mut self, + function: Option, + span: Span, + ty: hir::Type<'hir>, + name: Option, + kind: hir::VarKind, + ) -> hir::Variable<'hir> { + hir::Variable { + contract: self.current_contract_id, + function, + span, + ..hir::Variable::new(self.current_source_id, ty, name, kind) + } + } + + fn mk_var_stmt( + &mut self, + function: hir::FunctionId, + span: Span, + ty: hir::Type<'hir>, + name: Ident, + ) -> hir::Variable<'hir> { + self.mk_var(Some(function), span, ty, Some(name), hir::VarKind::Statement) + } + fn declare_kind_in( &self, scope: &mut Declarations, @@ -592,6 +627,7 @@ struct ResolveContext<'sess, 'hir, 'a> { hir: &'a mut hir::Hir<'hir>, resolver: &'a SymbolResolver<'sess>, scopes: SymbolResolverScopes, + function_id: Option, next_id: &'a AtomicUsize, } @@ -600,6 +636,7 @@ impl<'sess, 'hir, 'a> ResolveContext<'sess, 'hir, 'a> { lcx: &'a mut super::LoweringContext<'sess, '_, 'hir>, scopes: SymbolResolverScopes, next_id: &'a AtomicUsize, + function_id: Option, ) -> Self { Self { sess: lcx.sess, @@ -607,6 +644,7 @@ impl<'sess, 'hir, 'a> ResolveContext<'sess, 'hir, 'a> { hir: &mut lcx.hir, resolver: &lcx.resolver, scopes, + function_id, next_id, } } @@ -662,14 +700,16 @@ impl<'sess, 'hir, 'a> ResolveContext<'sess, 'hir, 'a> { #[instrument(name = "lower_stmt", level = "debug", skip_all)] fn lower_stmt_full(&mut self, stmt: &ast::Stmt<'_>) -> hir::Stmt<'hir> { let kind = match &stmt.kind { - ast::StmtKind::DeclSingle(var) => match self.lower_variable(var) { - (id, Ok(())) => hir::StmtKind::DeclSingle(id), - (_, Err(guar)) => hir::StmtKind::Err(guar), - }, + ast::StmtKind::DeclSingle(var) => { + match self.lower_variable(var, hir::VarKind::Statement) { + (id, Ok(())) => hir::StmtKind::DeclSingle(id), + (_, Err(guar)) => hir::StmtKind::Err(guar), + } + } ast::StmtKind::DeclMulti(vars, expr) => hir::StmtKind::DeclMulti( - self.arena.alloc_slice_fill_iter( - vars.iter().map(|var| var.as_ref().map(|var| self.lower_variable(var).0)), - ), + self.arena.alloc_slice_fill_iter(vars.iter().map(|var| { + var.as_ref().map(|var| self.lower_variable(var, hir::VarKind::Statement).0) + })), self.lower_expr(expr), ), ast::StmtKind::Assembly(_) => hir::StmtKind::Err( @@ -705,12 +745,12 @@ impl<'sess, 'hir, 'a> ResolveContext<'sess, 'hir, 'a> { ast::StmtKind::Try(ast::StmtTry { expr, returns, block, catch }) => { hir::StmtKind::Try(self.arena.alloc(hir::StmtTry { expr: self.lower_expr_full(expr), - returns: self.lower_variables(returns), + returns: self.lower_variables(returns, hir::VarKind::TryCatch), block: self.lower_block(block), catch: self.arena.alloc_slice_fill_iter(catch.iter().map(|catch| { hir::CatchClause { name: catch.name, - args: self.lower_variables(catch.args), + args: self.lower_variables(catch.args, hir::VarKind::TryCatch), block: self.lower_block(catch.block), } })), @@ -721,21 +761,27 @@ impl<'sess, 'hir, 'a> ResolveContext<'sess, 'hir, 'a> { hir::Stmt { span: stmt.span, kind } } - fn lower_variables(&mut self, vars: &[ast::VariableDefinition<'_>]) -> &'hir [hir::VariableId] { - self.arena.alloc_slice_fill_iter(vars.iter().map(|var| self.lower_variable(var).0)) + fn lower_variables( + &mut self, + vars: &[ast::VariableDefinition<'_>], + kind: hir::VarKind, + ) -> &'hir [hir::VariableId] { + self.arena.alloc_slice_fill_iter(vars.iter().map(|var| self.lower_variable(var, kind).0)) } /// Lowers `var` to HIR and declares it in the current scope. fn lower_variable( &mut self, var: &ast::VariableDefinition<'_>, + kind: hir::VarKind, ) -> (hir::VariableId, Result<(), ErrorGuaranteed>) { let id = super::lower::lower_variable_partial( self.hir, var, self.scopes.source.unwrap(), self.scopes.contract, - false, + self.function_id, + kind, ); self.hir.variables[id].ty = self.lower_type(&var.ty); self.hir.variables[id].initializer = self.lower_expr_opt(var.initializer.as_deref()); @@ -943,18 +989,14 @@ impl<'sess, 'hir, 'a> ResolveContext<'sess, 'hir, 'a> { element: self.lower_type(&array.element), size: self.lower_expr_opt(array.size.as_deref()), })), - ast::TypeKind::Function(f) => hir::TypeKind::Function( - self.arena.alloc(hir::TypeFunction { - parameters: self - .arena - .alloc_slice_fill_iter(f.parameters.iter().map(|p| self.lower_type(&p.ty))), + ast::TypeKind::Function(f) => { + hir::TypeKind::Function(self.arena.alloc(hir::TypeFunction { + parameters: self.lower_variables(f.parameters, hir::VarKind::FunctionTyParam), visibility: f.visibility.unwrap_or(ast::Visibility::Public), state_mutability: f.state_mutability, - returns: self - .arena - .alloc_slice_fill_iter(f.returns.iter().map(|p| self.lower_type(&p.ty))), - }), - ), + returns: self.lower_variables(f.returns, hir::VarKind::FunctionTyReturn), + })) + } ast::TypeKind::Mapping(mapping) => { hir::TypeKind::Mapping(self.arena.alloc(hir::TypeMapping { key: self.lower_type(&mapping.key), diff --git a/crates/sema/src/hir.rs b/crates/sema/src/hir.rs index df80a3f3..fcbb04f6 100644 --- a/crates/sema/src/hir.rs +++ b/crates/sema/src/hir.rs @@ -687,11 +687,15 @@ pub struct Variable<'hir> { pub source: SourceId, /// The contract this variable is defined in, if any. pub contract: Option, - /// The variable span. + /// The function this variable is defined in, if any. + pub function: Option, + /// The variable's span. pub span: Span, - /// The variable type. + /// The kind of variable. + pub kind: VarKind, + /// The variable's type. pub ty: Type<'hir>, - /// The variable name. + /// The variable's name. pub name: Option, /// The visibility of the variable. pub visibility: Option, @@ -701,18 +705,19 @@ pub struct Variable<'hir> { pub overrides: &'hir [ContractId], pub indexed: bool, pub initializer: Option<&'hir Expr<'hir>>, - pub is_state_variable: bool, /// The compiler-generated getter function, if any. pub getter: Option, } impl<'hir> Variable<'hir> { /// Creates a new variable. - pub fn new(ty: Type<'hir>, name: Option) -> Self { + pub fn new(source: SourceId, ty: Type<'hir>, name: Option, kind: VarKind) -> Self { Self { - source: SourceId::MAX, + source, contract: None, + function: None, span: Span::DUMMY, + kind, ty, name, visibility: None, @@ -722,14 +727,99 @@ impl<'hir> Variable<'hir> { overrides: &[], indexed: false, initializer: None, - is_state_variable: false, getter: None, } } + /// Creates a new variable statement. + pub fn new_stmt( + source: SourceId, + contract: ContractId, + function: FunctionId, + ty: Type<'hir>, + name: Ident, + ) -> Self { + Self { + contract: Some(contract), + function: Some(function), + ..Self::new(source, ty, Some(name), VarKind::Statement) + } + } + + /// Returns the description of the variable. + pub fn description(&self) -> &'static str { + self.kind.to_str() + } + + /// Returns `true` if the variable is [`constant`](VarMut::Constant). + pub fn is_constant(&self) -> bool { + self.mutability == Some(VarMut::Constant) + } + + /// Returns `true` if the variable is [`immutable`](VarMut::Immutable). + pub fn is_immutable(&self) -> bool { + self.mutability == Some(VarMut::Immutable) + } + + pub fn is_l_value(&self) -> bool { + !self.is_constant() + } + + pub fn is_struct_member(&self) -> bool { + matches!(self.kind, VarKind::Struct) + } + + pub fn is_event_or_error_parameter(&self) -> bool { + matches!(self.kind, VarKind::Event | VarKind::Error) + } + + pub fn is_local_variable(&self) -> bool { + matches!( + self.kind, + VarKind::FunctionTyParam + | VarKind::FunctionTyReturn + | VarKind::Event + | VarKind::Error + | VarKind::FunctionParam + | VarKind::FunctionReturn + | VarKind::Statement + | VarKind::TryCatch + ) + } + + pub fn is_callable_or_catch_parameter(&self) -> bool { + matches!( + self.kind, + VarKind::Event + | VarKind::Error + | VarKind::FunctionParam + | VarKind::FunctionTyParam + | VarKind::FunctionReturn + | VarKind::FunctionTyReturn + | VarKind::TryCatch + ) + } + + pub fn is_local_or_return(&self) -> bool { + self.is_return_parameter() + || (self.is_local_variable() && !self.is_callable_or_catch_parameter()) + } + + pub fn is_return_parameter(&self) -> bool { + matches!(self.kind, VarKind::FunctionReturn | VarKind::FunctionTyReturn) + } + + pub fn is_try_catch_parameter(&self) -> bool { + matches!(self.kind, VarKind::TryCatch) + } + /// Returns `true` if the variable is a state variable. pub fn is_state_variable(&self) -> bool { - self.is_state_variable + self.kind.is_state() + } + + pub fn is_file_level_variable(&self) -> bool { + matches!(self.kind, VarKind::Global) } /// Returns `true` if the variable is public. @@ -738,6 +828,49 @@ impl<'hir> Variable<'hir> { } } +/// The kind of variable. +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, EnumIs)] +pub enum VarKind { + /// Defined at the top level. + Global, + /// Defined in a contract. + State, + /// Defined in a struct. + Struct, + /// Defined in an event. + Event, + /// Defined in an error. + Error, + /// Defined as a function parameter. + FunctionParam, + /// Defined as a function return. + FunctionReturn, + /// Defined as a function type parameter. + FunctionTyParam, + /// Defined as a function type return. + FunctionTyReturn, + /// Defined as a statement, inside of a function, block or `for` statement. + Statement, + /// Defined in a catch clause. + TryCatch, +} + +impl VarKind { + pub fn to_str(self) -> &'static str { + match self { + Self::Global => "file-level variable", + Self::State => "state variable", + Self::Struct => "struct field", + Self::Event => "event parameter", + Self::Error => "error parameter", + Self::FunctionParam | Self::FunctionTyParam => "function parameter", + Self::FunctionReturn | Self::FunctionTyReturn => "function return parameter", + Self::Statement => "variable", + Self::TryCatch => "try/catch clause", + } + } +} + /// A block of statements. pub type Block<'hir> = &'hir [Stmt<'hir>]; @@ -1061,7 +1194,7 @@ pub struct Type<'hir> { pub kind: TypeKind<'hir>, } -impl Type<'_> { +impl<'hir> Type<'hir> { /// Dummy placeholder type. pub const DUMMY: Self = Self { span: Span::DUMMY, kind: TypeKind::Err(ErrorGuaranteed::new_unchecked()) }; @@ -1071,23 +1204,27 @@ impl Type<'_> { self.span == Span::DUMMY && matches!(self.kind, TypeKind::Err(_)) } - pub fn visit(&self, f: &mut impl FnMut(&Self) -> ControlFlow) -> ControlFlow { + pub fn visit( + &self, + hir: &Hir<'hir>, + f: &mut impl FnMut(&Self) -> ControlFlow, + ) -> ControlFlow { f(self)?; match self.kind { TypeKind::Elementary(_) => ControlFlow::Continue(()), - TypeKind::Array(ty) => ty.element.visit(f), + TypeKind::Array(ty) => ty.element.visit(hir, f), TypeKind::Function(ty) => { - for ty in ty.parameters { - ty.visit(f)?; + for ¶m in ty.parameters { + hir.variable(param).ty.visit(hir, f)?; } - for ty in ty.returns { - ty.visit(f)?; + for &ret in ty.returns { + hir.variable(ret).ty.visit(hir, f)?; } ControlFlow::Continue(()) } TypeKind::Mapping(ty) => { - ty.key.visit(f)?; - ty.value.visit(f) + ty.key.visit(hir, f)?; + ty.value.visit(hir, f) } TypeKind::Custom(_) => ControlFlow::Continue(()), TypeKind::Err(_) => ControlFlow::Continue(()), @@ -1119,6 +1256,16 @@ impl TypeKind<'_> { pub fn is_elementary(&self) -> bool { matches!(self, Self::Elementary(_)) } + + /// Returns `true` if the type is a reference type. + #[inline] + pub fn is_reference_type(&self) -> bool { + match self { + TypeKind::Elementary(t) => t.is_reference_type(), + TypeKind::Custom(ItemId::Struct(_)) | TypeKind::Array(_) => true, + _ => false, + } + } } /// An array type. @@ -1131,10 +1278,10 @@ pub struct TypeArray<'hir> { /// A function type name. #[derive(Debug)] pub struct TypeFunction<'hir> { - pub parameters: &'hir [Type<'hir>], + pub parameters: &'hir [VariableId], pub visibility: Visibility, pub state_mutability: StateMutability, - pub returns: &'hir [Type<'hir>], + pub returns: &'hir [VariableId], } /// A mapping type. diff --git a/crates/sema/src/ty/mod.rs b/crates/sema/src/ty/mod.rs index 936d65ce..2191df0e 100644 --- a/crates/sema/src/ty/mod.rs +++ b/crates/sema/src/ty/mod.rs @@ -3,12 +3,13 @@ use crate::{ builtins::{members, Builtin}, hir::{self, Hir}, }; -use alloy_primitives::{keccak256, Selector, B256}; +use alloy_primitives::{keccak256, Selector, B256, U256}; use either::Either; use solar_ast::{DataLocation, StateMutability, TypeSize, Visibility}; use solar_data_structures::{ - fmt_from_fn, + fmt::{from_fn, or_list}, map::{FxBuildHasher, FxHashMap, FxHashSet}, + smallvec::SmallVec, BumpExt, }; use solar_interface::{ @@ -274,7 +275,7 @@ impl<'gcx> Gcx<'gcx> { fn item_canonical_name_(self, id: hir::ItemId) -> impl fmt::Display { let name = self.item_name(id); let contract = self.hir.item(id).contract().map(|id| self.item_name(id)); - fmt_from_fn(move |f| { + from_fn(move |f| { if let Some(contract) = contract { write!(f, "{contract}.")?; } @@ -287,7 +288,7 @@ impl<'gcx> Gcx<'gcx> { self, id: hir::ContractId, ) -> impl fmt::Display + use<'gcx> { - fmt_from_fn(move |f| { + from_fn(move |f| { let c = self.hir.contract(id); let source = self.hir.source(c.source); write!(f, "{}:{}", source.file.name.display(), c.name) @@ -400,26 +401,23 @@ impl<'gcx> Gcx<'gcx> { Ok(int) => { if int.data.is_zero() { let msg = "array length must be greater than zero"; - self.dcx().err(msg).span(size.span).emit(); + let guar = self.dcx().err(msg).span(size.span).emit(); + TyKind::Array(self.mk_ty_err(guar), int.data) + } else { + TyKind::Array(ty, int.data) } - TyKind::Array(ty, int.data) } - Err(guar) => TyKind::Err(guar), + Err(guar) => TyKind::Array(self.mk_ty_err(guar), U256::from(1)), }, None => TyKind::DynArray(ty), } } - hir::TypeKind::Function(f) => { - let parameters = - self.mk_ty_iter(f.parameters.iter().map(|ty| self.type_of_hir_ty(ty))); - let returns = self.mk_ty_iter(f.returns.iter().map(|ty| self.type_of_hir_ty(ty))); - TyKind::FnPtr(self.interner.intern_ty_fn_ptr(TyFnPtr { - parameters, - returns, - state_mutability: f.state_mutability, - visibility: f.visibility, - })) - } + hir::TypeKind::Function(f) => TyKind::FnPtr(self.interner.intern_ty_fn_ptr(TyFnPtr { + parameters: self.mk_item_tys(f.parameters), + returns: self.mk_item_tys(f.returns), + state_mutability: f.state_mutability, + visibility: f.visibility, + })), hir::TypeKind::Mapping(mapping) => { let key = self.type_of_hir_ty(&mapping.key); let value = self.type_of_hir_ty(&mapping.value); @@ -530,6 +528,12 @@ pub fn interface_functions(gcx: _, id: hir::ContractId) -> InterfaceFunctions<'g continue; } if !ty.can_be_exported() { + // TODO: implement `interfaceType` + if c.kind.is_library() { + result = Err(ErrorGuaranteed::new_unchecked()); + continue; + } + let msg = if ty.has_mapping() { "types containing mappings cannot be parameter or return types of public functions" } else if ty.is_recursive() { @@ -537,7 +541,7 @@ pub fn interface_functions(gcx: _, id: hir::ContractId) -> InterfaceFunctions<'g } else { "this type cannot be parameter or return type of a public function" }; - let span = gcx.item_span(var_id); + let span = gcx.hir.variable(var_id).ty.span; result = Err(gcx.dcx().err(msg).span(span).emit()); } } @@ -600,11 +604,7 @@ pub fn type_of_item(gcx: _, id: hir::ItemId) -> Ty<'gcx> { hir::ItemId::Variable(id) => { let var = gcx.hir.variable(id); let ty = gcx.type_of_hir_ty(&var.ty); - match (var.contract, var.data_location) { - (_, Some(loc)) => TyKind::Ref(ty, loc), - (Some(_), None) => TyKind::Ref(ty, DataLocation::Storage), - (None, None) => return ty, - } + return var_type(gcx, var, ty); } hir::ItemId::Struct(id) => TyKind::Struct(id), hir::ItemId::Enum(id) => TyKind::Enum(id), @@ -672,7 +672,7 @@ pub fn struct_recursiveness(gcx: _, id: hir::StructId) -> Recursiveness { ty = &array.element; } cdr_try!(check(ty, dynamic)); - if let ControlFlow::Break(r) = field.ty.visit(&mut |ty| check(ty, true).to_controlflow()) { + if let ControlFlow::Break(r) = field.ty.visit(&gcx.hir, &mut |ty| check(ty, true).to_controlflow()) { return r; } } @@ -695,6 +695,122 @@ pub fn members_of(gcx: _, ty: Ty<'gcx>) -> members::MemberList<'gcx> { } } +fn var_type<'gcx>(gcx: Gcx<'gcx>, var: &'gcx hir::Variable<'gcx>, ty: Ty<'gcx>) -> Ty<'gcx> { + use hir::DataLocation::*; + + // https://github.com/ethereum/solidity/blob/48d40d5eaf97c835cf55896a7a161eedc57c57f9/libsolidity/ast/AST.cpp#L820 + let has_reference_or_mapping_type = ty.is_reference_type() || ty.has_mapping(); + let mut func_vis = None; + let mut locs; + let allowed: &[_] = if var.is_state_variable() { + &[None, Some(Transient)] + } else if !has_reference_or_mapping_type || var.is_event_or_error_parameter() { + &[None] + } else if var.is_callable_or_catch_parameter() { + locs = SmallVec::<[_; 3]>::new(); + locs.push(Some(Memory)); + let mut is_constructor_parameter = false; + if let Some(f) = var.function { + let f = gcx.hir.function(f); + is_constructor_parameter = f.kind.is_constructor(); + if !var.is_try_catch_parameter() && !is_constructor_parameter { + func_vis = Some(f.visibility); + } + if is_constructor_parameter + || f.visibility <= hir::Visibility::Internal + || f.contract.is_some_and(|c| gcx.hir.contract(c).kind.is_library()) + { + locs.push(Some(Storage)); + } + } + if !var.is_try_catch_parameter() && !is_constructor_parameter { + locs.push(Some(Calldata)); + } + &locs + } else if var.is_local_variable() { + &[Some(Memory), Some(Storage), Some(Calldata)] + } else { + &[None] + }; + + let mut var_loc = var.data_location; + if !allowed.contains(&var_loc) { + if ty.has_error().is_ok() { + let msg = if !has_reference_or_mapping_type { + "data location can only be specified for array, struct or mapping types".to_string() + } else if let Some(var_loc) = var_loc { + format!("invalid data location `{var_loc}`") + } else { + "expected data location".to_string() + }; + let mut err = gcx.dcx().err(msg).span(var.span); + if has_reference_or_mapping_type { + let note = format!( + "data location must be {expected} for {vis}{descr}{got}", + expected = or_list( + allowed.iter().map(|d| format!("`{}`", DataLocation::opt_to_str(*d))) + ), + vis = if let Some(vis) = func_vis { format!("{vis} ") } else { String::new() }, + descr = var.description(), + got = if let Some(var_loc) = var_loc { + format!(", but got `{var_loc}`") + } else { + String::new() + }, + ); + err = err.note(note); + } + err.emit(); + } + var_loc = allowed[0]; + } + + let ty_loc = if var.is_event_or_error_parameter() || var.is_file_level_variable() { + Memory + } else if var.is_state_variable() { + let mut_specified = var.mutability.is_some(); + match var_loc { + None => { + if mut_specified { + Memory + } else { + Storage + } + } + Some(Transient) => { + if mut_specified { + let msg = "transient cannot be used as data location for constant or immutable variables"; + gcx.dcx().err(msg).span(var.span).emit(); + } + if var.initializer.is_some() { + let msg = + "initialization of transient storage state variables is not supported"; + gcx.dcx().err(msg).span(var.span).emit(); + } + Transient + } + Some(_) => unreachable!(), + } + } else if var.is_struct_member() { + Storage + } else { + match var_loc { + Some(loc @ (Memory | Storage | Calldata)) => loc, + Some(Transient) => unimplemented!(), + None => { + assert!(!has_reference_or_mapping_type, "data location not properly set"); + Memory + } + } + }; + + if ty.is_reference_type() { + ty.with_loc(gcx, ty_loc) + } else { + ty + } +} + /// `OnceMap::insert` but with `Copy` keys and values. fn cache_insert( map: &once_map::OnceMap, diff --git a/crates/sema/src/ty/ty.rs b/crates/sema/src/ty/ty.rs index 9992ed6c..9445cdd0 100644 --- a/crates/sema/src/ty/ty.rs +++ b/crates/sema/src/ty/ty.rs @@ -133,6 +133,16 @@ impl<'gcx> Ty<'gcx> { } } + /// Returns `true` if the type is a reference type. + #[inline] + pub fn is_reference_type(self) -> bool { + match self.kind { + TyKind::Elementary(t) => t.is_reference_type(), + TyKind::Struct(_) | TyKind::Array(..) | TyKind::DynArray(_) => true, + _ => false, + } + } + /// Returns `true` if the type is recursive. pub fn is_recursive(self) -> bool { self.flags.contains(TyFlags::IS_RECURSIVE) diff --git a/tests/ui/parser/transient.sol b/tests/ui/parser/transient.sol index b1862ae8..658b429a 100644 --- a/tests/ui/parser/transient.sol +++ b/tests/ui/parser/transient.sol @@ -23,7 +23,11 @@ contract E { transient = 4; } - function g(uint256 transient transient) { //~ ERROR: not allowed here + function g(uint256 transient transient) { //~ ERROR: data location can only be specified for array, struct or mapping types + transient = 5; + } + + function g2(uint256[] transient transient) { //~ ERROR: invalid data location transient = 5; } } diff --git a/tests/ui/parser/transient.stderr b/tests/ui/parser/transient.stderr index 593e1b5b..5c8574b8 100644 --- a/tests/ui/parser/transient.stderr +++ b/tests/ui/parser/transient.stderr @@ -1,9 +1,17 @@ -error: `transient` data location is not allowed here +error: data location can only be specified for array, struct or mapping types --> ROOT/tests/ui/parser/transient.sol:LL:CC | LL | function g(uint256 transient transient) { - | ^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | -error: aborting due to 1 previous error +error: invalid data location `transient` + --> ROOT/tests/ui/parser/transient.sol:LL:CC + | +LL | function g2(uint256[] transient transient) { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: data location must be `memory` or `calldata` for public function parameter, but got `transient` + +error: aborting due to 2 previous errors diff --git a/tests/ui/typeck/issue_128_library_mapping.sol b/tests/ui/typeck/issue_128_library_mapping.sol new file mode 100644 index 00000000..88ecc03b --- /dev/null +++ b/tests/ui/typeck/issue_128_library_mapping.sol @@ -0,0 +1,15 @@ +// https://github.com/paradigmxyz/solar/issues/128 + +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.13; + +struct StructTest { + uint256 a; + mapping(uint256 index => uint256) data; +} + +library TestLibrary { + function testFunction(StructTest memory testParameter) external view returns (uint256) { + return testParameter.a; + } +} diff --git a/tests/ui/typeck/nested_struct_issue_129.sol b/tests/ui/typeck/issue_129_nested_struct.sol similarity index 100% rename from tests/ui/typeck/nested_struct_issue_129.sol rename to tests/ui/typeck/issue_129_nested_struct.sol diff --git a/tests/ui/typeck/mapping_structs.sol b/tests/ui/typeck/mapping_structs.sol index e5d26a28..51fc74b5 100644 --- a/tests/ui/typeck/mapping_structs.sol +++ b/tests/ui/typeck/mapping_structs.sol @@ -5,8 +5,10 @@ contract C { function f1(S memory) public {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions function f2(S storage) public {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions + //~^ ERROR: invalid data location function f3() public returns(S memory) {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions function f4() public returns(S storage) {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions + //~^ ERROR: invalid data location function fp1(S memory) internal {} function fp2(S storage) internal {} @@ -19,8 +21,10 @@ contract C { function n1(Nested memory) public {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions function n2(Nested storage) public {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions + //~^ ERROR: invalid data location function n3() public returns(Nested memory) {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions function n4() public returns(Nested storage) {} //~ ERROR: types containing mappings cannot be parameter or return types of public functions + //~^ ERROR: invalid data location function pn1(Nested memory) internal {} function pn2(Nested storage) internal {} diff --git a/tests/ui/typeck/mapping_structs.stderr b/tests/ui/typeck/mapping_structs.stderr index 1a030b72..65eeeeb2 100644 --- a/tests/ui/typeck/mapping_structs.stderr +++ b/tests/ui/typeck/mapping_structs.stderr @@ -2,57 +2,89 @@ error: types containing mappings cannot be parameter or return types of public f --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | LL | function f1(S memory) public {} - | ^^^^^^^^ + | ^ | -error: types containing mappings cannot be parameter or return types of public functions +error: invalid data location `storage` --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | LL | function f2(S storage) public {} | ^^^^^^^^^ | + = note: data location must be `memory` or `calldata` for public function parameter, but got `storage` error: types containing mappings cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | -LL | function f3() public returns(S memory) {} - | ^^^^^^^^ +LL | function f2(S storage) public {} + | ^ | error: types containing mappings cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | +LL | function f3() public returns(S memory) {} + | ^ + | + +error: invalid data location `storage` + --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC + | LL | function f4() public returns(S storage) {} | ^^^^^^^^^ | + = note: data location must be `memory` or `calldata` for public function return parameter, but got `storage` error: types containing mappings cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | -LL | function n1(Nested memory) public {} - | ^^^^^^^^^^^^^ +LL | function f4() public returns(S storage) {} + | ^ | error: types containing mappings cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | +LL | function n1(Nested memory) public {} + | ^^^^^^ + | + +error: invalid data location `storage` + --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC + | LL | function n2(Nested storage) public {} | ^^^^^^^^^^^^^^ | + = note: data location must be `memory` or `calldata` for public function parameter, but got `storage` error: types containing mappings cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | -LL | function n3() public returns(Nested memory) {} - | ^^^^^^^^^^^^^ +LL | function n2(Nested storage) public {} + | ^^^^^^ | error: types containing mappings cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC | +LL | function n3() public returns(Nested memory) {} + | ^^^^^^ + | + +error: invalid data location `storage` + --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC + | LL | function n4() public returns(Nested storage) {} | ^^^^^^^^^^^^^^ | + = note: data location must be `memory` or `calldata` for public function return parameter, but got `storage` + +error: types containing mappings cannot be parameter or return types of public functions + --> ROOT/tests/ui/typeck/mapping_structs.sol:LL:CC + | +LL | function n4() public returns(Nested storage) {} + | ^^^^^^ + | -error: aborting due to 8 previous errors +error: aborting due to 12 previous errors diff --git a/tests/ui/typeck/recursive_function_parameter.sol b/tests/ui/typeck/recursive_function_parameter.sol index 95a7cada..7f27e7ef 100644 --- a/tests/ui/typeck/recursive_function_parameter.sol +++ b/tests/ui/typeck/recursive_function_parameter.sol @@ -10,11 +10,11 @@ contract CC { C[] c; } - function a1(A) public {} - function b1(B) public {} - function c1(C) public {} //~ ERROR: recursive types cannot be parameter or return types of public functions + function a1(A memory) public {} + function b1(B memory) public {} + function c1(C memory) public {} //~ ERROR: recursive types cannot be parameter or return types of public functions - function a2() public returns(A) {} - function b2() public returns(B) {} - function c2() public returns(C) {} //~ ERROR: recursive types cannot be parameter or return types of public functions + function a2() public returns(A memory) {} + function b2() public returns(B memory) {} + function c2() public returns(C memory) {} //~ ERROR: recursive types cannot be parameter or return types of public functions } diff --git a/tests/ui/typeck/recursive_function_parameter.stderr b/tests/ui/typeck/recursive_function_parameter.stderr index 866b6e25..cb94c2cd 100644 --- a/tests/ui/typeck/recursive_function_parameter.stderr +++ b/tests/ui/typeck/recursive_function_parameter.stderr @@ -21,14 +21,14 @@ LL | | } error: recursive types cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/recursive_function_parameter.sol:LL:CC | -LL | function c1(C) public {} +LL | function c1(C memory) public {} | ^ | error: recursive types cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/recursive_function_parameter.sol:LL:CC | -LL | function c2() public returns(C) {} +LL | function c2() public returns(C memory) {} | ^ | diff --git a/tests/ui/typeck/recursive_types.sol b/tests/ui/typeck/recursive_types.sol index ed62809d..fb3f955e 100644 --- a/tests/ui/typeck/recursive_types.sol +++ b/tests/ui/typeck/recursive_types.sol @@ -15,10 +15,10 @@ contract CC { type U1 is U1; //~ ERROR: the underlying type of UDVTs must be an elementary value type - function a(A) public {} - function b(B) public {} - function c(C) public {} //~ ERROR: recursive types cannot be parameter or return types of public functions - function d(E1) public {} //~ ERROR: name has to refer to a valid user-defined type - function e(E2) public {} //~ ERROR: name has to refer to a valid user-defined type - function f(U1) public {} + function a(A memory) public {} + function b(B memory) public {} + function c(C memory) public {} //~ ERROR: recursive types cannot be parameter or return types of public functions + function d(E1 memory) public {} //~ ERROR: name has to refer to a valid user-defined type + function e(E2 memory) public {} //~ ERROR: name has to refer to a valid user-defined type + function f(U1 memory) public {} } diff --git a/tests/ui/typeck/recursive_types.stderr b/tests/ui/typeck/recursive_types.stderr index d980ecb3..8bf86cab 100644 --- a/tests/ui/typeck/recursive_types.stderr +++ b/tests/ui/typeck/recursive_types.stderr @@ -21,21 +21,21 @@ LL | | } error: recursive types cannot be parameter or return types of public functions --> ROOT/tests/ui/typeck/recursive_types.sol:LL:CC | -LL | function c(C) public {} +LL | function c(C memory) public {} | ^ | error: name has to refer to a valid user-defined type --> ROOT/tests/ui/typeck/recursive_types.sol:LL:CC | -LL | function d(E1) public {} +LL | function d(E1 memory) public {} | ^^ | error: name has to refer to a valid user-defined type --> ROOT/tests/ui/typeck/recursive_types.sol:LL:CC | -LL | function e(E2) public {} +LL | function e(E2 memory) public {} | ^^ | diff --git a/tests/ui/typeck/var_loc_contract_fns.sol b/tests/ui/typeck/var_loc_contract_fns.sol new file mode 100644 index 00000000..da1362f2 --- /dev/null +++ b/tests/ui/typeck/var_loc_contract_fns.sol @@ -0,0 +1,75 @@ +struct S { + uint x; +} + +enum E { + A +} + +contract C { + function f0( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) private {} + function f1( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) internal {} + function f2( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) public {} + function f3( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) external {} + + function r0() private returns ( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) {} + function r1() internal returns ( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) {} + function r2() public returns ( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) {} + function r3() external returns ( + uint memory a2, //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b2, + S memory c2, + S[] memory d2, + E memory e2, //~ ERROR: data location can only be specified for array, struct or mapping types + E[] memory f22 + ) {} +} diff --git a/tests/ui/typeck/var_loc_contract_fns.stderr b/tests/ui/typeck/var_loc_contract_fns.stderr new file mode 100644 index 00000000..f08ec2ac --- /dev/null +++ b/tests/ui/typeck/var_loc_contract_fns.stderr @@ -0,0 +1,114 @@ +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | uint memory a2, + | ^^^^^^^^^^^^^^ + | + +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_contract_fns.sol:LL:CC + | +LL | E memory e2, + | ^^^^^^^^^^^ + | + +error: aborting due to 16 previous errors + diff --git a/tests/ui/typeck/var_loc_file_level.sol b/tests/ui/typeck/var_loc_file_level.sol new file mode 100644 index 00000000..de3f6fe3 --- /dev/null +++ b/tests/ui/typeck/var_loc_file_level.sol @@ -0,0 +1,8 @@ +struct S { + uint x; +} + +uint memory constant a0 = 0; //~ ERROR: data locations are not allowed here +uint[] memory constant b0 = []; //~ ERROR: data locations are not allowed here +S memory constant c0 = S(0); //~ ERROR: data locations are not allowed here +S[] memory constant d0 = []; //~ ERROR: data locations are not allowed here diff --git a/tests/ui/typeck/var_loc_file_level.stderr b/tests/ui/typeck/var_loc_file_level.stderr new file mode 100644 index 00000000..19e61859 --- /dev/null +++ b/tests/ui/typeck/var_loc_file_level.stderr @@ -0,0 +1,30 @@ +error: data locations are not allowed here + --> ROOT/tests/ui/typeck/var_loc_file_level.sol:LL:CC + | +LL | uint memory constant a0 = 0; + | ^^^^^^ + | + +error: data locations are not allowed here + --> ROOT/tests/ui/typeck/var_loc_file_level.sol:LL:CC + | +LL | uint[] memory constant b0 = []; + | ^^^^^^ + | + +error: data locations are not allowed here + --> ROOT/tests/ui/typeck/var_loc_file_level.sol:LL:CC + | +LL | S memory constant c0 = S(0); + | ^^^^^^ + | + +error: data locations are not allowed here + --> ROOT/tests/ui/typeck/var_loc_file_level.sol:LL:CC + | +LL | S[] memory constant d0 = []; + | ^^^^^^ + | + +error: aborting due to 4 previous errors + diff --git a/tests/ui/typeck/var_loc_state.sol b/tests/ui/typeck/var_loc_state.sol new file mode 100644 index 00000000..a15029c3 --- /dev/null +++ b/tests/ui/typeck/var_loc_state.sol @@ -0,0 +1,10 @@ +struct S { + uint x; +} + +contract C { + uint memory a1 = 0; //~ ERROR: data location can only be specified for array, struct or mapping types + uint[] memory b1 = []; //~ ERROR: invalid data location + S memory c1 = S(0); //~ ERROR: invalid data location + S[] memory d1 = []; //~ ERROR: invalid data location +} diff --git a/tests/ui/typeck/var_loc_state.stderr b/tests/ui/typeck/var_loc_state.stderr new file mode 100644 index 00000000..1ffecb3e --- /dev/null +++ b/tests/ui/typeck/var_loc_state.stderr @@ -0,0 +1,33 @@ +error: data location can only be specified for array, struct or mapping types + --> ROOT/tests/ui/typeck/var_loc_state.sol:LL:CC + | +LL | uint memory a1 = 0; + | ^^^^^^^^^^^^^^^^^^^ + | + +error: invalid data location `memory` + --> ROOT/tests/ui/typeck/var_loc_state.sol:LL:CC + | +LL | uint[] memory b1 = []; + | ^^^^^^^^^^^^^^^^^^^^^^ + | + = note: data location must be `none` or `transient` for state variable, but got `memory` + +error: invalid data location `memory` + --> ROOT/tests/ui/typeck/var_loc_state.sol:LL:CC + | +LL | S memory c1 = S(0); + | ^^^^^^^^^^^^^^^^^^^ + | + = note: data location must be `none` or `transient` for state variable, but got `memory` + +error: invalid data location `memory` + --> ROOT/tests/ui/typeck/var_loc_state.sol:LL:CC + | +LL | S[] memory d1 = []; + | ^^^^^^^^^^^^^^^^^^^ + | + = note: data location must be `none` or `transient` for state variable, but got `memory` + +error: aborting due to 4 previous errors + diff --git a/tools/tester/src/solc/solidity.rs b/tools/tester/src/solc/solidity.rs index ebfe20a5..40777473 100644 --- a/tools/tester/src/solc/solidity.rs +++ b/tools/tester/src/solc/solidity.rs @@ -93,6 +93,14 @@ pub(crate) fn should_skip(path: &Path) -> Option<&'static str> { | "invalid_utf8_sequence" // Validation is in solar's AST stage (https://github.com/paradigmxyz/solar/pull/120). | "empty_enum" + + // Data locations are checked after parsing. + | "stopAfterParsingError" + | "state_variable_storage_named_transient" + | "transient_local_variable" + | "transient_function_type_with_transient_param" + | "invalid_state_variable_location" + | "location_specifiers_for_state_variables" ) { return Some("manually skipped"); };