sdk

sdk copied to clipboard

The dead code analysis in the analyzer produces diagnostics with the wrong highlight range in at least two cases. Create a file in a package that will be analyzed with null safety enabled that has the following content (after removing the [[ and ]] markers:

void f() {
  for (int i = 0; i < 2; [[i++) {
    print(i);
    return;
  }]]
}

void g(bool c) {
  do {
    print(c);
    return;
  } while ([[c);]]
}

The markers indicate where the highlight ranges are. The offsets are correct, but the lengths are wrong. The first length should be 3 (rather than 36) and the second length should be 1 (rather than 3).

For the second case (with additional statements after the while loop):

void g(bool c) {
  do {
    print(c);
    return;
  } while (c);
  print('2');
}

I think it is not only the condition in while should be dead code, but also the following statements (print('2')).

Also, how about marking the do { and } while(); as well, since they can be safely deleted, and their condition (c) is not the only dead code which isn't reachable.

@stereotype441

We are no longer reporting dead code in the first case, though I believe that the updater can't be reached. Is that expected?

Is the behavior in the second case (that only the condition is marked as being dead) expected, or should we be marking more, as per the second comment?

We are no longer reporting dead code in the first case, though I believe that the updater can't be reached. Is that expected?

Flow analysis is aware that the updater is unreachable, but the analyzer isn't querying reachability during the course of analyzing the updater. Looking at the code for ResolverVisitor, it looks like a few visit methods simply don't call checkUnreachableNode. It might be as easy as just sprinking in a few more unreachability checks.

Is the behavior in the second case (that only the condition is marked as being dead) expected, or should we be marking more, as per the second comment?

The behavior in the second case is that the additional statements after the while loop are marked as unreachable, and this is correct from the standpoint that yes, those statements are indeed unreachable. However, it feels a little weird to me that we highlight c); print('2'); as a single range of unreachable code. I think we're trying to do some clever heuristics in NullSafetyDeadCodeVerifier to try to coalesce together the results of several individual calls to isReachable into nice human-comprehensible code ranges. Those same heuristics are probably why in the original issue report, we highlighted c); as unreachable rather than just c.

Maybe we're being too clever for our own good? It seems like we could replace the clever logic in NullSafetyDeadCodeVerifier with a few simple heuristics based on the immediate parent of the dead node, e.g.:

• If control flow is unreachable at the beginning of any statement, highlight the statement as unreachable. If the statement's parent is a block, or a switch member, extend the unreachable range to include the rest of the statements in the parent as well.
• If control flow is unreachable at the beginning of any expression, highlight the expression as unreachable.
  • If the expression's parent is a list literal, set literal, or argument list, extend the unreachable range to include the rest of the expressions in the parent as well.
  • If the expression's parent is a binary operator (and the expression is the RHS of the binary operator), extend the unreachable range to include the operator as well (this way, if the parent is true || x, we mark || x as unreachable, which is nice because the user could delete the || x without effect)

And of course:

• If control flow is unreachable at the beginning of any expression or statement, we suppress reporting of dead code for sub-expressions and sup-statements, so that the user doesn't see overlapping dead code hints.

https://dart-review.googlesource.com/c/sdk/+/260109

changes the range of dead code of the RHS of binary operator

https://dart-review.googlesource.com/c/sdk/+/260110

handles do statement as two separate regions (one for do, another for while), and correcting the quick fix.

~~1. For removing the following statements after the do while, please see sdk/263900~~ 2. There is a currently a failing test, which points to this issue of the scenario:

void f(bool c) {
  do {
    print(c);
    return;
  } while (c);
}

It is expecting that the return is removed. It is more like a lint I guess, however the following case is not triggering any hint or lint (unnecessary_statements for example)

void f() {
  return;
}

So, I guess we either follow the test expectation, and lint about the unnecessary return (in various situations), or the test expectation should be changed to accept the existence of a redundant empty return.

https://dart-review.googlesource.com/c/sdk/+/263940 for the first part in OP.

https://dart-review.googlesource.com/c/sdk/+/266388

which is similar to https://github.com/dart-lang/sdk/commit/931961eac8d38df12f14a6d8b945c4c4840d2979.

MethodInvocation was missed, because I incorrectly locally tested it as a Statement, not as an Expression.

https://dart-review.googlesource.com/c/sdk/+/266389 which handles updatersofforParts`.

This CL fixes this issue, as it removes the previously failing tests referencing it.

Please note that there is a new failing test, because the coveredNode in for (; false; 1, 2) {} should be a NodeList, but is evaluated to the parent ForParts, so we lose track what was actually dead code.

https://dart-review.googlesource.com/c/sdk/+/271200

Fixes an additional case, as was hinted in the review in https://dart-review.googlesource.com/c/sdk/+/266389/comment/7c2f0988_3783009b/