Introduce basic typechecking

[nitnelave]

Checks that variable initialization have the same type as the declaration. Checks that function return have the same type as the declaration, or deduce function returns. Checks that binary operations are of the correct type, and deduce the type of the operation.

---

This change is 

[coveralls]

Coverage decreased (-0.7%) to 97.782% when pulling b609a094215dcfec2a288d7a6e6a977baf19d979 on typechecker into 3fd5d026358269acdc18243b65c99709a0c71ffc on master.

[coveralls]

Coverage decreased (-0.4%) to 98.141% when pulling cc5515a8df1142b08216c6cf69df3f661bdd436f on typechecker into 3fd5d026358269acdc18243b65c99709a0c71ffc on master.

[coveralls]

Coverage increased (+0.2%) to 98.699% when pulling a8c61074e74ce2fc30231bf5157d5198a7e98c8b on typechecker into d233c8f758da41926cb2b1a8b9bfd416291fe35b on master.

[coveralls]

Coverage increased (+0.2%) to 98.699% when pulling 48bc6a6a5ee7adcce2d557c6c561847179757191 on typechecker into d233c8f758da41926cb2b1a8b9bfd416291fe35b on master.

[bloody76]

Reviewed 5 of 5 files at r1. Review status: 2 of 18 files reviewed at latest revision, 2 unresolved discussions.

---

src/typechecker/typechecker.cc, line 25 at r1 (raw file):

