Add visitor for variable destructions

[nitnelave]

This PR starts by expanding the visitor that add returns to also add "unreachable" nodes, where the control flow cannot get.

Then the typechecker's handling of ints is improved.

And finally, a visitor adding "destruction nodes" for variables leaving the scope or before a return is added.

---

This change is 

[coveralls]

Coverage decreased (-0.2%) to 99.073% when pulling 8a1280fe293d2b384285d18d1e02b347534eadae on variable_destroy into 1dad241d4f7395507bece2fd9f82f71d5bc3279d on master.

[bloody76]

Reviewed 8 of 8 files at r1, 10 of 10 files at r2, 20 of 20 files at r3. Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

src/ast/variable_destruction.h, line 15 at r3 (raw file):

  explicit VariableDestruction(VariableDeclaration* declaration)
      : Statement(lexer::invalid_range(), NodeType::VARIABLE_DESTRUCTION),
        declaration_(declaration) {}

assert that declaration is not nullptr

---

src/transform/add_destroy_variable.cc, line 18 at r3 (raw file):

    InsertIterator iter,
    const VariableDestructorAdder::ScopeVariables& variables) {
  for (auto it = variables.rbegin(); it != variables.rend(); ++it) {

not sure if really necessary but const auto&

---

src/transform/add_destroy_variable.cc, line 19 at r3 (raw file):

    const VariableDestructorAdder::ScopeVariables& variables) {
  for (auto it = variables.rbegin(); it != variables.rend(); ++it) {
    log(ERROR) << "Destroying " << (*it)->id().to_string();

why error ? should be debug no ?

---

src/transform/add_destroy_variable.cc, line 29 at r3 (raw file):

    InsertIterator iter,
    const VariableDestructorAdder::ScopeStack& scope_stack) {
  for (auto it = scope_stack.rbegin(); it != scope_stack.rend(); ++it)

same, could be const auto& maybe

---

src/transform/add_destroy_variable.cc, line 38 at r3 (raw file):

void VariableDestructorAdder::visit(ast::FunctionDeclaration* node) {
  std::vector<ast::VariableDeclaration*> arguments;
  for (const auto& arg : node->arguments()) {

IMO arguments should be handled here.

---

src/transform/add_destroy_variable.cc, line 52 at r3 (raw file):

  using ast::NodeType;
  enter_scope();
  std::move(function_arguments.begin(), function_arguments.end(),

why not putting it during FunctionDeclaration or vising the FunctionDeclarationArguments ?

---

src/transform/add_destroy_variable.h, line 11 at r3 (raw file):

namespace transform {

/// Add a return statement at the end of functions returning `Void`.

wrong comment ?

---

src/transform/add_destroy_variable.h, line 15 at r3 (raw file):

 public:
  using ScopeVariables = std::vector<ast::VariableDeclaration*>;
  using ScopeStack = std::vector<ScopeVariables>;

use stack ?

---

src/transform/add_destroy_variable.h, line 34 at r3 (raw file):

  // Process a block, with potentially function arguments if it's the
  // function's main body.
  void visit_block(

no need for it, we only need to visit the arguments or add them in FunctionDeclaration

---

src/typechecker/typechecker.cc, line 119 at r2 (raw file):

}

bool is_compatible(const ast::Type& declaration_type, ast::Value* value) {

const ast::Value*

---

test/resources/transformer/destroy_variable/arguments.ref, line 11 at r3 (raw file):

    //destroy b//
    //destroy a//
    //destroy arg2//

euh wrong ? It should be destroyed at the end of the function

---

test/resources/transformer/destroy_variable/conditional_return.ref, line 6 at r3 (raw file):

    val b : Int32 = 2;
    //destroy b//
    //destroy a//

same why a is destroyed here ?

---

test/resources/transformer/destroy_variable/destroy_scope_return.ref, line 14 at r3 (raw file):

    //destroy c//
    //destroy b//
    //destroy a//

b and a should be destroyed after

---

Comments from Reviewable

[bloody76]

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

---

src/transform/add_destroy_variable.cc, line 20 at r3 (raw file):

  for (auto it = variables.rbegin(); it != variables.rend(); ++it) {
    log(ERROR) << "Destroying " << (*it)->id().to_string();
    iter = std::make_unique<ast::VariableDestruction>(*it);

should you break ? iter is overwritten each time here

---

src/typechecker/typechecker.cc, line 193 at r3 (raw file):

  ASTVisitor::visit(node);
  if (node->value().is_ok()) {
    CHECK(node->value().value_or_die()->type().is_ok());

store node->value().value_or_die() in variable

---

Comments from Reviewable

[nitnelave]

Review status: 13 of 35 files reviewed at latest revision, 15 unresolved discussions.

---

src/ast/variable_destruction.h, line 15 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

assert that declaration is not nullptr

There are no nullptr in the project, by design. The only nullable pointers are Option

---

src/transform/add_destroy_variable.cc, line 18 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

not sure if really necessary but `const auto&`

it's an iterator, it's not necessary

---

src/transform/add_destroy_variable.cc, line 19 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

why error ? should be debug no ?

debugging leftovers

---

src/transform/add_destroy_variable.cc, line 20 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

should you break ? iter is overwritten each time here

it's operator= is overloaded to insert in the container

---

src/transform/add_destroy_variable.cc, line 29 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

same, could be const auto& maybe

same, iterator

---

src/transform/add_destroy_variable.cc, line 38 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

IMO arguments should be handled here.

The idea is that we treat arguments to the function like variable declarations at the beginning of the first scope. So we "declare" them in the first block, and then treat it like a normal block.

---

src/transform/add_destroy_variable.cc, line 52 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

why not putting it during FunctionDeclaration or vising the FunctionDeclarationArguments ?

because we want to put it after the "enter_scope"

---

src/transform/add_destroy_variable.h, line 11 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

wrong comment ?

yep

---

src/transform/add_destroy_variable.h, line 15 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

use stack ?

No, we need to be able to access all the elements in case of a return.

---

src/transform/add_destroy_variable.h, line 34 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

no need for it, we only need to visit the arguments or add them in FunctionDeclaration

It's needed because we call the version with a default argument from the BlockStatement, but the function with two arguments from FunctionDeclaration.

---

src/typechecker/typechecker.cc, line 119 at r2 (raw file):

Previously, bloody76 (Bd76) wrote…

const ast::Value*

can't be const (so far) because type() is not a const method. Changing it now.

---

src/typechecker/typechecker.cc, line 193 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

store `node->value().value_or_die() ` in variable

sure

---

test/resources/transformer/destroy_variable/arguments.ref, line 11 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

euh wrong ? It should be destroyed at the end of the function

This is a return, so it is the end of the function

---

test/resources/transformer/destroy_variable/conditional_return.ref, line 6 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

same why a is destroyed here ?

because return

---

test/resources/transformer/destroy_variable/destroy_scope_return.ref, line 14 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

b and a should be destroyed after

return?

---

Comments from Reviewable

[coveralls]

Coverage decreased (-0.2%) to 99.071% when pulling 49fe62193ee3203473bd20ff495d65457427d04a on variable_destroy into 1dad241d4f7395507bece2fd9f82f71d5bc3279d on master.

[bloody76]

Reviewed 6 of 22 files at r4, 15 of 20 files at r5. Review status: 29 of 35 files reviewed at latest revision, 3 unresolved discussions.

---

src/ast/variable_destruction.h, line 15 at r3 (raw file):

Previously, nitnelave (nitnelave) wrote…

There are no nullptr in the project, by design. The only nullable pointers are Option

maybe but it's defensive programming. Asserting wouldn't do any bad in release mode and that way we can prevent some errors in the future

---

src/transform/add_destroy_variable.cc, line 52 at r3 (raw file):

Previously, nitnelave (nitnelave) wrote…

because we want to put it after the "enter_scope"

you can still do it after visiting the body of the function, you mean to destroy the args once the first block of the function is finished ?

---

test/resources/transformer/destroy_variable/arguments.ref, line 11 at r3 (raw file):

Previously, nitnelave (nitnelave) wrote…

This is a return, so it is the end of the function

oh yes indeed

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 22 files at r4. Review status: 30 of 35 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[bloody76]

Reviewed 5 of 20 files at r5. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[nitnelave]

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

---

src/ast/variable_destruction.h, line 15 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

maybe but it's defensive programming. Asserting wouldn't do any bad in release mode and that way we can prevent some errors in the future

I don't want to start adding null checks everywhere in the code. Instead, I'd like to control the edges, the interfaces, and the keyword nullptr. Right now, the only place it exists in the project is in Option, to handle Option<T*>, and that's the way I'd like it to stay.

---

src/transform/add_destroy_variable.cc, line 52 at r3 (raw file):

Previously, bloody76 (Bd76) wrote…

you can still do it after visiting the body of the function, you mean to destroy the args once the first block of the function is finished ?

the args must be destroyed before the return. For me, they are like variables declared at the beginning of the opening block of the function.

---

Comments from Reviewable