Incorrect detection of missing returns in frog.

[DartBot]

This issue was originally filed by dnljms...@gmail.com

---

I noticed a couple of small errors in the recent type checker change (http://code.google.com/p/dart/source/detail?r=2340).

For this code:

    int doWhileLoop() { do { return 10; } while(false); }

It claims that there might be a missing return, but the loop body of a do..while loop always runs, so there isn't one. 'visitDoWhile' could just return the bodyType. It could possibly warn about unreachable code since the 'while' clause will never be reached.

For this:

    int maybeFirst(x,y) {
      if (x) {
        if (y) {
          return 10;
        }
      }
      else {
        return 20;
      }
    }

I think it should give the warning MAYBE_MISSING_RETURN, but 'join' doesn't account for the case 'this === MAYBE_RETURNING' which should always return MAYBE_RETURNING. It seems that join could just be:

    StatementType join(StatementType other) =>
      this === other ? this : MAYBE_RETURNING;

[DartBot]

This comment was originally written by drfibonacci@google.com

---

Added Area-Frog, Triaged labels.

[DartBot]

This comment was originally written by jimhug@google.com

---

Set owner to jimhug@google.com.
Added Started label.

[DartBot]

This comment was originally written by jimhug@google.com

---

Thanks for what turned out to be a very interesting bug!

I'm marking this as "Invalid" because it is not a bug in the frog compiler (and the one part that was a bug has been fixed - but not in the way you expected).

Neither of these examples will produce an error. According to the dart language spec, "If the last statement of a function is not a return statement, the statement return null; is implicitly appended to the function body." In addition, in Dart, all types are nullable so it is legal to return null from a method typed to return int. As a result, neither of these will generate any errors or warnings. The issue that you were seeing before warning about a missing return was fixed by implementing this rule and assuming a null return.

Please, feel free to open a language issue if this part of the specification still feels like a bug to you.

Thanks - Jim

---

Added Invalid label.

[DartBot]

This comment was originally written by dnl...@gmail.com

---

Sorry, I should have explained that the change that I linked to doesn't check for compliance with the specification, but looks for potential problems such as unreachable code. The unit tests it introduced start with the comment, '/** Tests analysis of returns (not required by the specification). */'. So this report isn't about matching the specification, so a specification issue would be incorrect.

I've attached a patch which adds two new tests, one passes and the other fails with an unreachable code warning. Even if my interpretation is wrong, I'm pretty sure that they indicate a genuine problem because the tests are for equivalent code but get different results.

---

Attachment:
typechecker_test.patch (888 Bytes)