Skip to content

Commit

Permalink
Merge pull request #767 from cburgdorf/christoph/fix/visibility_rules
Browse files Browse the repository at this point in the history 
Fix enforcement of visibility rules of function calls
  • Loading branch information
@sbillig
sbillig authored Jul 11, 2022
2 parents 0ccdd1b + e4688c8 commit 0f0d1ec
Show file tree
Hide file tree
Showing 19 changed files with 409 additions and 245 deletions.
52 changes: 37 additions & 15 deletions crates/analyzer/src/namespace/items.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -814,26 +814,17 @@ pub enum TypeDef {
Contract(ContractId),
Primitive(types::Base),
}

impl TypeDef {
pub fn items(&self, db: &dyn AnalyzerDb) -> Rc<IndexMap<SmolStr, Item>> {
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"),
}
}
Expand Down Expand Up @@ -1323,6 +1314,37 @@ impl FunctionId {
}
}

trait FunctionsAsItems {
fn functions(&self, db: &dyn AnalyzerDb) -> Rc<IndexMap<SmolStr, FunctionId>>;

fn pure_functions_as_items(&self, db: &dyn AnalyzerDb) -> Rc<IndexMap<SmolStr, Item>> {
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<IndexMap<SmolStr, FunctionId>> {
self.functions(db)
}
}

impl FunctionsAsItems for ContractId {
fn functions(&self, db: &dyn AnalyzerDb) -> Rc<IndexMap<SmolStr, FunctionId>> {
self.functions(db)
}
}

#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub struct Struct {
pub ast: Node<ast::Struct>,
Expand Down
107 changes: 57 additions & 50 deletions crates/analyzer/src/traversal/expressions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1011,7 +1011,7 @@ fn expr_call_named_thing<T: std::fmt::Display>(
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 {
Expand Down Expand Up @@ -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<Node<Vec<fe::GenericArg>>>,
args: &Node<Vec<Node<fe::CallArg>>>,
) -> 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(
Expand Down Expand Up @@ -1451,6 +1501,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),
Expand All @@ -1460,24 +1513,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)?;

Expand Down Expand Up @@ -1924,37 +1961,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
Expand Down
2 changes: 2 additions & 0 deletions crates/analyzer/tests/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
Loading

0 comments on commit 0f0d1ec

Please sign in to comment.