Be Careful with Array Mutators

This article was originally published on my blog.

---

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, sort, and splice — 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.

---

Title photo by Corinne Kutz on Unsplash.

Comments

[Łukasz Wiktor]

Can you give an example of a hard-to-find bug caused by array.sort()?

[Emil Öberg]

// utils.js
export function getTop(list) {
return list
.sort()
.slice(0, 1)
}

// some-other-file.js
import { getTop } from “./utils”

const movies = getMoviesFromDb()
const topMovie = getTop(movies)

// movies is now not what you think it would be.

[Nicholas Jamieson]

The example that Emil gave is pretty typical.

I've seen the bug introduced in a Redux selector that did something similar - it sorted the array using some criteria and returned the top 10 or so elements.

In doing so, the selector mutated the Redux store - something that should never happen. The bug was 'hard-to-find' because:

• the bug was only effected if the app functionality that exercised the selector was used;
• whether or not the bug was effected depended upon the data in the store and how 'sorted' it was; and
• when it was effected, parts of the app seemed to be weirdly inconsistent.