hhvm / hhast

Mutable AST library for Hack with linting and code migrations
MIT License
69 stars 42 forks source link

Improve NoEmptyStatementLinter to detect more types of empty statements #142

Closed ryangreenberg closed 5 years ago

ryangreenberg commented 6 years ago

As discussed in the hacklang Slack, it would be nice for the NoEmptyStatementLinter to flag expressions that are semantically empty (expressions that have no effect on the code).

Here are some examples of things we can flag:

function more_empty_exprs(): void {
  // Obviously empty
  $a;
  $a.$b;
  $a + $b;
  "a"."b";
  $a.$b.$c;

  // Less clear
  // The concatenation is useless, but maybe fn() has side effects
  // Probably worth flagging as a problem; not sure if we should
  // avoid an autofix
  $a . fn();

  // Not empty—should not be considered empty statements
  side_effect();
  $counter++;
  $i = 5;
  $i += 1;
  $i--;
}

My initial thinking is to flag any ExpressionStatement where the expression node is one of the following:

How does this approach sound? Are there any other edge cases to consider?

Edit: This is, of course, more complicated than I initially thought. I went through all the possible values for ExpressionStatement::getExpression and I think there is a long list of unambiguous cases to flag, plus BinaryExpression, where we should flag most expressions without an assignment operator, and a couple other cases. I'll try running some sample code on a large codebase and report back.

ryangreenberg commented 5 years ago

This was implemented in #145