Be Careful with Array Mutators

A while ago, I wrote a linting rule to highlight an error that I’d made on a few occasions. Let’s look at the error — and its variations — and then at the rule that prevents it.

The error

If you are used to working with fluent APIs — perhaps with D3 or lodash — it feels natural to write Array-based code like this:

const developers = employees
  .filter((e) => e.role === "developer")
  .map((e) => e.name)
  .sort();

Which is fine. There’s no error in that code.

However, in situations in which you don’t need to filter or map, you might write something like this:

const sorted = names.sort();

It feels natural, but there’s a potential error in there. And it’s an error that can effect hard-to-find bugs. It’s also an error that’s easy to overlook in code reviews.

The sort method mutates the array. It sorts the array in place and returns a reference to the array itself. It does not return a sorted copy of the array. So the sorted and names variables both reference the same, sorted array. And it means that — unless the array was already sorted — any code using the names variable will be dealing with an array in which the elements that been rearranged.

So why is the code in the first snippet okay? It’s because sort is applied to the array returned by map — a transient array to which there are no references.

The rule

The fundamental problem is the use of the return value when the sort method is applied to an array that is referenced elsewhere in the program.

If the return value is not used, it’s fine — as the intent is clear:

names.sort();

And it’s also fine if the sort method is applied to an array that has no referencing variable:

const names = ["bob", "alice"].sort();

It is a potential problem if the result is assigned to another variable:

const sorted = names.sort();

Or assigned to a property:

const data = {
  names: names.sort()
};

Or passed as an argument:

console.log(names.sort());

The rule I wrote is named no-assign-mutated-array and is in the tslint-etc package. The source code is here.

It works by searching for method calls — like sort — that mutate arrays and checking to see if they are statements. The search is done using Craig Spence’s awesome tsquery.

If the call is a statement, the return value isn’t used — so there is no potential problem.

If the call is not a statement, the rule determines whether or not the array upon which the call is made is transient and effects a rule failure if it isn’t.

There are several methods on Array.prototype that mutate the array and return a reference to the mutated array — fill, reverse and sort — and the rule is enforced for all of them.

The future

In the TypeScript roadmap — for January to June 2019 — it was announced that TypeScript will be switching to ESLint — using @typescript-eslint/parser.

So sometime soon, I’ll start converting the rules in tslint-etc and rxjs-tslint-rules from TSLint rules to ESLint rules. Doing that should be … interesting. And I imagine it’ll prompt me to write a few more linting articles.