no-unused-expression warns on tagged templates

[EliSnow]

Bug Report

• TSLint version: 4.4.2
• TypeScript version: 2.2.1
• Running TSLint via: VSCode

TypeScript code being linted

function foo (strings: TemplateStringsArray, ...values: any[]) {}

foo`a template string`;

with tslint.json configuration:

{
  "extends": "tslint:recommended",
  "rules": {
    "no-unused-expression": true
  },
  "rulesDirectory": [
  ]
}

Actual behavior

TSLint gives a warning expected an assignment or function call (no-unused-expression)

Expected behavior

TSLint should recognize the tagged template as the equivalent of a function call.

[ajafff]

I'd like to get more feedback on this before making a change to the rule.
Why would you expect a tag function to have side effects?
IMO adding an exceptions for that to the rule is not the right way to go. That's what the enable / disable comments are for. On the other hand, there is already some special handling for constructor calls with side effects...

But then someone could argue that property access (using getters) or even array literals (messing with __defineSetter__) can have side effects

[EliSnow]

@ajaff, perhaps you are right. As you point out even property access can be made to have side effects. I would like to raise a few points and then leave it to the best judgement of the maintainers.

In my use case I'm using a tag function as a method on a class which does have a side effect. Based on my understanding of tag functions, I don't view them any different from a vanilla function other than their being some syntactical sugar around how they are invoked.

Is invoking a tag function really much different from:

function foo () {
  return "";
}
foo();

which the no-unused-expression rule has no problem with? I don't believe so. Unless and until the rule becomes smarter with some static analysis*, I think tag function invocations should be viewed no different.

Now what your argument really boils down to, I believe, is convention. Generally I think TS/JS developers would agree that property access should be viewed as an idempotent action, even though there may be some edge cases. Conversely, I don't think tag functions have such a convention, perhaps only because they are too new and not used widely enough for such a convention to have been formed.

* Doing some static analysis for no-unused-expression, I think, would be more intuitive. To start out it could be as simple as checking if the invoked function returns something and checking if the returned value is used.

[ajafff]

@EliSnow thank you for the detailed explanation of your point of view. I totally understand your reasoning. I don't have a strong opinion on this.

I'll wait for feedback from @adidahiya and/or @nchen63 if we want to handle this special case and if it should be configurable with a new config option and update PR #2269 accordingly

[nchen63]

It seems to me that the intention of tag functions is to simply convert strings. I don't think side effects should be encouraged.

[EliSnow]

@nchen63 There is a pattern where tag functions with side effects make a lot of sense. It is the case when you have a utility like a Java StringBuilder. Here's an example:

class StringBuilder {
  private strings: string[] = [];
  private values: any[] = [];

  public append (strings: TemplateStringsArray, ...values: any[]) {
    // append into strings and values
  }

  public finalize () {
    // return final string/object
  }
}

const sb = new StringBuilder();
sb.append`foo`;
sb.append`bar`;

I am doing something similar in my code to build an SQL AST and then return the finalized sql query with the parameters. There are probably other cases where you would have a tag method on a class to update the state of the object.

[nchen63]

That's an interesting usage for tags, but it doesn't look like the intended use case.

Out of curiosity, does this string builder have a measurable performance benefit over a simpler string builder? For example, calling it with the default template processor like sb.append(foo${var1}bar)? I'd be wary of premature micro-optimization with this code.

[EliSnow]

That was just a simplified example. What my code is doing is building an SQL AST and each substitution in the template becomes a parameter in the SQL query. Trying to use the default template processor means losing the SQL query parameter and exposing the system to SQL injection, which is obviously not an option. The purpose of using a tag function is not to be a "premature micro-optimization". It is a legitimate use case of a tag function with side effects.

I am curious by what you base the "intended use case" of tag functions. Is there some discussion by the ES committee where they state tag functions should have no side effect? The ES2015 spec makes no such claim that I can find. Neither does MDN's documentation. As to their purpose and possible uses MDN says:

Tags allow you to parse template literals with a function.

Tag functions don't need to return a string, as shown in the following example.

[nchen63]

Ok, that scenario is pretty compelling. I'm ok with treating tags as function calls by default. The MDN example does return a function instead of a string. I suppose side effects aren't out of the question.

As for the "intended use case", it's based on most of the examples I've seen. And you're right -- there is no documentation saying what the feature shouldn't be used for -- but TSLint is, by design, opinionated. I suppose that if we needed explicit documentation saying that there is a right and wrong way to do something, most of the rules wouldn't exist.

[EliSnow]

but TSLint is, by design, opinionated. I suppose that if we needed explicit documentation saying that there is a right and wrong way to do something, most of the rules wouldn't exist.

Good point. Though in my experience TSLint has always been flexible enough--either through disabling rules, or through a rule's config--that its opinionated nature has not been an annoyance. Until this issue, I've never had to choose between having rules I wanted and facing a slew of exceptions.

That being said, as you've noted @nchen63, most uses of tagged templates do not have side effects. Would it therefore be appropriate for no-unused-expression to have a config option of something like allow-tag-functions which could be opted into?

[nchen63]

I'm leaning toward making it default instead of behind a config option. In general, we try to have fewer things to configure. Also, not many people use tagged templates, so I don't think it would catch that many issues. There's probably far more people calling functions with no side effects without using the return value. Unfortunately (as far as I know) it seems prohibitively expensive to determine whether a function has side effects.