void TypeChecker::visit(ast::IntConstant* node) {
  node->type() = Type(&ast::types::int64);

You should check for the type I think, you don't necessarily want to declare a int64

---

src/typechecker/typechecker.cc, line 38 at r1 (raw file):

  if (is_integer(left_type) && is_integer(right_type)) {
    using namespace ast::types;
    IntWidth result_width =

we should output a warning or so if it shrinks or expands the type.

---

Comments from Reviewable

[bloody76]

Reviewed 2 of 3 files at r2, 1 of 3 files at r10. Review status: 5 of 18 files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 1 files at r3, 2 of 2 files at r7. Review status: 8 of 18 files reviewed at latest revision, 8 unresolved discussions.

---

src/typechecker/typechecker.cc, line 24 at r12 (raw file):

bool is_boolean(const Type& t) {
  auto decl = t.get_declaration();
  return decl == &ast::types::boolean;

directly confronts t.get_declaration()

---

src/typechecker/typechecker.cc, line 64 at r12 (raw file):

void TypeChecker::visit(ast::VariableReference* node) {
  assert(node->is_resolved() && "Variable was not resolved");
  const auto& declaration = node->resolution().value_or_die()->type();

not sure declaration is the right name, type_reference would be better IMO

---

src/typechecker/typechecker.cc, line 87 at r12 (raw file):

  const auto& left_type = node->left_value().type().value_or_die();
  const auto& right_type = node->right_value().type().value_or_die();
  if (is_integer_operator(node->operation()) && is_integer(left_type) &&

Later we should do it another way, in order to resolve inheritances maybe.

In a way, we can see that int8 < int16 < int32 < int64, don't know if it would be better to add this into the inheritance system ..

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

  Type t = [&]() {
    if (node->value().is_ok()) {

Lambda is useless, you can directly init with void

if (node->value().is_ok() {
   ...
}```

Also, should be named `value_type` instead of `t`

---

src/typechecker/typechecker.cc, line 146 at r12 (raw file):

  // types.
  if (node->type().is_ok()) return;
  if (!function_return_type_.is_ok())

we should add a method to do:

node->type() = function_return_type.get_or_else(&ast::types::void_type);

---

src/typechecker/typechecker.h, line 26 at r12 (raw file):

 private:
  // We may have to turn that into a stack to support nested functions.

totally, can you turn it into a stack ? With my declaration as a statement PR it should be.

---

Comments from Reviewable

[bloody76]

Reviewed 2 of 2 files at r4, 1 of 1 files at r5, 1 of 1 files at r8, 2 of 2 files at r9, 2 of 3 files at r10, 1 of 1 files at r11, 1 of 1 files at r12. Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

src/ast/base_types.h, line 64 at r12 (raw file):

inline bool operator==(const Type& t1, const Type& t2) {
  if (t1.is_resolved() != t2.is_resolved()) return false;

shouldn't it check also that both are resolved ?

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 9 unresolved discussions.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

shouldn't it check also that both are resolved ?

well, if one is resolved and not the other, it returns false. Otherwise, if the first one is resolved, it means that the second one is resolved.

---

src/typechecker/typechecker.cc, line 25 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

You should check for the type I think, you don't necessarily want to declare a int64

I plan to give it the smallest (signed) int type that can fit the value, and then possibly upcast in the variable declaration/wherever it's used. But in the next PR.

---

src/typechecker/typechecker.cc, line 38 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

we should output a warning or so if it shrinks or expands the type.

I really don't think so. You output a warning if the user does a conversion that might discard data, not if you increase the size of the type (especially to one of the types of the operands)

---

src/typechecker/typechecker.cc, line 24 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

directly confronts `t.get_declaration()`

true. Will change.

---

src/typechecker/typechecker.cc, line 64 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

not sure declaration is the right name, type_reference would be better IMO

Maybe declaration_type? The idea is that declaration points to the AST node with the variable declaration referenced here.

---

src/typechecker/typechecker.cc, line 87 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

Later we should do it another way, in order to resolve inheritances maybe. In a way, we can see that int8 < int16 < int32 < int64, don't know if it would be better to add this into the inheritance system ..

No, no inheritance for int types. int8 is not a int16, but it is convertible to one. Later we'll have some conversion rules.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

Lambda is useless, you can directly init with void ```Type t = &ast::types::void_type; if (node->value().is_ok() { ... }``` Also, should be named `value_type` instead of `t`

The lambda allows for two return, I prefer that to an uninitialized variable (especially since Type has no default constructor). I would like to avoid declaring another function just for that, and the lambda will get inlined by the compiler, so it's not a performance problem.

Will change the variable name

---

src/typechecker/typechecker.cc, line 146 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

we should add a method to do: `node->type() = function_return_type.get_or_else(&ast::types::void_type);`

ah, yes, good point

---

src/typechecker/typechecker.h, line 26 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

totally, can you turn it into a stack ? With my declaration as a statement PR it should be.

In the next PR. Let's keep them small and contained.

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

well, if one is resolved and not the other, it returns false. Otherwise, if the first one is resolved, it means that the second one is resolved.

Hum I would say that it's wrong as false != false is false. Is there a special operators for the types ?

---

src/typechecker/typechecker.cc, line 38 at r1 (raw file):

Previously, nitnelave (nitnelave) wrote…

I really don't think so. You output a warning if the user does a conversion that might discard data, not if you increase the size of the type (especially to one of the types of the operands)

I agree but there is some cases where it could happend, than we don't handle yet: like casting from signed to unsigned with different sizes

---

src/typechecker/typechecker.cc, line 64 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

Maybe declaration_type? The idea is that declaration points to the AST node with the variable declaration referenced here.

declaration_Type is ok for me

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

The lambda allows for two `return`, I prefer that to an uninitialized variable (especially since Type has no default constructor). I would like to avoid declaring another function just for that, and the lambda will get inlined by the compiler, so it's not a performance problem. Will change the variable name

IT wouldn't be uninitialized as the default one here is void_type

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

Hum I would say that it's wrong as `false != false` is false. Is there a special operators for the types ?

The idea is that if they're either both resolved or both unresolved, we can compare them, otherwise they're different. If they're both resolved, we compare the declarations. If they're both unresolved, we compare the identifiers.

---

src/typechecker/typechecker.cc, line 38 at r1 (raw file):

Previously, bloody76 (Bd76) wrote…

I agree but there is some cases where it could happend, than we don't handle yet: like casting from signed to unsigned with different sizes

signed/unsigned is another matter.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

IT wouldn't be uninitialized as the default one here is void_type

hmm, true. But then you're not initializing it directly with the final value. What's wrong with having a lambda? It's a common enough pattern if you want returns

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

The idea is that if they're either both resolved or both unresolved, we can compare them, otherwise they're different. If they're both resolved, we compare the declarations. If they're both unresolved, we compare the identifiers.

I see, seems dangerous to me though, what about code like:

val a: SomeClass;
{
    using SomeClass = Int;
    val b : SomeClass;

    a = b;
}```

Do we allow overrides of types in different scopes ?

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

hmm, true. But then you're not initializing it directly with the final value. What's wrong with having a lambda? It's a common enough pattern if you want returns

Seems overkill to me, that's all, especially here. We could reduce the indentation level without the lambda, would be more readable, concise

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

I see, seems dangerous to me though, what about code like: ``` val a: SomeClass; { using SomeClass = Int; val b : SomeClass; a = b; }``` Do we allow overrides of types in different scopes ?

So, you would forbid comparison if they're not both resolved? That also makes sense.

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

Seems overkill to me, that's all, especially here. We could reduce the indentation level without the lambda, would be more readable, concise

What if I declared t const? To make it clear that we're not going to modify it, and to justify the single-assignment pattern. I've got Herb Sutter on my side: https://herbsutter.com/2013/04/05/complex-initialization-for-a-const-variable/

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

So, you would forbid comparison if they're not both resolved? That also makes sense.

Its just that it would be weird for a typechecker to let some types unresolved, for me it should be an error if not infer maybe

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

What if I declared `t` const? To make it clear that we're not going to modify it, and to justify the single-assignment pattern. I've got Herb Sutter on my side: https://herbsutter.com/2013/04/05/complex-initialization-for-a-const-variable/

With the const I have nothing to say, but a lambda for this is the reason why some thing like this would be nice:

val a = if (condition) default_value else { do_your_stuff(); value_to_take; }

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

With the const I have nothing to say, but a lambda for this is the reason why some thing like this would be nice: ``` val a = if (condition) default_value else { do_your_stuff(); value_to_take; } ```

I understand your point, and I agree that for this case it could be nice. However, I don't think in the general case it would be nice, and we also have the lambda solution.

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

Its just that it would be weird for a typechecker to let some types unresolved, for me it should be an error if not infer maybe

Yeah, but when you parse the types, they're not resolved yet. Of course once the typechecker is done all the types are resolved, but until then, the question remains whether we want types to be comparable? I guess not. return false or assert?

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

I understand your point, and I agree that for this case it could be nice. However, I don't think in the general case it would be nice, and we also have the lambda solution.

yes but I feel like the lambda solution is kind of hacky, maybe we can tackle this issue later if we wish too

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

Yeah, but when you parse the types, they're not resolved yet. Of course once the typechecker is done all the types are resolved, but until then, the question remains whether we want types to be comparable? I guess not. `return false` or `assert`?

Good point, maybe we just don't want them to be comparable until the typechecker pass. If it's that way we should assert it. Because anyway it should fail at the end of typechecking if something is unresolved (I guess ?), maybe later if we want to handle all the passes and throw all the errors together we should return false, but for now I think the assert is better as it should help us develop ?

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

yes but I feel like the lambda solution is kind of hacky, maybe we can tackle this issue later if we wish too

Compared to functional languages, yes, it is less elegant. But then, I don't want to have a language in which everything is a value simply to support this case. I just really don't like that test1 and test2 return different things here (with the rust syntax):

fun test() {
  1 == 2;
}
fun test2() {
  1 == 2
}

And I am against both of them returning bool, because then it's hard to express a function returning void

---

Comments from Reviewable

[bloody76]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

Compared to functional languages, yes, it is less elegant. But then, I don't want to have a language in which everything is a value simply to support this case. I just *really* don't like that test1 and test2 return different things here (with the rust syntax): ``` fun test() { 1 == 2; } fun test2() { 1 == 2 } ``` And I am against both of them returning bool, because then it's hard to express a function returning void

Indeed its misleading. what about some new keyword ? something that would say "return from scope" ? You would have to specify that your block is returning a value

---

Comments from Reviewable

[nitnelave]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

Indeed its misleading. what about some new keyword ? something that would say "return from scope" ? You would have to specify that your block is returning a value

I'm open to suggestions about a new keyword, I can't think of any

---

Comments from Reviewable

[nitnelave]

Review status: 2 of 18 files reviewed at latest revision, 2 unresolved discussions.

---

src/ast/base_types.h, line 64 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

Good point, maybe we just don't want them to be comparable until the typechecker pass. If it's that way we should assert it. Because anyway it should fail at the end of typechecking if something is unresolved (I guess ?), maybe later if we want to handle all the passes and throw all the errors together we should `return false`, but for now I think the assert is better as it should help us develop ?

Done.

---

src/typechecker/typechecker.cc, line 64 at r12 (raw file):

Previously, bloody76 (Bd76) wrote…

declaration_Type is ok for me

Done.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

I'm open to suggestions about a new keyword, I can't think of any

Renamed the variable

---

Comments from Reviewable

[bloody76]

Review status: 2 of 18 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

src/typechecker/typechecker.cc, line 113 at r12 (raw file):

Previously, nitnelave (nitnelave) wrote…

Renamed the variable

I propose to think about the keyword thing later, lets have the basic features first :D

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 1 files at r17. Review status: 3 of 18 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[bloody76]

---

Review status: 3 of 18 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.06%) to 98.722% when pulling 94dad9d1cfcfd0fef99998bfdbba0bbc3c2c6457 on typechecker into 4f7f67eb1c6acbdd885dfa625690afdb69071bdd on master.

[coveralls]

Coverage increased (+0.06%) to 98.722% when pulling 6135e27811a6b32e7b231c659ac94ebefff72b8d on typechecker into 4f7f67eb1c6acbdd885dfa625690afdb69071bdd on master.

[bloody76]

---

Reviewed 1 of 16 files at r13, 2 of 3 files at r19, 1 of 1 files at r20, 1 of 2 files at r21, 1 of 1 files at r22, 1 of 2 files at r24, 1 of 3 files at r26, 1 of 1 files at r29. Review status: 10 of 17 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable