Move Environment from TypeChecker to Context

[rben01]

This allows not cloning function signatures during elaboration (and I haven't checked, but may obviate other clones as well)

[sharkdp]

Hm, this complicates the code quite a bit. Do we see significant performance improvements?

[rben01]

I am consistently seeing a roughly -0.25% improvement over master, but I am struggling to get statistical significance from cargo bench. There is probably too much noise for such a small improvement to show up.

Also, while I agree that the function signatures are now more complicated, I generally prefer to make clear where mutability is possible. If the environment really won't be mutated, then I think it makes sense to separate its mutability from the type-checker’s.

[sharkdp]

On the other hand, the environment really belongs to the typechecker. I need to think about this again, but I think I lean towards declining this change.

[rben01]

I've thought of an alternative, which also keeps function signatures clean. Define a &mut version of TypeChecker with just the fields we need (which, crucially, doesn't include the Environment). Define proper_function_call, and any methods proper_function_call calls, on it instead of TypeChecker. Move get_proper_function_reference to be a method on Environment instead of TypeChecker. Then we can prove to the borrow checker that there is no overlapping mutable borrow. Would look basically like this:

struct TypeCheckerMut<'a> {
    registry: &'a DimensionRegistry,
    name_generator: &'a mut NameGenerator,
    constraints: &'a mut ConstraintSet,
}

impl<'a> TypeCheckerMut<'a> {
    fn new(
        registry: &'a DimensionRegistry,
        name_generator: &'a mut NameGenerator,
        constraints: &'a mut ConstraintSet,
    ) -> Self {
        Self {
            registry,
            name_generator,
            constraints,
        }
    }

    fn add_equal_constraint(&mut self, lhs: &Type, rhs: &Type) -> TrivialResolution {
        self.constraints
            .add(Constraint::Equal(lhs.clone(), rhs.clone()))
    }

    fn add_dtype_constraint(&mut self, type_: &Type) -> TrivialResolution {
        self.constraints.add(Constraint::IsDType(type_.clone()))
    }

    fn proper_function_call<'b>(
        &mut self,
        span: &Span,
        full_span: &Span,
        function_name: &'b str,
        signature: &FunctionSignature,
        arguments: Vec<typed_ast::Expression<'b>>,
        argument_types: Vec<Type>,
    ) -> Result<typed_ast::Expression<'b>> {
    // snip
    }
}

impl TypeChecker {
    // change just these two functions to reuse implementation
    fn add_equal_constraint(&mut self, lhs: &Type, rhs: &Type) -> TrivialResolution {
        TypeCheckerMut::new(
            &self.registry,
            &mut self.name_generator,
            &mut self.constraints,
        )
        .add_equal_constraint(lhs, rhs)
    }

    fn add_dtype_constraint(&mut self, type_: &Type) -> TrivialResolution {
        TypeCheckerMut::new(
            &self.registry,
            &mut self.name_generator,
            &mut self.constraints,
        )
        .add_dtype_constraint(type_)
    }

    // rest unchanged

and then in elaborate_expression:

if let Some((name, signature)) = self.env.get_proper_function_reference(callable) {
    // borrow checker sees that `&self.env` doesn't overlap with anything below, so it's kosher
    TypeCheckerMut::new(
        &self.registry,
        &mut self.name_generator,
        &mut self.constraints,
    )
    .proper_function_call(
        span,
        full_span,
        name,
        signature,
        arguments_checked,
        argument_types,
    )?
}

[rben01]

Or a third option where proper_function_call just becomes a static function taking only those fields of TypeChecker that it needs (and it doesn't need env, so all is well).

[sharkdp]

Thank you for looking into this.

Or a third option where proper_function_call just becomes a static function taking only those fields of TypeChecker that it needs (and it doesn't need env, so all is well).

I'm still not 100% convinced that we definitely need to solve this, but this sounds like a reasonable option to me. I think I would have to see the result of this, to be able to judge whether I would like that code change or not. But I don't feel great about suggesting this to you if we might end up declining it after all.

[rben01]

See #639. Much simpler implementation.