Fix nullablility rules for field access (#394)

* [ci skip] Add test assign to nullable class field

* [ci skip] Fix field access constraint generation

However now we have an issue that primitive
comparisons seems to trip up.

Something is up with how we generate constraints
on the fly for field access in conjunction with
function access.

If we flip back the field_ty_exp, then we no
longer get the failed definition_ast_verify test.
It seems that if we fix the issue with the Union,
we get nullability check issues and vice versa.

* Some cleanup

* Substitute also assumes constraints mutable ref

- reinsert also assumes constraint mutable ref
- Improve Position Display
- Improve Constraint Display
- Improve access logic

* [ci skip] Field type super in field access gen

* Add exception that Name may be super of any

This means that in certain situations, a Name is
a superset of another if it is a superset of just
one of the types of other.

For now, this is useful at a function call site
where we generate such constraints on the fly.
There, we only need the current expression to be
super of one.

However, seeing as we only have one use case at
the moment, I'm not sure this logic belongs in
Name itself.
Perhaps this might be useful in future for fixing
other issues.
Issues such as #296 come to mind.

- Unignore certain mutability test.

* Cleanup

src/check/constrain/constraint/builder.rs

@@ -1,4 +1,4 @@
-use crate::check::constrain::constraint::Constraint;
+use crate::check::constrain::constraint::{Constraint, ConstrVariant};
 use crate::check::constrain::constraint::expected::Expected;
 use crate::check::constrain::constraint::iterator::Constraints;
 use crate::check::name::string_name::StringName;
@@ -73,8 +73,12 @@ impl ConstrBuilder {
         self.add_constr(&Constraint::new(msg, parent, child));
     }
 
+    pub fn add_var(&mut self, msg: &str, parent: &Expected, child: &Expected, var: ConstrVariant) {
+        self.add_constr(&Constraint::new_variant(msg, parent, child, var));
+    }
+
     pub fn add_constr(&mut self, constraint: &Constraint) {
-        trace!("Constr: {} == {}, {}: {}", constraint.left.pos, constraint.right.pos, self.level, constraint);
+        trace!("Constr[{}]: {} == {}, {}: {}", self.level, constraint.left.pos, constraint.right.pos, constraint.msg, constraint);
         self.constraints[self.level].1.push(constraint.clone())
     }
 

src/check/constrain/constraint/iterator.rs

@@ -36,12 +36,6 @@ impl Constraints {
         self.constraints.push_front(constraint)
     }
 
-    /// Append in_class and constraints of constraints to self
-    pub fn append(&mut self, constraints: &mut Constraints) {
-        self.in_class.append(&mut constraints.in_class);
-        self.constraints.append(&mut constraints.constraints);
-    }
-
     pub fn push_constr(&mut self, constr: &Constraint) {
         self.constraints.push_back(constr.clone())
     }

src/check/constrain/constraint/mod.rs

@@ -20,20 +20,26 @@ pub struct Constraint {
     pub superset: ConstrVariant,
 }
 
-#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 pub enum ConstrVariant {
     Left,
     Right,
 }
 
+impl Default for ConstrVariant {
+    fn default() -> Self {
+        ConstrVariant::Left
+    }
+}
+
 impl Display for Constraint {
     fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), Error> {
         let superset = match &self.superset {
-            ConstrVariant::Left => "{left}  ",
-            ConstrVariant::Right => "{right}  "
+            ConstrVariant::Left => ">=",
+            ConstrVariant::Right => "<="
         };
 
-        write!(f, "{}{} == {}", superset, self.left, self.right)
+        write!(f, "{} {superset} {}", self.left, self.right)
     }
 }
 
