From 2c85b0a9e01374802c0f4f6a28b952376f61da8a Mon Sep 17 00:00:00 2001 From: Christoph Burgdorf Date: Mon, 11 Jul 2022 09:54:38 +0200 Subject: [PATCH 1/2] Fix edge case in function call visibility check --- crates/analyzer/src/traversal/expressions.rs | 110 +++--- crates/analyzer/tests/errors.rs | 2 + .../tests/snapshots/analysis__struct_fns.snap | 368 ++++++++++-------- ..._call_non_pub_fn_on_external_contract.snap | 7 +- .../errors__call_non_pub_fn_on_struct.snap | 18 + .../errors__call_non_pub_fn_on_struct2.snap | 18 + .../call_non_pub_fn_on_struct.fe | 10 + .../call_non_pub_fn_on_struct2.fe | 10 + .../features/pure_fn_internal_call.fe | 2 +- .../fixtures/features/simple_traits.fe | 2 + .../fixtures/features/struct_fns.fe | 6 +- ...fe_compiler_tests__features__case_056.snap | 2 +- ..._tests__features__case_161_struct_fns.snap | 2 +- newsfragments/767.bugfix.md | 18 + 14 files changed, 344 insertions(+), 231 deletions(-) create mode 100644 crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct.snap create mode 100644 crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct2.snap create mode 100644 crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct.fe create mode 100644 crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct2.fe create mode 100644 newsfragments/767.bugfix.md diff --git a/crates/analyzer/src/traversal/expressions.rs b/crates/analyzer/src/traversal/expressions.rs index 5e56406302..520e14e4d8 100644 --- a/crates/analyzer/src/traversal/expressions.rs +++ b/crates/analyzer/src/traversal/expressions.rs @@ -4,7 +4,7 @@ use crate::context::{ }; use crate::display::Displayable; use crate::errors::{FatalError, IndexingError}; -use crate::namespace::items::{FunctionId, ImplId, Item, StructId, TypeDef}; +use crate::namespace::items::{FunctionId, FunctionSigId, ImplId, Item, StructId, TypeDef}; use crate::namespace::scopes::BlockScopeType; use crate::namespace::types::{Array, Base, FeString, Integer, Tuple, Type, TypeDowncast, TypeId}; use crate::operations; @@ -1011,7 +1011,7 @@ fn expr_call_named_thing( expr_call_intrinsic(context, function, func.span, generic_args, args) } NamedThing::Item(Item::Function(function)) => { - expr_call_pure(context, function, generic_args, args) + expr_call_pure(context, function, func.span, generic_args, args) } NamedThing::Item(Item::Type(def)) => { if let Some(args) = generic_args { @@ -1202,12 +1202,62 @@ fn expr_call_intrinsic( )) } +fn validate_visibility_of_called_fn( + context: &mut dyn AnalyzerContext, + call_span: Span, + called_fn: FunctionSigId, +) { + let fn_parent = called_fn.parent(context.db()); + + // We don't need to validate `pub` if the called function is a module function + if let Item::Module(_) = fn_parent { + return; + } + + let name = called_fn.name(context.db()); + let parent_name = fn_parent.name(context.db()); + + let is_called_from_same_item = fn_parent == context.parent_function().parent(context.db()); + if !called_fn.is_public(context.db()) && !is_called_from_same_item { + context.fancy_error( + &format!( + "the function `{}` on `{} {}` is private", + name, + fn_parent.item_kind_display_name(), + fn_parent.name(context.db()) + ), + vec![ + Label::primary(call_span, "this function is not `pub`"), + Label::secondary( + called_fn.name_span(context.db()), + format!("`{}` is defined here", name), + ), + ], + vec![ + format!( + "`{}` can only be called from other functions within `{}`", + name, parent_name + ), + format!( + "Hint: use `pub fn {fun}(..)` to make `{fun}` callable from outside of `{parent}`", + fun = name, + parent = parent_name + ), + ], + ); + } +} + fn expr_call_pure( context: &mut dyn AnalyzerContext, function: FunctionId, + call_span: Span, generic_args: &Option>>, args: &Node>>, ) -> Result<(ExpressionAttributes, CallType), FatalError> { + let sig = function.sig(context.db()); + validate_visibility_of_called_fn(context, call_span, sig); + let fn_name = function.name(context.db()); if let Some(args) = generic_args { context.fancy_error( @@ -1409,8 +1459,7 @@ fn expr_call_method( // and the global objects are replaced by `Context`, we can remove this. // All other `NamedThing`s will be handled correctly by `expr()`. if let fe::Expr::Name(name) = &target.kind { - if let Ok(Some(NamedThing::Item(Item::Type(def)))) = context.resolve_name(name, target.span) - { + if let Ok(Some(NamedThing::Item(Item::Type(def)))) = context.resolve_name(name, target.span) { let typ = def.typ(context.db())?; return expr_call_type_attribute(context, typ, target.span, field, generic_args, args); } @@ -1451,6 +1500,9 @@ fn expr_call_method( ))), [method] => { let is_self = is_self_value(target); + + validate_visibility_of_called_fn(context, field.span, *method); + if is_self && !method.takes_self(context.db()) { context.fancy_error( &format!("`{}` must be called without `self`", &field.kind), @@ -1460,24 +1512,8 @@ fn expr_call_method( &field.kind, &field.kind )], ); - } else if !is_self && !method.is_public(context.db()) { - context.fancy_error( - &format!( - "the function `{}` on `{} {}` is private", - &field.kind, - target_type.kind_display_name(context.db()), - target_type.name(context.db()) - ), - vec![ - Label::primary(field.span, "this function is not `pub`"), - Label::secondary( - method.data(context.db()).ast.span, - format!("`{}` is defined here", &field.kind), - ), - ], - vec![], - ); } + let sig = method.signature(context.db()); validate_named_args(context, &field.kind, field.span, args, &sig.params)?; @@ -1924,37 +1960,7 @@ fn expr_call_type_attribute( ); } - // Returns `true` if the current contract belongs to the same class as an input - // `callee_class`. - let is_same_class = |callee_class| match (context.root_item(), callee_class) { - (Item::Type(TypeDef::Contract(caller)), Type::Contract(callee)) => caller == callee, - (Item::Type(TypeDef::Struct(caller)), Type::Struct(callee)) => caller == callee, - _ => false, - }; - - // Check for visibility of callee. - // Emits an error if callee is not public and caller doesn't belong to the same - // class as callee. - if !sig.is_public(context.db()) && is_same_class(target_type.typ(context.db())) { - context.fancy_error( - &format!( - "the function `{}.{}` is private", - &target_name, - &field.kind, - ), - vec![ - Label::primary(field.span, "this function is not `pub`"), - Label::secondary( - sig.data(context.db()).ast.span, - format!("`{}` is defined here", &field.kind), - ), - ], - vec![ - format!("`{}.{}` can only be called from other functions within `{}`", &target_name, &field.kind, &target_name), - format!("Hint: use `pub fn {fun}(..` to make `{cls}.{fun}` callable from outside of `{cls}`", fun=&field.kind, cls=&target_name), - ], - ); - } + validate_visibility_of_called_fn(context, field.span, sig); if target_type.is_contract(context.db()) { // TODO: `MathLibContract::square(x)` can't be called yet, because yulgen diff --git a/crates/analyzer/tests/errors.rs b/crates/analyzer/tests/errors.rs index dfb4e133db..6720b5f308 100644 --- a/crates/analyzer/tests/errors.rs +++ b/crates/analyzer/tests/errors.rs @@ -228,6 +228,8 @@ test_file! { call_undefined_function_on_external_contract } test_file! { call_undefined_function_on_memory_struct } test_file! { call_undefined_function_on_storage_struct } test_file! { call_non_pub_fn_on_external_contract } +test_file! { call_non_pub_fn_on_struct } +test_file! { call_non_pub_fn_on_struct2 } test_file! { cannot_move } test_file! { cannot_move2 } test_file! { circular_dependency_create } diff --git a/crates/analyzer/tests/snapshots/analysis__struct_fns.snap b/crates/analyzer/tests/snapshots/analysis__struct_fns.snap index 570efee3ce..ef0d3361b1 100644 --- a/crates/analyzer/tests/snapshots/analysis__struct_fns.snap +++ b/crates/analyzer/tests/snapshots/analysis__struct_fns.snap @@ -14,7 +14,7 @@ note: note: ┌─ struct_fns.fe:5:5 │ -5 │ ╭ pub fn new(x: u64, y: u64) -> Point { +5 │ ╭ fn internal_new(x: u64, y: u64) -> Point { 6 │ │ return Point(x, y) 7 │ │ } │ ╰─────^ self: None, params: [{ label: None, name: x, typ: u64 }, { label: None, name: y, typ: u64 }] -> Point @@ -36,485 +36,507 @@ note: note: ┌─ struct_fns.fe:9:5 │ - 9 │ ╭ pub fn origin() -> Point { -10 │ │ return Point(x: 0, y: 0) + 9 │ ╭ pub fn new(x: u64, y: u64) -> Point { +10 │ │ return Point::internal_new(x, y) 11 │ │ } + │ ╰─────^ self: None, params: [{ label: None, name: x, typ: u64 }, { label: None, name: y, typ: u64 }] -> Point + +note: + ┌─ struct_fns.fe:10:36 + │ +10 │ return Point::internal_new(x, y) + │ ^ ^ u64: Value + │ │ + │ u64: Value + +note: + ┌─ struct_fns.fe:10:16 + │ +10 │ return Point::internal_new(x, y) + │ ^^^^^^^^^^^^^^^^^^^^^^^^^ Point: Memory + +note: + ┌─ struct_fns.fe:13:5 + │ +13 │ ╭ pub fn origin() -> Point { +14 │ │ return Point(x: 0, y: 0) +15 │ │ } │ ╰─────^ self: None, params: [] -> Point note: - ┌─ struct_fns.fe:10:25 + ┌─ struct_fns.fe:14:25 │ -10 │ return Point(x: 0, y: 0) +14 │ return Point(x: 0, y: 0) │ ^ ^ u64: Value │ │ │ u64: Value note: - ┌─ struct_fns.fe:10:16 + ┌─ struct_fns.fe:14:16 │ -10 │ return Point(x: 0, y: 0) +14 │ return Point(x: 0, y: 0) │ ^^^^^^^^^^^^^^^^^ Point: Memory note: - ┌─ struct_fns.fe:13:5 + ┌─ struct_fns.fe:17:5 │ -13 │ ╭ pub fn x(self) -> u64 { -14 │ │ return self.x -15 │ │ } +17 │ ╭ pub fn x(self) -> u64 { +18 │ │ return self.x +19 │ │ } │ ╰─────^ self: Some(Mutable), params: [] -> u64 note: - ┌─ struct_fns.fe:14:16 + ┌─ struct_fns.fe:18:16 │ -14 │ return self.x +18 │ return self.x │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:14:16 + ┌─ struct_fns.fe:18:16 │ -14 │ return self.x +18 │ return self.x │ ^^^^^^ u64: Memory => Value note: - ┌─ struct_fns.fe:17:5 + ┌─ struct_fns.fe:21:5 │ -17 │ ╭ pub fn set_x(self, _ x: u64) -> u64 { -18 │ │ let old: u64 = self.x -19 │ │ self.x = x -20 │ │ return old -21 │ │ } +21 │ ╭ pub fn set_x(self, _ x: u64) -> u64 { +22 │ │ let old: u64 = self.x +23 │ │ self.x = x +24 │ │ return old +25 │ │ } │ ╰─────^ self: Some(Mutable), params: [{ label: Some("_"), name: x, typ: u64 }] -> u64 note: - ┌─ struct_fns.fe:18:13 + ┌─ struct_fns.fe:22:13 │ -18 │ let old: u64 = self.x +22 │ let old: u64 = self.x │ ^^^ u64 note: - ┌─ struct_fns.fe:18:24 + ┌─ struct_fns.fe:22:24 │ -18 │ let old: u64 = self.x +22 │ let old: u64 = self.x │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:18:24 + ┌─ struct_fns.fe:22:24 │ -18 │ let old: u64 = self.x +22 │ let old: u64 = self.x │ ^^^^^^ u64: Memory => Value -19 │ self.x = x +23 │ self.x = x │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:19:9 + ┌─ struct_fns.fe:23:9 │ -19 │ self.x = x +23 │ self.x = x │ ^^^^^^ ^ u64: Value │ │ │ u64: Memory -20 │ return old +24 │ return old │ ^^^ u64: Value note: - ┌─ struct_fns.fe:23:5 + ┌─ struct_fns.fe:27:5 │ -23 │ ╭ pub fn reflect(self) { -24 │ │ let x: u64 = self.x -25 │ │ let y: u64 = self.y -26 │ │ self.x = y -27 │ │ self.y = x -28 │ │ } +27 │ ╭ pub fn reflect(self) { +28 │ │ let x: u64 = self.x +29 │ │ let y: u64 = self.y +30 │ │ self.x = y +31 │ │ self.y = x +32 │ │ } │ ╰─────^ self: Some(Mutable), params: [] -> () note: - ┌─ struct_fns.fe:24:13 + ┌─ struct_fns.fe:28:13 │ -24 │ let x: u64 = self.x +28 │ let x: u64 = self.x │ ^ u64 -25 │ let y: u64 = self.y +29 │ let y: u64 = self.y │ ^ u64 note: - ┌─ struct_fns.fe:24:22 + ┌─ struct_fns.fe:28:22 │ -24 │ let x: u64 = self.x +28 │ let x: u64 = self.x │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:24:22 + ┌─ struct_fns.fe:28:22 │ -24 │ let x: u64 = self.x +28 │ let x: u64 = self.x │ ^^^^^^ u64: Memory => Value -25 │ let y: u64 = self.y +29 │ let y: u64 = self.y │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:25:22 + ┌─ struct_fns.fe:29:22 │ -25 │ let y: u64 = self.y +29 │ let y: u64 = self.y │ ^^^^^^ u64: Memory => Value -26 │ self.x = y +30 │ self.x = y │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:26:9 + ┌─ struct_fns.fe:30:9 │ -26 │ self.x = y +30 │ self.x = y │ ^^^^^^ ^ u64: Value │ │ │ u64: Memory -27 │ self.y = x +31 │ self.y = x │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:27:9 + ┌─ struct_fns.fe:31:9 │ -27 │ self.y = x +31 │ self.y = x │ ^^^^^^ ^ u64: Value │ │ │ u64: Memory note: - ┌─ struct_fns.fe:30:5 + ┌─ struct_fns.fe:34:5 │ -30 │ ╭ pub fn translate(self, x: u64, y: u64) { -31 │ │ self.x += x -32 │ │ self.y += y -33 │ │ } +34 │ ╭ pub fn translate(self, x: u64, y: u64) { +35 │ │ self.x += x +36 │ │ self.y += y +37 │ │ } │ ╰─────^ self: Some(Mutable), params: [{ label: None, name: x, typ: u64 }, { label: None, name: y, typ: u64 }] -> () note: - ┌─ struct_fns.fe:31:9 + ┌─ struct_fns.fe:35:9 │ -31 │ self.x += x +35 │ self.x += x │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:31:9 + ┌─ struct_fns.fe:35:9 │ -31 │ self.x += x +35 │ self.x += x │ ^^^^^^ ^ u64: Value │ │ │ u64: Memory -32 │ self.y += y +36 │ self.y += y │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:32:9 + ┌─ struct_fns.fe:36:9 │ -32 │ self.y += y +36 │ self.y += y │ ^^^^^^ ^ u64: Value │ │ │ u64: Memory note: - ┌─ struct_fns.fe:35:5 + ┌─ struct_fns.fe:39:5 │ -35 │ ╭ pub fn add(self, _ other: Point) -> Point { -36 │ │ let x: u64 = self.x + other.x -37 │ │ let y: u64 = self.y + other.y -38 │ │ return Point(x, y) -39 │ │ } +39 │ ╭ pub fn add(self, _ other: Point) -> Point { +40 │ │ let x: u64 = self.x + other.x +41 │ │ let y: u64 = self.y + other.y +42 │ │ return Point(x, y) +43 │ │ } │ ╰─────^ self: Some(Mutable), params: [{ label: Some("_"), name: other, typ: Point }] -> Point note: - ┌─ struct_fns.fe:36:13 + ┌─ struct_fns.fe:40:13 │ -36 │ let x: u64 = self.x + other.x +40 │ let x: u64 = self.x + other.x │ ^ u64 -37 │ let y: u64 = self.y + other.y +41 │ let y: u64 = self.y + other.y │ ^ u64 note: - ┌─ struct_fns.fe:36:22 + ┌─ struct_fns.fe:40:22 │ -36 │ let x: u64 = self.x + other.x +40 │ let x: u64 = self.x + other.x │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:36:22 + ┌─ struct_fns.fe:40:22 │ -36 │ let x: u64 = self.x + other.x +40 │ let x: u64 = self.x + other.x │ ^^^^^^ ^^^^^ Point: Memory │ │ │ u64: Memory => Value note: - ┌─ struct_fns.fe:36:31 + ┌─ struct_fns.fe:40:31 │ -36 │ let x: u64 = self.x + other.x +40 │ let x: u64 = self.x + other.x │ ^^^^^^^ u64: Memory => Value note: - ┌─ struct_fns.fe:36:22 + ┌─ struct_fns.fe:40:22 │ -36 │ let x: u64 = self.x + other.x +40 │ let x: u64 = self.x + other.x │ ^^^^^^^^^^^^^^^^ u64: Value -37 │ let y: u64 = self.y + other.y +41 │ let y: u64 = self.y + other.y │ ^^^^ Point: Memory note: - ┌─ struct_fns.fe:37:22 + ┌─ struct_fns.fe:41:22 │ -37 │ let y: u64 = self.y + other.y +41 │ let y: u64 = self.y + other.y │ ^^^^^^ ^^^^^ Point: Memory │ │ │ u64: Memory => Value note: - ┌─ struct_fns.fe:37:31 + ┌─ struct_fns.fe:41:31 │ -37 │ let y: u64 = self.y + other.y +41 │ let y: u64 = self.y + other.y │ ^^^^^^^ u64: Memory => Value note: - ┌─ struct_fns.fe:37:22 + ┌─ struct_fns.fe:41:22 │ -37 │ let y: u64 = self.y + other.y +41 │ let y: u64 = self.y + other.y │ ^^^^^^^^^^^^^^^^ u64: Value -38 │ return Point(x, y) +42 │ return Point(x, y) │ ^ ^ u64: Value │ │ │ u64: Value note: - ┌─ struct_fns.fe:38:16 + ┌─ struct_fns.fe:42:16 │ -38 │ return Point(x, y) +42 │ return Point(x, y) │ ^^^^^^^^^^^ Point: Memory note: - ┌─ struct_fns.fe:42:1 + ┌─ struct_fns.fe:46:1 │ -42 │ ╭ pub fn do_pointy_things() { -43 │ │ let p1: Point = Point::origin() -44 │ │ p1.translate(x: 5, y: 10) -45 │ │ let p2: Point = Point(x: 1, y: 2) -46 │ │ let p3: Point = p1.add(p2) -47 │ │ assert p3.x == 6 and p3.y == 12 -48 │ │ } +46 │ ╭ pub fn do_pointy_things() { +47 │ │ let p1: Point = Point::origin() +48 │ │ p1.translate(x: 5, y: 10) +49 │ │ let p2: Point = Point(x: 1, y: 2) +50 │ │ let p3: Point = p1.add(p2) +51 │ │ assert p3.x == 6 and p3.y == 12 +52 │ │ } │ ╰─^ self: None, params: [] -> () note: - ┌─ struct_fns.fe:43:9 + ┌─ struct_fns.fe:47:9 │ -43 │ let p1: Point = Point::origin() +47 │ let p1: Point = Point::origin() │ ^^ Point -44 │ p1.translate(x: 5, y: 10) -45 │ let p2: Point = Point(x: 1, y: 2) +48 │ p1.translate(x: 5, y: 10) +49 │ let p2: Point = Point(x: 1, y: 2) │ ^^ Point -46 │ let p3: Point = p1.add(p2) +50 │ let p3: Point = p1.add(p2) │ ^^ Point note: - ┌─ struct_fns.fe:43:21 + ┌─ struct_fns.fe:47:21 │ -43 │ let p1: Point = Point::origin() +47 │ let p1: Point = Point::origin() │ ^^^^^^^^^^^^^^^ Point: Memory -44 │ p1.translate(x: 5, y: 10) +48 │ p1.translate(x: 5, y: 10) │ ^^ ^ ^^ u64: Value │ │ │ │ │ u64: Value │ Point: Memory note: - ┌─ struct_fns.fe:44:5 + ┌─ struct_fns.fe:48:5 │ -44 │ p1.translate(x: 5, y: 10) +48 │ p1.translate(x: 5, y: 10) │ ^^^^^^^^^^^^^^^^^^^^^^^^^ (): Value -45 │ let p2: Point = Point(x: 1, y: 2) +49 │ let p2: Point = Point(x: 1, y: 2) │ ^ ^ u64: Value │ │ │ u64: Value note: - ┌─ struct_fns.fe:45:21 + ┌─ struct_fns.fe:49:21 │ -45 │ let p2: Point = Point(x: 1, y: 2) +49 │ let p2: Point = Point(x: 1, y: 2) │ ^^^^^^^^^^^^^^^^^ Point: Memory -46 │ let p3: Point = p1.add(p2) +50 │ let p3: Point = p1.add(p2) │ ^^ ^^ Point: Memory │ │ │ Point: Memory note: - ┌─ struct_fns.fe:46:21 + ┌─ struct_fns.fe:50:21 │ -46 │ let p3: Point = p1.add(p2) +50 │ let p3: Point = p1.add(p2) │ ^^^^^^^^^^ Point: Memory -47 │ assert p3.x == 6 and p3.y == 12 +51 │ assert p3.x == 6 and p3.y == 12 │ ^^ Point: Memory note: - ┌─ struct_fns.fe:47:12 + ┌─ struct_fns.fe:51:12 │ -47 │ assert p3.x == 6 and p3.y == 12 +51 │ assert p3.x == 6 and p3.y == 12 │ ^^^^ ^ u64: Value │ │ │ u64: Memory => Value note: - ┌─ struct_fns.fe:47:12 + ┌─ struct_fns.fe:51:12 │ -47 │ assert p3.x == 6 and p3.y == 12 +51 │ assert p3.x == 6 and p3.y == 12 │ ^^^^^^^^^ ^^ Point: Memory │ │ │ bool: Value note: - ┌─ struct_fns.fe:47:26 + ┌─ struct_fns.fe:51:26 │ -47 │ assert p3.x == 6 and p3.y == 12 +51 │ assert p3.x == 6 and p3.y == 12 │ ^^^^ ^^ u64: Value │ │ │ u64: Memory => Value note: - ┌─ struct_fns.fe:47:26 + ┌─ struct_fns.fe:51:26 │ -47 │ assert p3.x == 6 and p3.y == 12 +51 │ assert p3.x == 6 and p3.y == 12 │ ^^^^^^^^^^ bool: Value note: - ┌─ struct_fns.fe:47:12 + ┌─ struct_fns.fe:51:12 │ -47 │ assert p3.x == 6 and p3.y == 12 +51 │ assert p3.x == 6 and p3.y == 12 │ ^^^^^^^^^^^^^^^^^^^^^^^^ bool: Value note: - ┌─ struct_fns.fe:51:5 + ┌─ struct_fns.fe:55:5 │ -51 │ ╭ pub fn bar(x: u64, y: u64) -> u64 { -52 │ │ do_pointy_things() -53 │ │ let p: Point = Point::new(x, y) -54 │ │ assert p.x == x and p.y == y +55 │ ╭ pub fn bar(x: u64, y: u64) -> u64 { +56 │ │ do_pointy_things() +57 │ │ let p: Point = Point::new(x, y) +58 │ │ assert p.x == x and p.y == y · │ -58 │ │ return p.y -59 │ │ } +62 │ │ return p.y +63 │ │ } │ ╰─────^ self: None, params: [{ label: None, name: x, typ: u64 }, { label: None, name: y, typ: u64 }] -> u64 note: - ┌─ struct_fns.fe:53:13 + ┌─ struct_fns.fe:57:13 │ -53 │ let p: Point = Point::new(x, y) +57 │ let p: Point = Point::new(x, y) │ ^ Point note: - ┌─ struct_fns.fe:52:9 + ┌─ struct_fns.fe:56:9 │ -52 │ do_pointy_things() +56 │ do_pointy_things() │ ^^^^^^^^^^^^^^^^^^ (): Value -53 │ let p: Point = Point::new(x, y) +57 │ let p: Point = Point::new(x, y) │ ^ ^ u64: Value │ │ │ u64: Value note: - ┌─ struct_fns.fe:53:24 + ┌─ struct_fns.fe:57:24 │ -53 │ let p: Point = Point::new(x, y) +57 │ let p: Point = Point::new(x, y) │ ^^^^^^^^^^^^^^^^ Point: Memory -54 │ assert p.x == x and p.y == y +58 │ assert p.x == x and p.y == y │ ^ Point: Memory note: - ┌─ struct_fns.fe:54:16 + ┌─ struct_fns.fe:58:16 │ -54 │ assert p.x == x and p.y == y +58 │ assert p.x == x and p.y == y │ ^^^ ^ u64: Value │ │ │ u64: Memory => Value note: - ┌─ struct_fns.fe:54:16 + ┌─ struct_fns.fe:58:16 │ -54 │ assert p.x == x and p.y == y +58 │ assert p.x == x and p.y == y │ ^^^^^^^^ ^ Point: Memory │ │ │ bool: Value note: - ┌─ struct_fns.fe:54:29 + ┌─ struct_fns.fe:58:29 │ -54 │ assert p.x == x and p.y == y +58 │ assert p.x == x and p.y == y │ ^^^ ^ u64: Value │ │ │ u64: Memory => Value note: - ┌─ struct_fns.fe:54:29 + ┌─ struct_fns.fe:58:29 │ -54 │ assert p.x == x and p.y == y +58 │ assert p.x == x and p.y == y │ ^^^^^^^^ bool: Value note: - ┌─ struct_fns.fe:54:16 + ┌─ struct_fns.fe:58:16 │ -54 │ assert p.x == x and p.y == y +58 │ assert p.x == x and p.y == y │ ^^^^^^^^^^^^^^^^^^^^^ bool: Value -55 │ p.set_x(100) +59 │ p.set_x(100) │ ^ ^^^ u64: Value │ │ │ Point: Memory note: - ┌─ struct_fns.fe:55:9 + ┌─ struct_fns.fe:59:9 │ -55 │ p.set_x(100) +59 │ p.set_x(100) │ ^^^^^^^^^^^^ u64: Value -56 │ p.reflect() +60 │ p.reflect() │ ^ Point: Memory note: - ┌─ struct_fns.fe:56:9 + ┌─ struct_fns.fe:60:9 │ -56 │ p.reflect() +60 │ p.reflect() │ ^^^^^^^^^^^ (): Value -57 │ assert p.x() == y and p.y == 100 +61 │ assert p.x() == y and p.y == 100 │ ^ Point: Memory note: - ┌─ struct_fns.fe:57:16 + ┌─ struct_fns.fe:61:16 │ -57 │ assert p.x() == y and p.y == 100 +61 │ assert p.x() == y and p.y == 100 │ ^^^^^ ^ u64: Value │ │ │ u64: Value note: - ┌─ struct_fns.fe:57:16 + ┌─ struct_fns.fe:61:16 │ -57 │ assert p.x() == y and p.y == 100 +61 │ assert p.x() == y and p.y == 100 │ ^^^^^^^^^^ ^ Point: Memory │ │ │ bool: Value note: - ┌─ struct_fns.fe:57:31 + ┌─ struct_fns.fe:61:31 │ -57 │ assert p.x() == y and p.y == 100 +61 │ assert p.x() == y and p.y == 100 │ ^^^ ^^^ u64: Value │ │ │ u64: Memory => Value note: - ┌─ struct_fns.fe:57:31 + ┌─ struct_fns.fe:61:31 │ -57 │ assert p.x() == y and p.y == 100 +61 │ assert p.x() == y and p.y == 100 │ ^^^^^^^^^^ bool: Value note: - ┌─ struct_fns.fe:57:16 + ┌─ struct_fns.fe:61:16 │ -57 │ assert p.x() == y and p.y == 100 +61 │ assert p.x() == y and p.y == 100 │ ^^^^^^^^^^^^^^^^^^^^^^^^^ bool: Value -58 │ return p.y +62 │ return p.y │ ^ Point: Memory note: - ┌─ struct_fns.fe:58:16 + ┌─ struct_fns.fe:62:16 │ -58 │ return p.y +62 │ return p.y │ ^^^ u64: Memory => Value diff --git a/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_external_contract.snap b/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_external_contract.snap index 013e0ac3e2..07e58f5069 100644 --- a/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_external_contract.snap +++ b/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_external_contract.snap @@ -3,13 +3,16 @@ source: crates/analyzer/tests/errors.rs expression: "error_string(&path, test_files::fixture(path))" --- -error: the function `do_private_thingz` on `contract Foo` is private +error: the function `do_private_thingz` on `type Foo` is private ┌─ compile_errors/call_non_pub_fn_on_external_contract.fe:13:25 │ 6 │ fn do_private_thingz(self) { - │ -------------------------- `do_private_thingz` is defined here + │ ----------------- `do_private_thingz` is defined here · 13 │ Foo(address(0)).do_private_thingz() │ ^^^^^^^^^^^^^^^^^ this function is not `pub` + │ + = `do_private_thingz` can only be called from other functions within `Foo` + = Hint: use `pub fn do_private_thingz(..)` to make `do_private_thingz` callable from outside of `Foo` diff --git a/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct.snap b/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct.snap new file mode 100644 index 0000000000..e5efc2a515 --- /dev/null +++ b/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct.snap @@ -0,0 +1,18 @@ +--- +source: crates/analyzer/tests/errors.rs +expression: "error_string(&path, test_files::fixture(path))" + +--- +error: the function `do_private_thingz` on `struct Foo` is private + ┌─ compile_errors/call_non_pub_fn_on_struct.fe:8:9 + │ +2 │ fn do_private_thingz() { + │ ----------------- `do_private_thingz` is defined here + · +8 │ Foo::do_private_thingz() + │ ^^^^^^^^^^^^^^^^^^^^^^ this function is not `pub` + │ + = `do_private_thingz` can only be called from other functions within `Foo` + = Hint: use `pub fn do_private_thingz(..)` to make `do_private_thingz` callable from outside of `Foo` + + diff --git a/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct2.snap b/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct2.snap new file mode 100644 index 0000000000..c3a10267b7 --- /dev/null +++ b/crates/analyzer/tests/snapshots/errors__call_non_pub_fn_on_struct2.snap @@ -0,0 +1,18 @@ +--- +source: crates/analyzer/tests/errors.rs +expression: "error_string(&path, test_files::fixture(path))" + +--- +error: the function `do_private_thingz` on `struct Foo` is private + ┌─ compile_errors/call_non_pub_fn_on_struct2.fe:8:15 + │ +2 │ fn do_private_thingz(self) { + │ ----------------- `do_private_thingz` is defined here + · +8 │ Foo().do_private_thingz() + │ ^^^^^^^^^^^^^^^^^ this function is not `pub` + │ + = `do_private_thingz` can only be called from other functions within `Foo` + = Hint: use `pub fn do_private_thingz(..)` to make `do_private_thingz` callable from outside of `Foo` + + diff --git a/crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct.fe b/crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct.fe new file mode 100644 index 0000000000..b67698f27d --- /dev/null +++ b/crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct.fe @@ -0,0 +1,10 @@ +struct Foo { + fn do_private_thingz() { + } +} + +contract Bar { + fn test() { + Foo::do_private_thingz() + } +} diff --git a/crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct2.fe b/crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct2.fe new file mode 100644 index 0000000000..bbf60ad520 --- /dev/null +++ b/crates/test-files/fixtures/compile_errors/call_non_pub_fn_on_struct2.fe @@ -0,0 +1,10 @@ +struct Foo { + fn do_private_thingz(self) { + } +} + +contract Bar { + fn test() { + Foo().do_private_thingz() + } +} diff --git a/crates/test-files/fixtures/features/pure_fn_internal_call.fe b/crates/test-files/fixtures/features/pure_fn_internal_call.fe index ea72347dea..7f0b0eeaaf 100644 --- a/crates/test-files/fixtures/features/pure_fn_internal_call.fe +++ b/crates/test-files/fixtures/features/pure_fn_internal_call.fe @@ -3,7 +3,7 @@ contract Foo { return pure_bar(x, y) } - pub fn pure_bar(x: u256, y: u256) -> u256 { + fn pure_bar(x: u256, y: u256) -> u256 { return x + y } } diff --git a/crates/test-files/fixtures/features/simple_traits.fe b/crates/test-files/fixtures/features/simple_traits.fe index cf1940bdce..325fcf74d3 100644 --- a/crates/test-files/fixtures/features/simple_traits.fe +++ b/crates/test-files/fixtures/features/simple_traits.fe @@ -36,6 +36,8 @@ contract Example { } pub fn run_test(self) { + let x: Bar = Bar() + assert x.double() == 2 assert Bar().double() == 2 assert u8(1).double() == 2 assert (0, 1).double() == 2 diff --git a/crates/test-files/fixtures/features/struct_fns.fe b/crates/test-files/fixtures/features/struct_fns.fe index fbf3f5569a..e388232e3d 100644 --- a/crates/test-files/fixtures/features/struct_fns.fe +++ b/crates/test-files/fixtures/features/struct_fns.fe @@ -2,10 +2,14 @@ struct Point { pub x: u64 pub y: u64 - pub fn new(x: u64, y: u64) -> Point { + fn internal_new(x: u64, y: u64) -> Point { return Point(x, y) } + pub fn new(x: u64, y: u64) -> Point { + return Point::internal_new(x, y) + } + pub fn origin() -> Point { return Point(x: 0, y: 0) } diff --git a/crates/tests/src/snapshots/fe_compiler_tests__features__case_056.snap b/crates/tests/src/snapshots/fe_compiler_tests__features__case_056.snap index 9a7e07bff0..37b6559a96 100644 --- a/crates/tests/src/snapshots/fe_compiler_tests__features__case_056.snap +++ b/crates/tests/src/snapshots/fe_compiler_tests__features__case_056.snap @@ -3,5 +3,5 @@ source: crates/tests/src/features.rs expression: "format!(\"{}\", harness.gas_reporter)" --- -bar([Uint(42), Uint(26)]) used 250 gas +bar([Uint(42), Uint(26)]) used 202 gas diff --git a/crates/tests/src/snapshots/fe_compiler_tests__features__case_161_struct_fns.snap b/crates/tests/src/snapshots/fe_compiler_tests__features__case_161_struct_fns.snap index 9ffe4fe1d2..9b6a53c067 100644 --- a/crates/tests/src/snapshots/fe_compiler_tests__features__case_161_struct_fns.snap +++ b/crates/tests/src/snapshots/fe_compiler_tests__features__case_161_struct_fns.snap @@ -3,5 +3,5 @@ source: crates/tests/src/features.rs expression: "format!(\"{}\", harness.gas_reporter)" --- -bar([Uint(10), Uint(20)]) used 2056 gas +bar([Uint(10), Uint(20)]) used 2082 gas diff --git a/newsfragments/767.bugfix.md b/newsfragments/767.bugfix.md new file mode 100644 index 0000000000..5ac372530e --- /dev/null +++ b/newsfragments/767.bugfix.md @@ -0,0 +1,18 @@ +Fix issue where calls to assiciated functions did not enforce visibility rules. + +E.g the following code should be rejected but previously wasn't: + +``` +struct Foo { + fn do_private_things() { + } +} + +contract Bar { + fn test() { + Foo::do_private_things() + } +} +``` + +With this change, the above code is now rejected because `do_private_things` is not `pub`. From e4688c80e156165c66488cbb8e1ff51675b1839b Mon Sep 17 00:00:00 2001 From: Christoph Burgdorf Date: Mon, 11 Jul 2022 11:48:06 +0200 Subject: [PATCH 2/2] Ensure contract pure fns can be called via `ContractName::fn_name()` --- crates/analyzer/src/namespace/items.rs | 52 +++++++++++++------ crates/analyzer/src/traversal/expressions.rs | 3 +- .../fixtures/features/contract_pure_fns.fe | 19 +++++++ crates/tests/src/features.rs | 3 +- ...s__features__case_4_contract_pure_fns.snap | 7 +++ newsfragments/767.feature.md | 1 + 6 files changed, 68 insertions(+), 17 deletions(-) create mode 100644 crates/test-files/fixtures/features/contract_pure_fns.fe create mode 100644 crates/tests/src/snapshots/fe_compiler_tests__features__case_4_contract_pure_fns.snap create mode 100644 newsfragments/767.feature.md diff --git a/crates/analyzer/src/namespace/items.rs b/crates/analyzer/src/namespace/items.rs index 2877303966..1368445536 100644 --- a/crates/analyzer/src/namespace/items.rs +++ b/crates/analyzer/src/namespace/items.rs @@ -814,26 +814,17 @@ pub enum TypeDef { Contract(ContractId), Primitive(types::Base), } + impl TypeDef { pub fn items(&self, db: &dyn AnalyzerDb) -> Rc> { match self { TypeDef::Struct(val) => { - Rc::new( - val.functions(db) - .iter() - .filter_map(|(name, field)| { - if field.takes_self(db) { - // In the future we probably want to resolve instance methods as well. But this would require - // the caller to pass an instance as the first argument e.g. `Rectangle::can_hold(self_instance, other)`. - // This isn't yet supported so for now path access to functions is limited to static functions only. - None - } else { - Some((name.to_owned(), Item::Function(*field))) - } - }) - .collect(), - ) + // In the future we probably want to resolve instance methods as well. But this would require + // the caller to pass an instance as the first argument e.g. `Rectangle::can_hold(self_instance, other)`. + // This isn't yet supported so for now path access to functions is limited to static functions only. + val.pure_functions_as_items(db) } + TypeDef::Contract(val) => val.pure_functions_as_items(db), _ => todo!("cannot access items in types yet"), } } @@ -1323,6 +1314,37 @@ impl FunctionId { } } +trait FunctionsAsItems { + fn functions(&self, db: &dyn AnalyzerDb) -> Rc>; + + fn pure_functions_as_items(&self, db: &dyn AnalyzerDb) -> Rc> { + Rc::new( + self.functions(db) + .iter() + .filter_map(|(name, field)| { + if field.takes_self(db) { + None + } else { + Some((name.to_owned(), Item::Function(*field))) + } + }) + .collect(), + ) + } +} + +impl FunctionsAsItems for StructId { + fn functions(&self, db: &dyn AnalyzerDb) -> Rc> { + self.functions(db) + } +} + +impl FunctionsAsItems for ContractId { + fn functions(&self, db: &dyn AnalyzerDb) -> Rc> { + self.functions(db) + } +} + #[derive(Debug, PartialEq, Eq, Hash, Clone)] pub struct Struct { pub ast: Node, diff --git a/crates/analyzer/src/traversal/expressions.rs b/crates/analyzer/src/traversal/expressions.rs index 520e14e4d8..c9ca77a81b 100644 --- a/crates/analyzer/src/traversal/expressions.rs +++ b/crates/analyzer/src/traversal/expressions.rs @@ -1459,7 +1459,8 @@ fn expr_call_method( // and the global objects are replaced by `Context`, we can remove this. // All other `NamedThing`s will be handled correctly by `expr()`. if let fe::Expr::Name(name) = &target.kind { - if let Ok(Some(NamedThing::Item(Item::Type(def)))) = context.resolve_name(name, target.span) { + if let Ok(Some(NamedThing::Item(Item::Type(def)))) = context.resolve_name(name, target.span) + { let typ = def.typ(context.db())?; return expr_call_type_attribute(context, typ, target.span, field, generic_args, args); } diff --git a/crates/test-files/fixtures/features/contract_pure_fns.fe b/crates/test-files/fixtures/features/contract_pure_fns.fe new file mode 100644 index 0000000000..0d0f46f1e3 --- /dev/null +++ b/crates/test-files/fixtures/features/contract_pure_fns.fe @@ -0,0 +1,19 @@ +contract Example { + + pub fn run_test(self) { + assert self.bar(42, 26) == 68 + assert self.bar2(42, 26) == 68 + } + + pub fn bar(self, _ x: u256, _ y: u256) -> u256 { + return pure_bar(x, y) + } + + pub fn bar2(self, _ x: u256, _ y: u256) -> u256 { + return Example::pure_bar(x, y) + } + + fn pure_bar(x: u256, y: u256) -> u256 { + return x + y + } +} diff --git a/crates/tests/src/features.rs b/crates/tests/src/features.rs index 3b85f067d7..92f46eee35 100644 --- a/crates/tests/src/features.rs +++ b/crates/tests/src/features.rs @@ -2070,7 +2070,8 @@ fn ctx_init_in_call() { fixture_file, case::simple_traits("simple_traits.fe"), case::generic_functions("generic_functions.fe"), - case::generic_functions_primitves("generic_functions_primitves.fe") + case::generic_functions_primitves("generic_functions_primitves.fe"), + case::contract_pure_fns("contract_pure_fns.fe") )] fn execution_tests(fixture_file: &str) { with_executor(&|mut executor| { diff --git a/crates/tests/src/snapshots/fe_compiler_tests__features__case_4_contract_pure_fns.snap b/crates/tests/src/snapshots/fe_compiler_tests__features__case_4_contract_pure_fns.snap new file mode 100644 index 0000000000..09b509cccb --- /dev/null +++ b/crates/tests/src/snapshots/fe_compiler_tests__features__case_4_contract_pure_fns.snap @@ -0,0 +1,7 @@ +--- +source: crates/tests/src/features.rs +expression: "format!(\"{}\", harness.gas_reporter)" + +--- +run_test([]) used 35 gas + diff --git a/newsfragments/767.feature.md b/newsfragments/767.feature.md new file mode 100644 index 0000000000..3f17922430 --- /dev/null +++ b/newsfragments/767.feature.md @@ -0,0 +1 @@ +Allow contract associated functions to be called via `ContractName::function_name()` syntax.