nitlang / nit

Nit language
http://nitlanguage.org
Apache License 2.0
238 stars 64 forks source link

Removed `ModelBuilder` refinement from `naive_interpreter.nit` #2802

Closed Louis-Vincent closed 4 years ago

Louis-Vincent commented 4 years ago

Dangerous refinement

Since refinement is really powerful, we should be particularly careful when adding new dependancies into a class. Here's the problem

interface IA
end

# module 1
class A1
    super IA
    ...
end

# module2
class A2
    super IA
    ...
end

redef class ConsumerOfIA
    # new dependency
    var a: IA
end

The challenge here is: how do we instantiate ConsumerOfIA:a. Currently, most users will resolve this issue like this :

import module2
redef class ConsumerOfIA
    var a: IA
    init
    do
        a = new A2(...)
    end
end

This solution is bad since we create a strong dependency between the consumer and the service provider. Thus, if we need to change the implementation of the service we would need to change the code since we don't really code against an abstract (I in SOLID). Moreover, we make our consumer dependent on all module2 and A2 dependencies. This is classic bad coupling between classes, but the problem is worse with refinement, here's why:

We divide the construction of a class between all modules. Instead of having a single composition root (where we create our object graph), the object creation process is shared between module, thus making it very hard to understand what goes where and in which order (smells like temporal dependency).

The interpreter

Currently, we can't add a new abstract dependency in the interpreter by simply refining the interpreter. This is due to the strong coupling between ModelBuilder and NaiveInterpreter. If we want a new dependency in NaiveInterpreter without manually creating it, we must redefine ModeBuilder to instantiate dependency, but it is equivalent to the original problem since we have to redefine the ModelBuilder in the same module.

My suggestion

We should not code the instantiation logic inside the naive_interpreter.nit module itself. We should have only one composition root : nit.nit which resolves the dependency graph.

If we add a new dependency, we change the nit.nit file. We can't get out of it without at least changing one file since when we do a refinement we need to change interpreter.nit to publicly import the new features. However, this change would make dependency management less sparsed are more controlled.

Signed-off-by: Louis-Vincent Boudreault lv.boudreault95@gmail.com