Fix mut self fn calls, disallow primitive mut params

.github/workflows/main.yml

@@ -42,7 +42,7 @@ jobs:
         uses: actions-rs/cargo@v1
         with:
           command: clippy
-          args: --workspace --all-targets --all-features -- -D warnings -W clippy::redundant_closure_for_method_calls
+          args: --workspace --all-targets --all-features -- -D warnings
 
   book:
     runs-on: ubuntu-latest

crates/analyzer/src/db/queries/functions.rs

@@ -107,8 +107,16 @@ pub fn function_signature(
             ast::FunctionArg::Regular { mut_, label, name, typ: typedesc } => {
                 let typ = resolve_function_param_type(db, function, &mut scope, typedesc).and_then(|typ| match typ {
                     typ if typ.has_fixed_size(db) => {
-                        if mut_.is_some() {
-                            Ok(Type::Mut(typ).id(db))
+                        if let Some(mut_span) = mut_ {
+                            if typ.is_primitive(db) {
+                                Err(TypeError::new(scope.error(
+                                    "primitive type function parameters cannot be `mut`",
+                                    *mut_span + typedesc.span,
+                                    &format!("`{}` type can't be used as a `mut` function parameter",
+                                             typ.display(db)))))
+                            } else {
+                                Ok(Type::Mut(typ).id(db))
+                            }
                         } else {
                             Ok(typ)
                         }

crates/analyzer/src/db/queries/module.rs

@@ -315,9 +315,9 @@ pub fn module_constant_type(
     let typ = type_desc(&mut scope, &constant_data.ast.kind.typ);
 
     match &typ {
-        Ok(typ) if !typ.is_base(db) => {
+        Ok(typ) if !typ.is_primitive(db) => {
             scope.error(
-                "Non-base types not yet supported for constants",
+                "Non-primitive types not yet supported for constants",
                 constant.data(db).ast.kind.typ.span,
                 &format!(
                     "this has type `{}`; expected a primitive type",

crates/analyzer/src/namespace/items.rs

@@ -1097,7 +1097,6 @@ impl FunctionSigId {
         db.lookup_intern_function_sig(*self)
     }
 
-    // XXX #[deprecated]
     pub fn takes_self(&self, db: &dyn AnalyzerDb) -> bool {
         self.signature(db).self_decl.is_some()
     }
@@ -1113,13 +1112,7 @@ impl FunctionSigId {
         }
     }
     pub fn self_span(&self, db: &dyn AnalyzerDb) -> Option<Span> {
-        if self.takes_self(db) {
-            self.data(db).ast.kind.args.iter().find_map(|arg| {
-                matches!(arg.kind, ast::FunctionArg::Self_ { .. }).then(|| arg.span)
-            })
-        } else {
-            None
-        }
+        Some(self.signature(db).self_decl?.span)
     }
     pub fn signature(&self, db: &dyn AnalyzerDb) -> Rc<types::FunctionSignature> {
         db.function_signature(*self).value

crates/analyzer/src/namespace/types.rs

@@ -98,7 +98,6 @@ impl TypeId {
         self.typ(db).has_fixed_size(db)
     }
 
-    // XXX is Contract type usable everywhere Base types are?
     /// `true` if Type::Base or Type::Contract (which is just an Address)
     pub fn is_primitive(&self, db: &dyn AnalyzerDb) -> bool {
         matches!(self.typ(db), Type::Base(_) | Type::Contract(_))

crates/analyzer/src/traversal/assignments.rs

@@ -83,7 +83,7 @@ fn is_valid_assign_target(
         }
 
         Name(name) => match scope.resolve_name(name, expr.span) {
-            Ok(Some(NamedThing::SelfValue { .. })) => Ok(true), // XXX ?
+            Ok(Some(NamedThing::SelfValue { .. })) => Ok(true),
             Ok(Some(NamedThing::Item(_)) | Some(NamedThing::EnumVariant(_)) | None) => {
                 bad_assign_target_error(scope, expr, "invalid assignment target");
                 Ok(false)
@@ -123,7 +123,7 @@ pub fn aug_assign(scope: &mut BlockScope, stmt: &Node<fe::FuncStmt>) -> Result<(
     };
 
     if is_valid_assign_target(scope, target)? {
-        let lhs_ty = assignment_lhs_type(scope, target)?; // XXX check this
+        let lhs_ty = assignment_lhs_type(scope, target)?;
         let rhs = expressions::expr(scope, value, Some(lhs_ty))?;
 
         if let Err(err) = operations::bin(scope, lhs_ty, target, op.kind, rhs.typ, value) {

crates/analyzer/src/traversal/call_args.rs

@@ -40,16 +40,15 @@ impl LabeledParameter for (SmolStr, Result<TypeId, TypeError>, bool) {
     }
 }
 
-impl LabeledParameter for (Option<SmolStr>, Result<TypeId, TypeError>) {
+impl LabeledParameter for (Option<SmolStr>, Result<TypeId, TypeError>, bool) {
     fn label(&self) -> Option<&str> {
         self.0.as_ref().map(smol_str::SmolStr::as_str)
     }
     fn typ(&self) -> Result<TypeId, TypeError> {
         self.1.clone()
     }
     fn is_sink(&self) -> bool {
-        // XXX
-        true
+        self.2
     }
 }
 
@@ -218,8 +217,14 @@ pub fn validate_named_args(
             }
             arg_attr.typ
         };
+
         if param_type.is_mut(context.db()) && !arg_type.is_mut(context.db()) {
-            context.error("XXX need mut", arg.span, "make this `mut`");
+            let msg = if let Some(label) = param.label() {
+                format!("`{}` argument `{}` must be mutable", name, label)
+            } else {
+                format!("`{}` argument at position {} must be mutable", name, index)
+            };
+            context.error(&msg, arg.kind.value.span, "is not `mut`");
         }
     }
     Ok(())

crates/analyzer/src/traversal/expressions.rs

@@ -66,7 +66,6 @@ pub fn expect_expr_type(
     Ok(attr)
 }
 
-// XXX remove?
 pub fn value_expr_type(
     context: &mut dyn AnalyzerContext,
     exp: &Node<fe::Expr>,
@@ -284,8 +283,8 @@ fn expr_tuple(
             })
             .collect::<Result<Vec<_>, _>>()?;
 
-        // XXX don't fatal if expected.is_some()
         if !&types.iter().all(|id| id.has_fixed_size(context.db())) {
+            // TODO: doesn't need to be fatal if expected.is_some()
             return Err(FatalError::new(context.error(
                 "variable size types can not be part of tuples",
                 exp.span,
@@ -759,7 +758,6 @@ fn expr_bin_operation(
     ) {
         Err(err) => {
             let diag = add_bin_operations_errors(
-                // XXX deref types?
                 context,
                 &op.kind,
                 left.span,
@@ -1374,7 +1372,7 @@ fn expr_call_enum_constructor(
             );
         }
         EnumVariantKind::Tuple(elts) => {
-            let params: Vec<_> = elts.iter().map(|ty| (None, Ok(*ty))).collect();
+            let params: Vec<_> = elts.iter().map(|ty| (None, Ok(*ty), true)).collect();
             validate_named_args(context, name, name_span, args, &params)?;
         }
     }
@@ -1423,11 +1421,7 @@ fn expr_call_method(
         check_for_call_to_special_fns(context, &field.kind, field.span)?;
     }
 
-    match obj_type
-        .function_sigs(context.db(), &field.kind)
-        .to_vec()
-        .as_slice()
-    {
+    match obj_type.function_sigs(context.db(), &field.kind).as_ref() {
         [] => Err(FatalError::new(context.fancy_error(
             &format!(
                 "No function `{}` exists on type `{}`",
@@ -1460,6 +1454,17 @@ fn expr_call_method(
             }
 
             let sig = method.signature(context.db());
+
+            if matches!(sig.self_decl.map(|d| d.is_mut()), Some(true))
+                && !target_attributes.typ.is_mut(context.db())
+            {
+                context.error(
+                    &format!("`{}` takes `mut self`", &field.kind),
+                    target.span,
+                    "this is not mutable",
+                );
+            }
+
             validate_named_args(context, &field.kind, field.span, args, &sig.params)?;
 
             let calltype = match obj_type.typ(context.db()) {
@@ -1764,7 +1769,6 @@ fn expr_call_type_attribute(
                 if let Some(attrs) = arg_attributes.get(i) {
                     if i == 0 {
                         if let Some(ctx_type) = context.get_context_type() {
-                            // XXX require mut ctx
                             if attrs.typ != Type::Mut(ctx_type).id(context.db()) {
                                 context.fancy_error(
                                     &format!(
@@ -1991,7 +1995,6 @@ fn expr_ternary(
         let if_attr = expr(context, if_expr, expected_type)?;
         let else_attr = expr(context, else_expr, expected_type.or(Some(if_attr.typ)))?;
 
-        // XXX expect_types_to_match(
         // Should have the same return Type
         if if_attr.typ != else_attr.typ {
             let if_expr_ty = deref_type(context, exp, if_attr.typ);

crates/analyzer/src/traversal/types.rs

@@ -15,6 +15,7 @@ use fe_common::utils::humanize::pluralize_conditionally;
 use fe_common::Spanned;
 use fe_parser::ast;
 use fe_parser::node::{Node, Span};
+use std::cmp::Ordering;
 
 /// Try to perform an explicit type cast, eg `u256(my_address)` or `address(my_contract)`.
 /// Returns nothing. Emits an error if the cast fails; explicit cast failures are not fatal.
@@ -47,7 +48,6 @@ pub fn try_cast_type(
                     ),
                 );
             } else {
-                // XXX should this be here?
                 adjust_type(context, from_expr, into, AdjustmentKind::StringSizeIncrease);
             }
         }
@@ -174,7 +174,6 @@ fn coerce(
     }
 
     match (from.typ(context.db()), into.typ(context.db())) {
-        // XXX insert copy, deref rhs, rm assignment heuristic in codegen?
         (Type::SPtr(from), Type::SPtr(into)) => {
             coerce(context, from_expr, from, into, false, chain)
         }
@@ -215,21 +214,18 @@ fn coerce(
         // Note that no `Adjustment` is added here.
         (_, Type::SPtr(into)) => coerce(context, from_expr, from, into, false, chain),
 
-        // XXX handle should_copy
         (
             Type::String(FeString { max_size: from_sz }),
             Type::String(FeString { max_size: into_sz }),
-        ) => {
-            if into_sz >= from_sz {
-                Ok(add_adjustment(
-                    chain,
-                    into,
-                    AdjustmentKind::StringSizeIncrease,
-                ))
-            } else {
-                Err(TypeCoercionError::Incompatible)
-            }
-        }
+        ) => match into_sz.cmp(&from_sz) {
+            Ordering::Equal => Ok(chain),
+            Ordering::Greater => Ok(add_adjustment(
+                chain,
+                into,
+                AdjustmentKind::StringSizeIncrease,
+            )),
+            Ordering::Less => Err(TypeCoercionError::Incompatible),
+        },
         (Type::SelfContract(from), Type::Contract(into)) => {
             if from == into {
                 Err(TypeCoercionError::SelfContractType)
@@ -264,7 +260,6 @@ fn coerce(
             Err(TypeCoercionError::Incompatible)
         }
 
-        // XXX check?
         (Type::Base(Base::Numeric(f)), Type::Base(Base::Numeric(i))) => {
             if f.is_signed() == i.is_signed() && i.size() > f.size() {
                 Ok(add_adjustment(chain, into, AdjustmentKind::IntSizeIncrease))
@@ -393,7 +388,7 @@ pub fn apply_generic_type_args(
 
             (GenericParamKind::PrimitiveType, ast::GenericArg::TypeDesc(type_node)) => {
                 let typ = type_desc(context, type_node)?;
-                if typ.is_base(context.db()) {
+                if typ.is_primitive(context.db()) {
                     Ok(GenericArg::Type(typ))
                 } else {
                     Err(TypeError::new(context.error(