@@ -42,10 +48,10 @@ impl Constraint {
     ///
     /// By default, the left side is assumed to be the superset of the right side.
     pub fn new(msg: &str, parent: &Expected, child: &Expected) -> Constraint {
-        Constraint::new_variant(msg, parent, child, &ConstrVariant::Left)
+        Constraint::new_variant(msg, parent, child, ConstrVariant::default())
     }
 
-    pub fn new_variant(msg: &str, parent: &Expected, child: &Expected, superset: &ConstrVariant)
+    pub fn new_variant(msg: &str, parent: &Expected, child: &Expected, superset: ConstrVariant)
                        -> Constraint {
         Constraint {
             left: parent.clone(),
@@ -61,36 +67,26 @@ impl Constraint {
     fn flag(&self) -> Constraint { Constraint { is_flag: true, ..self.clone() } }
 
     pub fn stringy(msg: &str, expected: &Expected) -> Constraint {
-        let string =
-            Expected::new(expected.pos, &Type { name: Name::from(clss::STRING) });
-        let access = Access {
-            entity: Box::from(expected.clone()),
-            name: Box::new(Expected::new(expected.pos, &Function {
-                name: StringName::from(function::STR),
-                args: vec![expected.clone()],
-            })),
-        };
-
-        Constraint::new(msg, &string, &Expected::new(expected.pos, &access))
+        Self::access(msg, expected, &Name::from(clss::STRING), &StringName::from(function::STR))
     }
 
     pub fn truthy(msg: &str, expected: &Expected) -> Constraint {
-        let bool =
-            Expected::new(expected.pos, &Type { name: Name::from(clss::BOOL) });
+        Self::access(msg, expected, &Name::from(clss::BOOL), &StringName::from(function::TRUTHY))
+    }
+
+    fn access(msg: &str, expected: &Expected, ty_name: &Name, fun_name: &StringName) -> Constraint {
+        let obj = Expected::new(expected.pos, &Type { name: ty_name.clone() });
+        let fun = Function { name: fun_name.clone(), args: vec![expected.clone()] };
         let access = Access {
             entity: Box::from(expected.clone()),
-            name: Box::new(Expected::new(expected.pos, &Function {
-                name: StringName::from(function::TRUTHY),
-                args: vec![expected.clone()],
-            })),
+            name: Box::new(Expected::new(expected.pos, &fun)),
         };
 
-        Constraint::new(msg, &bool, &Expected::new(expected.pos, &access))
+        Constraint::new(msg, &obj, &Expected::new(expected.pos, &access))
     }
 
     pub fn undefined(msg: &str, expected: &Expected) -> Constraint {
-        let none =
-            Expected::new(expected.pos, &Type { name: Name::from(clss::NONE) });
+        let none = Expected::new(expected.pos, &Type { name: Name::from(clss::NONE) });
         Constraint::new(msg, expected, &none)
     }
 }

src/check/constrain/generate/call.rs

@@ -48,8 +48,8 @@ pub fn gen_call(
 
                 constr.add(
                     "reassign",
-                    &Expected::try_from((left, &env_assigned_to.var_mappings))?,
-                    &Expected::try_from((right, &env_assigned_to.var_mappings))?,
+                    &Expected::try_from((left, &env.var_mappings))?,
+                    &Expected::try_from((right, &env.var_mappings))?,
                 );
                 generate(right, &env_assigned_to, ctx, constr)?;
                 generate(left, &env_assigned_to, ctx, constr)?;
@@ -103,7 +103,7 @@ pub fn gen_call(
                     &fun_ret_exp,
                 );
 
-                check_raises_caught(&constr, &fun.raises.names, env, ctx, ast.pos)?;
+                check_raises_caught(constr, &fun.raises.names, env, ctx, ast.pos)?;
                 env.clone()
             })
         }

src/check/constrain/generate/class.rs

@@ -31,7 +31,7 @@ pub fn gen_class(
         Node::TypeAlias { conditions, isa, .. } => constrain_class_body(conditions, isa, env, ctx, constr),
         Node::Condition { cond, el: Some(el) } => {
             generate(cond, env, ctx, constr)?;
-            generate(el, &env, ctx, constr)
+            generate(el, env, ctx, constr)
         }
         Node::Condition { cond, .. } => generate(cond, env, ctx, constr),
 
@@ -82,13 +82,13 @@ pub fn property_from_field(
     let field_ty = Expected::new(pos, &Type { name: field.ty.clone() });
 
     let env = env.insert_var(field.mutable, &field.name, &field_ty);
-    constr.add("field property", &field_ty, &property_call);
+    constr.add("class field type", &field_ty, &property_call);
 
     let access = Expected::new(pos, &Access {
         entity: Box::new(Expected::new(pos, &Type { name: Name::from(class) })),
         name: Box::new(Expected::new(pos, &Field { name: field.name.clone() })),
     });
 
-    constr.add("field property", &property_call, &access);
+    constr.add("class field access", &property_call, &access);
     Ok(env)
 }

src/check/constrain/generate/control_flow.rs

@@ -60,10 +60,8 @@ pub fn gen_flow(
                 let then = Expected::try_from((then, &then_env.var_mappings))?;
                 let el = Expected::try_from((el, &else_env.var_mappings))?;
 
-                let then_constr = Constraint::new_variant("if then branch", &if_expr, &then, &ConstrVariant::Left);
-                constr.add_constr(&then_constr);
-                let else_constr = Constraint::new_variant("if else branch", &if_expr, &el, &ConstrVariant::Left);
-                constr.add_constr(&else_constr);
+                constr.add_var("if then branch", &if_expr, &then, ConstrVariant::default());
+                constr.add_var("if else branch", &if_expr, &el, ConstrVariant::default());
             }
 
             constr.exit_set(ast.pos)?;

src/check/constrain/generate/definition.rs

@@ -195,7 +195,7 @@ pub fn identifier_from_var(
 
     let var_expect = Expected::try_from((var, &env_with_var.var_mappings))?;
     if let Some(ty) = ty {
-        let ty_exp = Expected::new(ty.pos, &Type { name: Name::try_from(ty.deref())? });
+        let ty_exp = Expected::new(var.pos, &Type { name: Name::try_from(ty.deref())? });
         constr.add("variable and type", &ty_exp, &var_expect);
     }
     if let Some(expr) = expr {

src/check/constrain/unify/expression/mod.rs

@@ -21,32 +21,26 @@ pub fn unify_expression(constraint: &Constraint, constraints: &mut Constraints,
         // Not sure if necessary, but exception made for tuple
         (Tuple { elements }, Expression { ast: AST { node: Node::Tuple { elements: ast_elements }, .. } }) |
         (Expression { ast: AST { node: Node::Tuple { elements: ast_elements }, .. } }, Tuple { elements }) => {
-            let mut constraints = substitute(left, right, constraints, count, total)?;
+            substitute(constraints, left, right, count, total)?;
 
             for pair in ast_elements.iter().cloned().zip_longest(elements.iter()) {
                 match &pair {
                     Both(ast, exp) => {
                         let expect = Expression { ast: ast.clone() };
-                        let l_ty = Expected::new(left.pos, &expect);
-                        constraints.push("tuple", &l_ty, exp)
+                        constraints.push("tuple", &Expected::new(left.pos, &expect), exp)
                     }
                     _ => {
                         let msg = format!("Expected tuple with {} elements, was {}", elements.len(), ast_elements.len());
                         return Err(vec![TypeErr::new(left.pos, &msg)]);
                     }
                 }
             }
-
-            unify_link(&mut constraints, finished, ctx, total)
         }
 
-        (Expression { .. }, _) if constraint.superset == ConstrVariant::Left => {
-            let mut constraints = substitute(right, left, constraints, count, total)?;
-            unify_link(&mut constraints, finished, ctx, total)
-        }
-        _ => {
-            let mut constraints = substitute(left, right, constraints, count, total)?;
-            unify_link(&mut constraints, finished, ctx, total)
-        }
+        (Expression { .. }, _) if constraint.superset == ConstrVariant::Left =>
+            substitute(constraints, right, left, count, total)?,
+        _ => substitute(constraints, left, right, count, total)?
     }
+
+    unify_link(constraints, finished, ctx, total)
 }

src/check/constrain/unify/expression/substitute.rs

@@ -1,6 +1,6 @@
 use crate::check::constrain::constraint::expected::{Expect, Expected};
 use crate::check::constrain::constraint::iterator::Constraints;
-use crate::check::result::TypeResult;
+use crate::check::constrain::Unified;
 
 /// Substitute old expression with new.
 ///
@@ -13,32 +13,29 @@ use crate::check::result::TypeResult;
 /// If identifier override detected, only substitute right hand side of
 /// unification before ceasing substitution.
 pub fn substitute(
+    constraints: &mut Constraints,
     new: &Expected,
     old: &Expected,
-    constraints: &mut Constraints,
     offset: usize,
     total: usize,
-) -> TypeResult<Constraints> {
-    let mut substituted = Constraints::new();
+) -> Unified<()> {
     let mut constraint_pos = offset;
-
     trace!("{:width$} [subbing {}\\{}]  {}  <=  {}", "", offset, total, old, new, width = 30);
 
-    while let Some(mut constr) = constraints.pop_constr() {
-        let old_constr = constr.clone();
+    for _ in 0..constraints.len() {
+        let mut constr = constraints.pop_constr().expect("Cannot be empty");
         constraint_pos += 1;
         macro_rules! replace {
             ($left:expr, $new:expr) => {{
-                let pos =
-                    format!("({}={}) ", old_constr.left.pos, old_constr.right.pos);
+                let pos = format!("({}={}) ", constr.left.pos, constr.right.pos);
                 let side = if $left { "l" } else { "r" };
                 trace!(
                     "{:width$} [subbed {}\\{} {}]  {}  =>  {}",
                     pos,
                     constraint_pos,
                     total,
                     side,
-                    old_constr,
+                    constr,
                     $new,
                     width = 32
                 );
@@ -55,11 +52,10 @@ pub fn substitute(
         }
 
         constr.is_sub = constr.is_sub || sub_l || sub_r;
-        substituted.push_constr(&constr)
+        constraints.push_constr(&constr);
     }
 
-    substituted.append(constraints);
-    Ok(substituted)
+    Ok(())
 }
 
 fn recursive_substitute(
@@ -81,7 +77,7 @@ fn recursive_substitute(
             (subs_e || sub_n, Expected::new(inspected.pos, &expect))
         }
         Expect::Tuple { elements } => {
-            let (elements, any_substituted) = substitute_vec(side, old, new, elements);
+            let (any_substituted, elements) = substitute_vec(side, old, new, elements);
             (any_substituted, Expected::new(inspected.pos, &Expect::Tuple { elements }))
         }
         Expect::Collection { ty } => {
@@ -90,32 +86,24 @@ fn recursive_substitute(
             (subs_ty, Expected::new(inspected.pos, &expect))
         }
         Expect::Function { name, args } => {
-            let (args, any_substituted) = substitute_vec(side, old, new, args);
+            let (any_substituted, args) = substitute_vec(side, old, new, args);
             let func = Expect::Function { name: name.clone(), args };
             (any_substituted, Expected::new(inspected.pos, &func))
         }
         _ => (false, inspected.clone()),
     }
 }
 
-/// Substitues all in vector, and returns True if any substituted.
+/// Substitute all in vector, and also returns True if any substituted.
 fn substitute_vec(
     side: &str,
     old: &Expected,
     new: &Expected,
     elements: &[Expected],
-) -> (Vec<Expected>, bool) {
-    let mut any_substituted = false;
+) -> (bool, Vec<Expected>) {
+    let elements = elements
+        .iter()
+        .map(|e| recursive_substitute(side, e, old, new));
 
-    (
-        elements
-            .iter()
-            .map(|e| {
-                let (subs, el) = recursive_substitute(side, e, old, new);
-                any_substituted = any_substituted || subs;
-                el
-            })
-            .collect(),
-        any_substituted,
-    )
+    (elements.clone().any(|(i, _)| i), elements.map(|(_, i)| i).collect())
 }