Discovering and fixing a bug in JSLint and JSHint

By: James Allardice

Tags:

  • javascript
  • jslint
  • jshint

JSLint, and the popular fork of it JSHint, are static analysis tools for JavaScript programs. They are designed to alert the developer to parts of code that could potentially cause problems, be that a fatal syntax error or a lack of convention that could cause maintenance problems in the future.

Recently I’ve spent a lot of time playing around with JSLint and JSHint, both for my work on jslinterrors.com and in preparation for the use of JSLint to enforce a coding style. During this investigation, I came across a potentially nasty little bug in both tools.

A bit of background

In JavaScript (as in most languages) you cannot use a reserved word as an identifier. For example, the following will fail with a syntax error:

var default = 10;

In the most widely supported iteration of the language, ECMAScript 3, it is also illegal to use a reserved word as an object property identifier:

var myObject = {
    default: 10
};

However, in the most recent version of the language specification ECMAScript 5, which now enjoys widespread support across the major browsers, as well as in Node.js, a change has been made to allow the use of reserved words as object property identifers. This means that the above example is actually valid and will not cause a syntax error in any modern browser.

The details of the change can be seen if we delve into the ECMAScript spec. First, here’s the relevant snippets from the old ES3 version:

ObjectLiteral :
    { }
    { PropertyNameAndValueList }

PropertyNameAndValueList :
    PropertyName : AssignmentExpression
    PropertyNameAndValueList , PropertyName : AssignmentExpression

PropertyName :
    Identifier
    StringLiteral
    NumericLiteral

The important part from that snippet is the Identifier production of PropertyName. If we look at the grammar for identifiers, we see that reserved words are not allowed:

Identifier ::
    IdentifierName but not ReservedWord

If we now look at the same section from the ES5 spec, we can see the slight change to the grammar for PropertyName:

PropertyName :
    IdentifierName
    StringLiteral
    NumericLiteral

It’s a small change, but a significant one. The Identifer production has been replaced with IdentifierName, which is effectively the same as Identifier but without the restriction on reserved words. It’s this change that means in ES5-supporting environments we are able to use reserved words as object property identifiers.

How do JSLint and JSHint fit into this?

Both JSLint and JSHint will warn you about the use of a reserved word as an identifier by default. If you are writing code that will only run in ES5-supporting environments, you can set an option (the es5 option) that surpresses this warning:

/*jslint es5: true */
var myObject = {
    default: 10 // Valid JavaScript, does not cause a JSLint/JSHint error
};

This is useful, expected behaviour. However, when the es5 option is set, JSLint and JSHint will also allow you to use a reserved word as a variable or function identifier, as well as an object property identifier:

/*jslint es5: true */
var default = 10; // Invalid JavaScript, does not cause a JSLint/JSHint error

This is incorrect and will actually throw a syntax error in all environments, regardless of ES5 support. This is a problem, especially if you rely on JSLint/JSHint to give confidence in your code (perhaps as part of your build process), as you could easily end up releasing broken code.

Tracking down the cause of the bug

Following through the JSLint source led me to the property_name function, which is called when the parser encounters a object property identifier. The first thing it does is call another function, optional_identifier, passing through true as its lone argument. This is the only call to optional_identifier that ever passes through an argument and the function signature for optional_identifier does not contain any formal parameters.

At this point, I am guessing that when JSLint was written this was a slight oversight and the author simply forgot to handle this specific case within the optional_identifier function. A quick look at the implementation of the function reveals that no arguments are considered, and the error in question is raised if the identifier is a reserved word and the es5 option does not evaluate to true:

function optional_identifier() {
    if (next_token.identifier) {
        advance();
        if (option.safe && banned[token.string]) {
            warn('adsafe_a', token);
        } else if (token.reserved && !option.es5) {
            warn('expected_identifier_a_reserved', token); // This line raises the error in question
        }
        return token.string;
    }
}

We can make a simple change to this function to take into account the argument passed in by the call mentioned earlier and ensure JSLint continues to raise this error when it encounters a reserved word as a variable or function identifier:

function optional_identifier(prop) { // Added a formal parameter
    if (next_token.identifier) {
        advance();
        if (option.safe && banned[token.string]) {
            warn('adsafe_a', token);
        } else if (token.reserved && (!prop || !option.es5)) { // Handle that parameter
            warn('expected_identifier_a_reserved', token);
        }
        return token.string;
    }
}

The fix is pretty much the same for JSHint, since it started life as a straight fork of JSLint and much of the internals remain unchanged. I opened a pull request in JSHint on GitHub, which has since been merged in, meaning this is no longer an issue in JSHint. The problem remains in JSLint for now, but we hope to get a patch submitted to it in the near future.


About the Author

James Allardice

James Allardice is a Senior JavaScript Developer at Venntro. He is passionate about clean, maintainable JavaScript and is regularly on the lookout for ways to improve the JavaScript development process. He's currently interested in JSHint, Grunt, AngularJS and ECMAScript6.