fix: Apply trait constraints from method calls (#4152)

# Description

## Problem\*

Resolves #4124
Resolves #4095

## Summary\*

We were never applying trait constraints from method calls before. These
have been handled for other identifiers since #4000, but not for method
calls which desugar to a function identifier that is called, then type
checked with its own special function. I've fixed this by removing the
special function and recursively type checking the function call they
desugar to instead. This way we have less code duplication and only need
to fix things in one spot in the future.

## Additional Context

It is a good day when you get to fix a bug by removing code.

This is a draft currently because I still need:
- [x] To add `&mut` implicitly where applicable to the function calls
that are now checked recursively
- [x] To add the test case I'm using locally

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

Author: jfecher

compiler/noirc_frontend/src/hir/resolution/resolver.rs

@@ -1450,6 +1450,8 @@ impl<'a> Resolver<'a> {
                 HirExpression::MemberAccess(HirMemberAccess {
                     lhs: self.resolve_expression(access.lhs),
                     rhs: access.rhs,
+                    // This is only used when lhs is a reference and we want to return a reference to rhs
+                    is_offset: false,
                 })
             }
             ExpressionKind::Error => HirExpression::Error,

compiler/noirc_frontend/src/hir/type_check/expr.rs

@@ -206,24 +206,22 @@ impl<'interner> TypeChecker<'interner> {
                 let object_ref = &mut object;
                 let mutable_ref = &mut mutable;
 
+                let dereference_lhs = move |_: &mut Self, _, element_type| {
+                    // We must create a temporary value first to move out of object_ref before
+                    // we eventually reassign to it.
+                    let id = DefinitionId::dummy_id();
+                    let location = Location::new(span, fm::FileId::dummy());
+                    let ident = HirIdent::non_trait_method(id, location);
+                    let tmp_value = HirLValue::Ident(ident, Type::Error);
+
+                    let lvalue = std::mem::replace(object_ref, Box::new(tmp_value));
+                    *object_ref = Box::new(HirLValue::Dereference { lvalue, element_type });
+                    *mutable_ref = true;
+                };
+
+                let name = &field_name.0.contents;
                 let (object_type, field_index) = self
-                    .check_field_access(
-                        &lhs_type,
-                        &field_name.0.contents,
-                        span,
-                        move |_, _, element_type| {
-                            // We must create a temporary value first to move out of object_ref before
-                            // we eventually reassign to it.
-                            let id = DefinitionId::dummy_id();
-                            let location = Location::new(span, fm::FileId::dummy());
-                            let ident = HirIdent::non_trait_method(id, location);
-                            let tmp_value = HirLValue::Ident(ident, Type::Error);
-
-                            let lvalue = std::mem::replace(object_ref, Box::new(tmp_value));
-                            *object_ref = Box::new(HirLValue::Dereference { lvalue, element_type });
-                            *mutable_ref = true;
-                        },
-                    )
+                    .check_field_access(&lhs_type, name, span, Some(dereference_lhs))
                     .unwrap_or((Type::Error, 0));
 
                 let field_index = Some(field_index);
@@ -325,6 +323,7 @@ impl<'interner> TypeChecker<'interner> {
             // Now check if LHS is the same type as the RHS
             // Importantly, we do not coerce any types implicitly
             let expr_span = self.interner.expr_span(&rhs_expr);
+
             self.unify_with_coercions(&expr_type, &annotated_type, rhs_expr, || {
                 TypeCheckError::TypeMismatch {
                     expected_typ: annotated_type.to_string(),
@@ -335,10 +334,8 @@ impl<'interner> TypeChecker<'interner> {
             if annotated_type.is_unsigned() {
                 self.lint_overflowing_uint(&rhs_expr, &annotated_type);
             }
-            annotated_type
-        } else {
-            expr_type
         }
+        expr_type
     }
 
     /// Check if an assignment is overflowing with respect to `annotated_type`

compiler/noirc_frontend/src/hir/type_check/stmt.rs

@@ -152,6 +152,12 @@ pub struct HirMemberAccess {
     // This field is not an IdentId since the rhs of a field
     // access has no corresponding definition
     pub rhs: Ident,
+
+    /// True if we should return an offset of the field rather than the field itself.
+    /// For most cases this is false, corresponding to `foo.bar` in source code.
+    /// This is true when calling methods or when we have an lvalue we want to preserve such
+    /// that if `foo : &mut Foo` has a field `bar : Bar`, we can return an `&mut Bar`.
+    pub is_offset: bool,
 }
 
 #[derive(Debug, Clone)]
@@ -201,13 +207,14 @@ pub enum HirMethodReference {
 }
 
 impl HirMethodCallExpression {
+    /// Converts a method call into a function call
     pub fn into_function_call(
         mut self,
         method: &HirMethodReference,
         object_type: Type,
         location: Location,
         interner: &mut NodeInterner,
-    ) -> (ExprId, HirExpression) {
+    ) -> HirExpression {
         let mut arguments = vec![self.object];
         arguments.append(&mut self.arguments);
 
@@ -225,9 +232,10 @@ impl HirMethodCallExpression {
                 (id, ImplKind::TraitMethod(*method_id, constraint, false))
             }
         };
-        let expr = HirExpression::Ident(HirIdent { location, id, impl_kind });
-        let func = interner.push_expr(expr);
-        (func, HirExpression::Call(HirCallExpression { func, arguments, location }))
+        let func = HirExpression::Ident(HirIdent { location, id, impl_kind });
+        let func = interner.push_expr(func);
+        interner.push_expr_location(func, location.span, location.file);
+        HirExpression::Call(HirCallExpression { func, arguments, location })
     }
 }
 

compiler/noirc_frontend/src/hir_def/expr.rs

@@ -30,7 +30,9 @@ impl AstPrinter {
 
     pub fn print_expr(&mut self, expr: &Expression, f: &mut Formatter) -> std::fmt::Result {
         match expr {
-            Expression::Ident(ident) => write!(f, "{}${}", ident.name, ident.definition),
+            Expression::Ident(ident) => {
+                write!(f, "{}${}", ident.name, ident.definition)
+            }
             Expression::Literal(literal) => self.print_literal(literal, f),
             Expression::Block(exprs) => self.print_block(exprs, f),
             Expression::Unary(unary) => self.print_unary(unary, f),

compiler/noirc_frontend/src/monomorphization/printer.rs

@@ -0,0 +1,7 @@
+[package]
+name = "regression_4124"
+type = "bin"
+authors = [""]
+compiler_version = ">=0.22.0"
+
+[dependencies]

test_programs/execution_success/regression_4124/Nargo.toml

@@ -0,0 +1 @@
+value = 0

test_programs/execution_success/regression_4124/Prover.toml

@@ -0,0 +1,39 @@
+use dep::std::option::Option;
+
+trait MyDeserialize<N> {
+    fn deserialize(fields: [Field; N]) -> Self;
+}
+
+impl MyDeserialize<1> for Field {
+    fn deserialize(fields: [Field; 1]) -> Self {
+        fields[0]
+    }
+}
+
+pub fn storage_read<N>() -> [Field; N] {
+    dep::std::unsafe::zeroed()
+}
+
+struct PublicState<T> {
+    storage_slot: Field,
+}
+
+impl<T> PublicState<T> {
+    pub fn new(storage_slot: Field) -> Self {
+        assert(storage_slot != 0, "Storage slot 0 not allowed. Storage slots must start from 1.");
+        PublicState { storage_slot }
+    }
+
+    pub fn read<T_SERIALIZED_LEN>(_self: Self) -> T where T: MyDeserialize<T_SERIALIZED_LEN> {
+        // storage_read returns slice here
+        let fields: [Field; T_SERIALIZED_LEN] = storage_read();
+        T::deserialize(fields)
+    }
+}
+
+fn main(value: Field) {
+    let ps: PublicState<Field> = PublicState::new(27);
+
+    // error here
+    assert(ps.read() == value);
+}