Add return to void functions

[nitnelave]

This PR adds a return statement at the end of functions returning Void, making the implicit return explicit.

---

This change is 

[coveralls]

Coverage increased (+0.02%) to 99.016% when pulling 4bf873b7afbc573305ac46fe54851893b70b0ef2 on add_return into 3969955fb3b6b9b182bac7c917a90cf669fddc83 on master.

[coveralls]

Coverage increased (+0.2%) to 99.182% when pulling 0c5310559a375eaa174a79937727684c9ddb8455 on add_return into 3969955fb3b6b9b182bac7c917a90cf669fddc83 on master.

[bloody76]

Reviewed 1 of 1 files at r1, 20 of 20 files at r2, 2 of 6 files at r3, 10 of 10 files at r4. Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

src/transform/add_return.cc, line 14 at r4 (raw file):

void VoidFunctionReturnAdder::visit(ast::IfStatement* node) {
  if (node->else_statement().is_ok()) {
    ASTVisitor::visit(node->body().get());

why not node->body()->accept(*this); ?

---

src/transform/add_return.cc, line 16 at r4 (raw file):

    ASTVisitor::visit(node->body().get());
    bool if_returned = has_returned_;
    has_returned_ = false;

no need for this as we are going down the children

---

src/transform/add_return.cc, line 17 at r4 (raw file):

    bool if_returned = has_returned_;
    has_returned_ = false;
    ASTVisitor::visit(node->else_statement().value_or_die().get());

same why not ->accept(*this); ?

---

src/transform/add_return.cc, line 32 at r4 (raw file):

    const auto& statement = *stmt_it;
    if (has_returned_) {
      error_list_.add_error(statement->location(), "Unreachable code");

should be add_warning I think, valid code, maybe not the user intention though

---

src/transform/add_return.cc, line 42 at r4 (raw file):

void VoidFunctionReturnAdder::visit(ast::FunctionDeclaration* node) {
  has_returned_ = false;
  ASTVisitor::visit(node);

I don't get it, why visiting the same node ?

---

src/transform/add_return.cc, line 44 at r4 (raw file):

  ASTVisitor::visit(node);

  using StatementsBody = ast::FunctionDeclaration::StatementsBody;

put it closer to where its used in the first place

---

src/transform/add_return.cc, line 54 at r4 (raw file):

  if (has_returned_) {
    CHECK(!statements->statements().empty()) << "Empty function that returned: " << node->name();

shouldn't be possible I think, is it an assert to make sure there isn't some wrong behavior ?

---

src/transform/add_return.h, line 22 at r4 (raw file):

 private:
  bool has_returned_ = false;
  ast::ErrorList<> error_list_;

I wonder if we couldn't make this part of the ASTVisitor directly ... instead of defining it again and again

---

src/visitor/error_visitor.h, line 12 at r4 (raw file):

class VisitorError : public GenericError {
 public:
  VisitorError(const lexer::Range& range, const std::string& message)

where di you need to put it non const ?

---

Comments from Reviewable

[nitnelave]

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

---

src/transform/add_return.cc, line 14 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

why not node->body()->accept(*this); ?

This would imply another virtual call, whereas here we already know the type of the node, so we can call directly visit.

---

src/transform/add_return.cc, line 16 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

no need for this as we are going down the children

That would imply that we guarantee that children set it to false when they don't return, which is not true.

---

src/transform/add_return.cc, line 17 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

same why not `->accept(*this); ` ?

Same answer, we know the type (otherwise it wouldn't compile).

---

src/transform/add_return.cc, line 32 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

should be add_warning I think, valid code, maybe not the user intention though

I kinda want it to be an error, since we remove the statements after. I think being a little harsh on the user is not too bad.

---

src/transform/add_return.cc, line 42 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

I don't get it, why visiting the same node ?

calling the parent function that will recurse into the children, rather than writing the recursion ourselves everywhere.

---

src/transform/add_return.cc, line 54 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

shouldn't be possible I think, is it an assert to make sure there isn't some wrong behavior ?

yeah, a CHECK is a nice-looking assert, removed in release mode.

---

src/transform/add_return.h, line 22 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

I wonder if we couldn't make this part of the ASTVisitor directly ... instead of defining it again and again

Yeah, could be nice. Or another base class inheriting from ASTVisitor, maybe?

---

src/visitor/error_visitor.h, line 12 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

where di you need to put it non const ?

it's not about const vs non-const, it's about reference vs value. When we remove the statements, if we have an error pointing to their location, the reference is invalid, we need to copy it.

---

Comments from Reviewable

[bloody76]

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

---

src/transform/add_return.cc, line 14 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

This would imply another virtual call, whereas here we already know the type of the node, so we can call directly `visit`.

I think its better to do a virtual call instead of calling unique_ptr::get

---

src/transform/add_return.cc, line 16 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

That would imply that we guarantee that children set it to false when they don't return, which is not true.

han yes indeed.

---

src/transform/add_return.cc, line 32 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

I kinda want it to be an error, since we remove the statements after. I think being a little harsh on the user is not too bad.

I am not a big fan honestly, putting it a warning and make the warnings errors by default would be better in my opinion. But we shouldn't force the user with habits

---

src/transform/add_return.cc, line 42 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

calling the parent function that will recurse into the children, rather than writing the recursion ourselves everywhere.

but we only care about the body of the function right ? Why would you check for returns in non-statement things ?

---

src/transform/add_return.h, line 22 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

Yeah, could be nice. Or another base class inheriting from ASTVisitor, maybe?

what would be the point of another base class ?

---

Comments from Reviewable

[nitnelave]

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

---

src/transform/add_return.cc, line 14 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

I think its better to do a virtual call instead of calling unique_ptr::get

Why? There's nothing dangerous about unique_ptr::get.

---

src/transform/add_return.cc, line 32 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

I am not a big fan honestly, putting it a warning and make the warnings errors by default would be better in my opinion. But we shouldn't force the user with habits

If you have a look at Go, if you declare a variable and don't use it, it's a compile error. If you call a function and don't use the return value, it's a compile error. And from some time working at HeartFlow, I suspect that some companies have thousands of warnings in their codebase, and they wouldn't see one more or one less. I'm more in favor of errors when it doesn't make sense and is easily fixed.

---

src/transform/add_return.cc, line 42 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

but we only care about the body of the function right ? Why would you check for returns in non-statement things ?

Fair enough. Will change.

---

src/transform/add_return.h, line 22 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

what would be the point of another base class ?

to add the error_list, because not all visitors produce errors.

---

Comments from Reviewable

[nitnelave]

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

---

src/transform/add_return.cc, line 42 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

Fair enough. Will change.

Done.

---

src/transform/add_return.cc, line 44 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

put it closer to where its used in the first place

Done.

---

src/transform/add_return.h, line 22 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

to add the `error_list`, because not all visitors produce errors.

Done.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.3%) to 99.26% when pulling e96ec31e9cb60890428a5781c55c4f4368970332 on add_return into c02ce343d48aa3ee5bc419cd33b14e5f572f0cde on master.

[bloody76]

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

---

src/transform/add_return.cc, line 14 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

Why? There's nothing dangerous about `unique_ptr::get`.

well its dangerous because you can copy this ptr then and you bypass the smart ptr concept

---

src/transform/add_return.cc, line 32 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

If you have a look at Go, if you declare a variable and don't use it, it's a compile error. If you call a function and don't use the return value, it's a compile error. And from some time working at HeartFlow, I suspect that some companies have *thousands* of warnings in their codebase, and they wouldn't see one more or one less. I'm more in favor of errors when it doesn't make sense and is easily fixed.

I get the point, it's just that we should be able to ignore those errors at least

---

Comments from Reviewable

[bloody76]

Reviewed 2 of 4 files at r5, 2 of 9 files at r6. Review status: 30 of 37 files reviewed at latest revision, 3 unresolved discussions.

---

src/transform/add_return.cc, line 18 at r6 (raw file):

    has_returned_ = false;
    visit(node->else_statement().value_or_die().get());
    has_returned_ = has_returned_ && if_returned;

&&= but not really important ..

---

Comments from Reviewable

[bloody76]

Reviewed 1 of 9 files at r6. Review status: 31 of 37 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[bloody76]

Reviewed 7 of 9 files at r6. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[nitnelave]

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

---

src/transform/add_return.cc, line 14 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

well its dangerous because you can copy this ptr then and you bypass the smart ptr concept

Copying the pointer itself is not dangerous, it's an observer pointer. What's dangerous is:

• deleting the pointer. There is no delete in the whole project, and I intend to keep it that way
• accessing it after it was destroyed. This is always possible (except in Rust), but in this context it's not.

Consider it the same as passing a reference.

---

src/transform/add_return.cc, line 32 at r4 (raw file):

Previously, bloody76 (Bd76) wrote…

I get the point, it's just that we should be able to ignore those errors at least

For now, let's keep it at error. The language is a bit opinionated, on purpose. And in the future, we can always downgrade it to a warning (whereas going the other way is a breaking change).

---

src/transform/add_return.cc, line 18 at r6 (raw file):

Previously, bloody76 (Bd76) wrote…

&&= but not really important ..

That's not an operator. &&= doesn't exist. Anyway, in the last version, you'll see that I changed it to = .. && ...; because clang-tidy was complaining about an implicit cast to int with the &= expression.

---

Comments from Reviewable

[bloody76]

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

---

src/transform/add_return.cc, line 14 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

Copying the pointer itself is not dangerous, it's an observer pointer. What's dangerous is: - deleting the pointer. There is no `delete` in the whole project, and I intend to keep it that way - accessing it after it was destroyed. This is always possible (except in Rust), but in this context it's not. Consider it the same as passing a reference.

I understand that it's safe in the current context, but still a bad design to me. Provided the get method is bad IMO in the unique_ptr interface anyway ...

---

Comments from Reviewable

[bloody76]

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

---

src/transform/add_return.cc, line 32 at r4 (raw file):

Previously, nitnelave (nitnelave) wrote…

For now, let's keep it at error. The language is a bit opinionated, on purpose. And in the future, we can always downgrade it to a warning (whereas going the other way is a breaking change).

Ok let's do this then !

---

Comments from Reviewable

[bloody76]

---

Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable