Fix type check and inference issues in references, structs, and enums (…

…FuelLabs#5643)

## Description

This PR:
- fixes FuelLabs#5559, FuelLabs#5597, and FuelLabs#5492 by removing the `TypeInfo::Unknown` and
providing the required contextual information to type checking of
referencing, dereferencing, `if`, and `match` expressions respectively.
The contextual information provided is taken from the
`ctx.type_annotation()` but always adapted according to the semantics of
the type-checked expression.
- fixes FuelLabs#5583 and FuelLabs#5581 by combining the contextual information coming
from the `ctx.type_annotation()` with the one coming from the enum and
struct instantiation and declaration.
- fixes FuelLabs#5598 by forcing the name-based and not structure-based
identity. In other words, two enums or structs are considered equal only
if they whole `call_path`s are equal. Up to now, we were expecting only
the enum or struct _names_ to be equal, which was treating types with
same names and structures (variants or fields) as equal although they
were defined in different modules.

The PR also introduces parsing of references to mutable values (`&mut
T`). Since this addition does not overlap with the above bug fixes, it
was left as is, and can be fully ignored during the review. Other
changes related to references to mutable values are removed from the
code from type-checking onward to make this PR only fixing the issues.
Continuation on references to mutable values will be done in a separate
PR.

Closes FuelLabs#5559, FuelLabs#5597, FuelLabs#5492, FuelLabs#5583, FuelLabs#5581, FuelLabs#5598.

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.

sway-ast/src/expr/mod.rs

@@ -76,6 +76,7 @@ pub enum Expr {
     },
     Ref {
         ampersand_token: AmpersandToken,
+        mut_token: Option<MutToken>,
         expr: Box<Expr>,
     },
     Deref {
@@ -240,6 +241,7 @@ impl Spanned for Expr {
             Expr::Ref {
                 ampersand_token,
                 expr,
+                ..
             } => Span::join(ampersand_token.span(), expr.span()),
             Expr::Deref { star_token, expr } => Span::join(star_token.span(), expr.span()),
             Expr::Not { bang_token, expr } => Span::join(bang_token.span(), expr.span()),

sway-ast/src/ty/mod.rs

@@ -24,6 +24,7 @@ pub enum Ty {
     },
     Ref {
         ampersand_token: AmpersandToken,
+        // TODO-IG: Extend to support references to mutable values.
         ty: Box<Ty>,
     },
     Never {

sway-core/src/language/parsed/expression/mod.rs

@@ -321,10 +321,17 @@ pub enum ExpressionKind {
     /// semicolon, denoting that it is the [Expression] to be returned from that block.
     ImplicitReturn(Box<Expression>),
     Return(Box<Expression>),
-    Ref(Box<Expression>),
+    Ref(RefExpression),
     Deref(Box<Expression>),
 }
 
+#[derive(Debug, Clone)]
+pub struct RefExpression {
+    /// True if the reference is a reference to a mutable `value`.
+    pub to_mutable_value: bool,
+    pub value: Box<Expression>,
+}
+
 #[derive(Debug, Clone)]
 pub enum ReassignmentTarget {
     VariableExpression(Box<Expression>),

sway-core/src/language/ty/declaration/enum.rs

@@ -36,7 +36,7 @@ impl Named for TyEnumDecl {
 impl EqWithEngines for TyEnumDecl {}
 impl PartialEqWithEngines for TyEnumDecl {
     fn eq(&self, other: &Self, engines: &Engines) -> bool {
-        self.call_path.suffix == other.call_path.suffix
+        self.call_path == other.call_path
             && self.type_parameters.eq(&other.type_parameters, engines)
             && self.variants.eq(&other.variants, engines)
             && self.visibility == other.visibility
@@ -55,7 +55,7 @@ impl HashWithEngines for TyEnumDecl {
             span: _,
             attributes: _,
         } = self;
-        call_path.suffix.hash(state);
+        call_path.hash(state);
         variants.hash(state, engines);
         type_parameters.hash(state, engines);
         visibility.hash(state);

sway-core/src/language/ty/declaration/struct.rs

@@ -34,7 +34,7 @@ impl Named for TyStructDecl {
 impl EqWithEngines for TyStructDecl {}
 impl PartialEqWithEngines for TyStructDecl {
     fn eq(&self, other: &Self, engines: &Engines) -> bool {
-        self.call_path.suffix == other.call_path.suffix
+        self.call_path == other.call_path
             && self.fields.eq(&other.fields, engines)
             && self.type_parameters.eq(&other.type_parameters, engines)
             && self.visibility == other.visibility
@@ -53,7 +53,7 @@ impl HashWithEngines for TyStructDecl {
             span: _,
             attributes: _,
         } = self;
-        call_path.suffix.hash(state);
+        call_path.hash(state);
         fields.hash(state, engines);
         type_parameters.hash(state, engines);
         visibility.hash(state);

sway-core/src/semantic_analysis/ast_node/declaration/declaration.rs

@@ -54,14 +54,29 @@ impl TyDecl {
                     ty::TyExpression::error(err, var_decl.name.span(), engines)
                 });
 
+                // TODO: Integers shouldn't be anything special. RHS expressions should be written in
+                //       a way to always use the context provided from the LHS, and if the LHS is
+                //       an integer, RHS should properly unify or type check should fail.
+                //       Remove this special case as a part of the initiative of improving type inference.
                 // Integers are special in the sense that we can't only rely on the type of `body`
                 // to get the type of the variable. The type of the variable *has* to follow
                 // `type_ascription` if `type_ascription` is a concrete integer type that does not
                 // conflict with the type of `body` (i.e. passes the type checking above).
                 let return_type = match &*type_engine.get(type_ascription.type_id) {
                     TypeInfo::UnsignedInteger(_) => type_ascription.type_id,
-                    _ => body.return_type,
+                    _ => match &*type_engine.get(body.return_type) {
+                        // If RHS type check ends up in an error we want to use the
+                        // provided type ascription as the variable type. E.g.:
+                        //   let v: Struct<u8> = Struct<u64> { x: 0 }; // `v` should be "Struct<u8>".
+                        //   let v: ExistingType = non_existing_identifier; // `v` should be "ExistingType".
+                        //   let v = <some error>; // `v` will remain "{unknown}".
+                        // TODO: Refine and improve this further. E.g.,
+                        //   let v: Struct { /* MISSING FIELDS */ }; // Despite the error, `v` should be of type "Struct".
+                        TypeInfo::ErrorRecovery(_) => type_ascription.type_id,
+                        _ => body.return_type,
+                    },
                 };
+
                 let typed_var_decl = ty::TyDecl::VariableDecl(Box::new(ty::TyVariableDecl {
                     name: var_decl.name.clone(),
                     body,

sway-core/src/semantic_analysis/ast_node/expression/match_expression/typed/typed_match_branch.rs

@@ -85,14 +85,14 @@ impl ty::TyMatchBranch {
             )?;
 
         // create a new namespace for this branch result
-        ctx.scoped(|mut branch_ctx| {
+        ctx.scoped(|mut scoped_ctx| {
             // for every variable that comes into result block, create a variable declaration,
             // insert it into the branch namespace, and add it to the block of code statements
             let mut code_block_contents: Vec<ty::TyAstNode> = vec![];
 
             for (var_ident, var_body) in result_var_declarations {
                 let var_decl = instantiate.var_decl(var_ident.clone(), var_body.clone());
-                let _ = branch_ctx.insert_symbol(handler, var_ident.clone(), var_decl.clone());
+                let _ = scoped_ctx.insert_symbol(handler, var_ident.clone(), var_decl.clone());
                 code_block_contents.push(ty::TyAstNode {
                     content: ty::TyAstNodeContent::Declaration(var_decl),
                     span: var_ident.span(),
@@ -101,18 +101,34 @@ impl ty::TyMatchBranch {
 
             // type check the branch result
             let typed_result = {
-                let ctx = branch_ctx.by_ref().with_type_annotation(type_engine.insert(
+                // If there is an expectation coming from the context via `ctx.type_annotation()` we need
+                // to pass that contextual requirement to the branch in order to provide more specific contextual
+                // information. E.g., that `Option<u8>` is expected.
+                // But at the same time, we do not want to unify during type checking with that contextual information
+                // at this stage, because the branch might get `TypeInfo::Unknown` as the expectation and diverge
+                // at the same time. The divergence would unify `TypeInfo::Never` and `Unknown` in that case, leaving
+                // `Never` as the expected type for the subsequent branches.
+                // In order to pass the contextual information, but not to affect the original type with potential
+                // unwanted unification with `Never`, we create a copies of the `ctx.type_annotation()` type and pass
+                // it as the expectation to the branch.
+                let type_annotation = (*type_engine.get(scoped_ctx.type_annotation())).clone();
+                let branch_ctx = scoped_ctx.by_ref().with_type_annotation(type_engine.insert(
                     engines,
-                    TypeInfo::Unknown,
+                    type_annotation,
                     None,
                 ));
-                ty::TyExpression::type_check(handler, ctx, result)?
+                ty::TyExpression::type_check(handler, branch_ctx, result)?
             };
 
             // Check if return type is Never if it is we don't unify as it would replace the Unknown annotation with Never.
             if !matches!(*type_engine.get(typed_result.return_type), TypeInfo::Never) {
                 // unify the return type from the typed result with the type annotation
-                branch_ctx.unify_with_type_annotation(
+                // Note here that the `scoped_ctx` is actually the original `ctx` just scoped
+                // to the `namespace`, thus, having the same original type annotation.
+                // This unification is also the mechanism for carrying the type of a branch to
+                // the subsequent branch. It potentially alters the type behind the `ctx.type_annotation()`
+                // which will then be picked by the next branch.
+                scoped_ctx.unify_with_type_annotation(
                     handler,
                     typed_result.return_type,
                     &typed_result.span,

sway-core/src/semantic_analysis/ast_node/expression/typed_expression.rs

@@ -414,7 +414,10 @@ impl ty::TyExpression {
                 };
                 Ok(typed_expr)
             }
-            ExpressionKind::Ref(expr) => Self::type_check_ref(handler, ctx.by_ref(), expr, span),
+            ExpressionKind::Ref(RefExpression {
+                to_mutable_value,
+                value,
+            }) => Self::type_check_ref(handler, ctx.by_ref(), to_mutable_value, value, span),
             ExpressionKind::Deref(expr) => {
                 Self::type_check_deref(handler, ctx.by_ref(), expr, span)
             }
@@ -648,31 +651,47 @@ impl ty::TyExpression {
             ty::TyExpression::type_check(handler, ctx, condition.clone())
                 .unwrap_or_else(|err| ty::TyExpression::error(err, condition.span(), engines))
         };
+
+        // The final type checking and unification, as well as other semantic requirement like the same type
+        // in the `then` and `else` branch are done in the `instantiate_if_expression`.
+        // However, if there is an expectation coming from the context via `ctx.type_annotation()` we need
+        // to pass that contextual requirement to both branches in order to provide more specific contextual
+        // information. E.g., that `Option<u8>` is expected.
+        // But at the same time, we do not want to unify during type checking with that contextual information
+        // at this stage, because the unification will be done in the `instantiate_if_expression`.
+        // In order to pass the contextual information, but not to affect the original type with premature
+        // unification, we create two copies of the `ctx.type_annotation()` type and pass them as the
+        // expectation to both branches.
+        let type_annotation = (*type_engine.get(ctx.type_annotation())).clone();
+
         let then = {
             let ctx = ctx
                 .by_ref()
                 .with_help_text("")
                 .with_type_annotation(type_engine.insert(
                     engines,
-                    TypeInfo::Unknown,
+                    type_annotation.clone(),
                     then.span().source_id(),
                 ));
             ty::TyExpression::type_check(handler, ctx, then.clone())
                 .unwrap_or_else(|err| ty::TyExpression::error(err, then.span(), engines))
         };
+
         let r#else = r#else.map(|expr| {
             let ctx = ctx
                 .by_ref()
                 .with_help_text("")
                 .with_type_annotation(type_engine.insert(
                     engines,
-                    TypeInfo::Unknown,
+                    type_annotation,
                     expr.span().source_id(),
                 ));
             ty::TyExpression::type_check(handler, ctx, expr.clone())
                 .unwrap_or_else(|err| ty::TyExpression::error(err, expr.span(), engines))
         });
+
         let exp = instantiate_if_expression(handler, ctx, condition, then.clone(), r#else, span)?;
+
         Ok(exp)
     }
 
@@ -2064,21 +2083,34 @@ impl ty::TyExpression {
     fn type_check_ref(
         handler: &Handler,
         mut ctx: TypeCheckContext<'_>,
-        expr: Box<Expression>,
+        _to_mutable_value: bool,
+        value: Box<Expression>,
         span: Span,
     ) -> Result<ty::TyExpression, ErrorEmitted> {
         let engines = ctx.engines();
         let type_engine = ctx.engines().te();
 
-        // We need to remove the type annotation, because the type expected from the context will
-        // be the reference type, and we are checking the referenced type.
+        // Get the type annotation.
+        // If the type provided by the context is a reference, we expect the type of the `value`
+        // to be the referenced type of that reference.
+        // Otherwise, we have a wrong expectation coming from the context. So we will pass a new
+        // `TypeInfo::Unknown` as the annotation, to allow the `value` to be evaluated
+        // without any expectations. That value will at the end not unify with the type
+        // annotation coming from the context and a type-mismatch error will be emitted.
+        let type_annotation = match &*type_engine.get(ctx.type_annotation()) {
+            TypeInfo::Ref(referenced_type) => referenced_type.type_id,
+            _ => type_engine.insert(engines, TypeInfo::Unknown, None),
+        };
+
         let ctx = ctx
             .by_ref()
-            .with_type_annotation(type_engine.insert(engines, TypeInfo::Unknown, None))
+            .with_type_annotation(type_annotation)
             .with_help_text("");
-        let expr_span = expr.span();
-        let expr = ty::TyExpression::type_check(handler, ctx, *expr)
+
+        let expr_span = value.span();
+        let expr = ty::TyExpression::type_check(handler, ctx, *value)
             .unwrap_or_else(|err| ty::TyExpression::error(err, expr_span.clone(), engines));
+
         let expr_type_argument: TypeArgument = expr.return_type.into();
         let typed_expr = ty::TyExpression {
             expression: ty::TyExpressionVariant::Ref(Box::new(expr)),
@@ -2098,14 +2130,24 @@ impl ty::TyExpression {
         let engines = ctx.engines();
         let type_engine = ctx.engines().te();
 
-        // We need to remove the type annotation, because the type expected from the context will
-        // be the referenced type, and we are checking the reference type.
-        let ctx = ctx
+        // Get the type annotation.
+        // If there is an expectation coming from the context, i.e., if the context
+        // type is not `TypeInfo::Unknown`, we expect the type of the `expr` to be a
+        // reference to the expected type.
+        // Otherwise, we pass a new `TypeInfo::Unknown` as the annotation, to allow the `expr`
+        // to be evaluated without any expectations.
+        let type_annotation = match &*type_engine.get(ctx.type_annotation()) {
+            TypeInfo::Unknown => type_engine.insert(engines, TypeInfo::Unknown, None),
+            _ => type_engine.insert(engines, TypeInfo::Ref(ctx.type_annotation().into()), None),
+        };
+
+        let deref_ctx = ctx
             .by_ref()
-            .with_type_annotation(type_engine.insert(engines, TypeInfo::Unknown, None))
+            .with_type_annotation(type_annotation)
             .with_help_text("");
+
         let expr_span = expr.span();
-        let expr = ty::TyExpression::type_check(handler, ctx, *expr)
+        let expr = ty::TyExpression::type_check(handler, deref_ctx, *expr)
             .unwrap_or_else(|err| ty::TyExpression::error(err, expr_span.clone(), engines));
 
         let expr_type = type_engine.get(expr.return_type);