no-unused-expression rule doesn't handle directives

[ArturDorochowicz]

Bug Report

• TSLint version: 3.6.0
• TypeScript version: 1.8.9
• Running TSLint via: CLI , but doesn't actually matter

TypeScript code being linted

'ngInject';
console.log('abc');
'use strict';

with tslint.json

{
  "rules": {
    "no-unused-expression": true
  }
}

Actual behavior

1.ts[1, 1]: expected an assignment or function call

1. Directive 'ngInject'; triggers no-unused-expression violation
2. "no-op" expression 'use strict'; does not trigger no-unused-expression violation

Expected behavior

The no-unused-expression rule does not handle directive prologues at all. It only has a hard coded detection of 'use strict' expression in any position.

1. Directive prologue should be handled and any directive in directive prologue position should not trigger the violation (i.e. there should be no error on 'ngInject' in the example code).
2. 'use strict' must not be hardcoded and allowed in just any position (i.e. there should be an error on 'use strict' in the example code).

'ngInject' directive is just an example. Any directive should work in directive prologue position.

This "reopens" #423 which was valid but maybe not explained well enough.

For reference, the directive prologue definition from ECMAScript 5.1

14.1 Directive Prologues and the Use Strict Directive

A Directive Prologue is the longest sequence of ExpressionStatement productions occurring as the initial SourceElement productions of a Program or FunctionBody and where each ExpressionStatement in the sequence consists entirely of a StringLiteral token followed a semicolon. The semicolon may appear explicitly or may be inserted by automatic semicolon insertion. A Directive Prologue may be an empty sequence.

[adidahiya]

Thanks for the detailed report @ArturDorochowicz; we should definitely support this.

How about a new "allow-directives" option for the no-unused-expression rule that allows any expression which is a string literal only? Would that be too lenient?

[ArturDorochowicz]

I'm fine with lenient for starters and maybe refining it later. But why not incorporate it into the base rule, i.e. without additional option? Especially that the proposed option suggests something that doesn't exist in the language. After all, a statement consisting of a string literal expression outside of a directive prologue is not a directive.

NB ESLint seems to get this rule right [1]

[1] https://github.com/eslint/eslint/blob/5d15e382faa4af162393983bb12c5f95d6acc8a0/lib/rules/no-unused-expressions.js

[adidahiya]

Ah, ok, that makes sense, I wasn't paying close enough attention to the valid directive prologue positions. We should support it without the additional option. Thanks for the ESLint link.

[jkillian]

@ArturDorochowicz Filed #1094 to fix this. Feel free to give it a look over so we make sure it gets the right behavior

[ArturDorochowicz]

@jkillian LGTM. Thank you!

I initially thought that it will break on comments before or in between directives:

function a() {
  /* comment */
  // comment
  'directive1';
  // comment
  'directive2';
  /* comment */
  'directive3';
  var a = 1;
}

But no, things are ok. Probably the comments don't make it into the parsed syntax tree - that's something I did not know about.