Reevaluate decision to skip running rules when linting whitespace-only files

[not-an-aardvark]

When I was refactoring Linter a few months ago, I was surprised to find that we have a special case when linting text that only contains whitespace; we skip running rules, and always return 0 linting errors.

Looking back at the when that case was added, I found that this was an intentional design decision made in #546. From what I can tell, the reasoning for the decision was that users might not expect ESLint to inform them that their files are empty.

In the 3 years since that decision was made, a few things about ESLint have changed:

• We've introduced whitespace-only rules like no-trailing-spaces, which would have a well-defined and useful behavior for files that only contain whitespace.
• Many more people now write custom rules, and they frequently use RuleTester to test their rules. A whitespace-only string is occasionally used to test that a rule works correctly when a program contains nothing of interest. However, since Linter ignores empty strings, any "valid" test on a whitespace-only string does not run the tested rule at all. This is usually unexpected for rule developers. Case in point: We have 15 buggy tests in the ESLint codebase (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15) where the test author incorrectly assumed a test was making an assertion about a rule, when it actually was not. In my view, this is undesirable because ESLint could unexpectedly conceal rule logic errors, leading to potential bugs.
• We have many more people using ESLint's Node.js API than we used to, so I think it's important that the API remains intuitively consistent to avoid unexpected conditions in integrations. I think it's unexpected that Linter#verify runs rules on almost all strings that get passed to it, but it does not run rules when a string happens to only contain whitespace. This seems like it would lead to unanticipated edge cases for API users.

Due to these issues, I think the current behavior on whitespace-only files is actually more unexpected than the alternative. I think we should remove this special case and have ESLint treat whitespace-only strings the same way it treats other text.

[not-an-aardvark]

TSC Summary: This is a proposal to change ESLint's behavior when it encounters a file that only contains whitespace. Currently, ESLint ignores whitespace-only files by design. However, this design choice arguably makes things more confusing now due to some changes in ESLint, so this issue proposes removing the special case.
TSC Question: Should we remove the special case for whitespace-only files?

[not-an-aardvark]

This issue was discussed in today's TSC meeting. The TSC decided to postpone voting on it so that we can gather more evidence about what rules would be broken by the change.

[not-an-aardvark]

To test how this would affect rules, I applied the following changes:

Diff

diff --git a/lib/linter.js b/lib/linter.js
index 12879842..dcea3df2 100755
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -12,7 +12,6 @@
 const eslintScope = require("eslint-scope"),
     levn = require("levn"),
     lodash = require("lodash"),
-    blankScriptAST = require("../conf/blank-script.json"),
     defaultConfig = require("../conf/default-config-options.js"),
     CodePathAnalyzer = require("./code-path-analysis/code-path-analyzer"),
     ConfigOps = require("./config/config-ops"),
@@ -778,13 +777,6 @@ module.exports = class Linter {
         if (lastSourceCodes.get(this)) {
             parserServices = {};
         } else {
-
-            // there's no input, just exit here
-            if (text.trim().length === 0) {
-                lastSourceCodes.set(this, new SourceCode(text, blankScriptAST));
-                return [];
-            }
-
             const parseResult = parse(
                 stripUnicodeBOM(text).replace(astUtils.SHEBANG_MATCHER, (match, captured) => `//${captured}`),
                 config.parserOptions,
diff --git a/lib/testers/rule-tester.js b/lib/testers/rule-tester.js
index 9861998f..8ae38366 100644
--- a/lib/testers/rule-tester.js
+++ b/lib/testers/rule-tester.js
@@ -530,7 +530,7 @@ class RuleTester {
          */
         RuleTester.describe(ruleName, () => {
             RuleTester.describe("valid", () => {
-                test.valid.forEach(valid => {
+                test.valid.concat("").forEach(valid => {
                     RuleTester.it(typeof valid === "object" ? valid.code : valid, () => {
                         linter.defineRules(this.rules);
                         testValidTemplate(valid);

The patch does the following:

• Removes the special case for whitespace-only files
• Updates RuleTester to always add an empty-string case as "valid", in addition to the other valid cases supplied by the user. (I don't think we would do this if we accepted this change -- it's just being used right now to test how rules respond to empty-string input.)

With these changes applied, I ran the following test suites:

• npm test on the eslint repository
• npm test on the eslint-plugin-react repository
• npm test on the eslint-plugin-import repository
• npm test on the eslint-plugin-node repository

In total, 6 tests failed for 2 total rules. All of the failing tests were in the eslint repository.

Failing tests

  1) eol-last valid :

      AssertionError [ERR_ASSERTION]: Should have no errors but had 1: [ { ruleId: 'eol-last',
    severity: 1,
    message: 'Newline required at end of file but not found.',
    line: 1,
    column: 1,
    nodeType: 'Program',
    source: '',
    fix: { range: [Array], text: '\n' } } ]
      + expected - actual

      -1
      +0

      at testValidTemplate (lib/testers/rule-tester.js:9:10578)
      at Context.RuleTester.it (lib/testers/rule-tester.js:9:16869)

  2) eol-last valid :

      AssertionError [ERR_ASSERTION]: Should have no errors but had 1: [ { ruleId: 'eol-last',
    severity: 1,
    message: 'Newline required at end of file but not found.',
    line: 1,
    column: 1,
    nodeType: 'Program',
    source: '',
    fix: { range: [Array], text: '\n' } } ]
      + expected - actual

      -1
      +0

      at testValidTemplate (lib/testers/rule-tester.js:9:10578)
      at Context.RuleTester.it (lib/testers/rule-tester.js:9:16869)

  3) eol-last valid :

      AssertionError [ERR_ASSERTION]: Should have no errors but had 1: [ { ruleId: 'eol-last',
    severity: 1,
    message: 'Newline required at end of file but not found.',
    line: 1,
    column: 1,
    nodeType: 'Program',
    source: '',
    fix: { range: [Array], text: '\r\n' } } ]
      + expected - actual

      -1
      +0

      at testValidTemplate (lib/testers/rule-tester.js:9:10578)
      at Context.RuleTester.it (lib/testers/rule-tester.js:9:16869)

  4) eol-last valid :

      AssertionError [ERR_ASSERTION]: Should have no errors but had 1: [ { ruleId: 'eol-last',
    severity: 1,
    message: 'Newline required at end of file but not found.',
    line: 1,
    column: 1,
    nodeType: 'Program',
    source: '',
    fix: { range: [Array], text: '\n' } } ]
      + expected - actual

      -1
      +0

      at testValidTemplate (lib/testers/rule-tester.js:9:10578)
      at Context.RuleTester.it (lib/testers/rule-tester.js:9:16869)

  6) no-invalid-meta valid :
     TypeError: Cannot read property 'type' of undefined
      at isCorrectExportsFormat (tools/internal-rules/no-invalid-meta.js:9:4412)
      at Program:exit (tools/internal-rules/no-invalid-meta.js:9:5492)
      at listeners.(anonymous function).forEach.listener (lib/util/safe-emitter.js:9:843)
      at Array.forEach (<anonymous>)
      at Object.emit (lib/util/safe-emitter.js:9:779)
      at NodeEventGenerator.applySelector (lib/util/node-event-generator.js:9:8191)
      at NodeEventGenerator.applySelectors (lib/util/node-event-generator.js:9:9855)
      at NodeEventGenerator.leaveNode (lib/util/node-event-generator.js:9:10374)
      at CodePathAnalyzer.leaveNode (lib/code-path-analysis/code-path-analyzer.js:9:23428)
      at Traverser.leave (lib/linter.js:9:32217)
      at Traverser.__execute (node_modules/estraverse/estraverse.js:397:31)
      at Traverser.traverse (node_modules/estraverse/estraverse.js:491:28)
      at Traverser.traverse (lib/util/traverser.js:9:424)
      at Linter._verifyWithoutProcessors (lib/linter.js:9:31896)
      at preprocess.map.textBlock (lib/linter.js:9:33419)
      at Array.map (<anonymous>)
      at Linter.verify (lib/linter.js:9:33351)
      at runRuleForItem (lib/testers/rule-tester.js:9:9860)
      at testValidTemplate (lib/testers/rule-tester.js:9:10441)
      at Context.RuleTester.it (lib/testers/rule-tester.js:9:16869)

Note: I've omitted some test failures which are unrelated to this proposal. For example, several of the tests for RuleTester fail after applying this patch, which is expected because RuleTester generally is not supposed to append an empty-string test case to the user's tests.

Summary of results:

• This change would cause the eol-last rule to start reporting empty files, because empty files do not contain a trailing newline.
• This change would cause the internal no-invalid-meta rule to crash when run on an empty file. (I note that the rule already crashes on any file that does not contain an AssignmentStatement, so it doesn't seem to me that empty files are a special case here.)

[not-an-aardvark]

This change was accepted in today's TSC meeting. As discussed in the meeting, I'm going to collect some additional data with non-default rule configs to ensure there are no unexpected bugs on empty files.

[j-f1]

This change would cause the eol-last rule to start reporting empty files, because empty files do not contain a trailing newline.

This should be changed IMO — the current explicit "" test seems to say that an empty file should be valid

[platinumazure]

@j-f1 Thanks for calling that out. We did discuss eol-last in the meeting as well and we agree that empty files should be special-cased.

Rationale: eol-last is really about making sure the last line of content in a file has a trailing newline, but empty files have no content.

Or, put another way: It's not really within the mandate of eol-last to warn about empty files- that probably should be done elsewhere if that's something we think needs to be done in an ESLint core rule.

EDIT: I've dived into eol-last. So it turns out three tests (lines 23, 39, and 46 as of current master) are never actually run in the rule, due to the special case behavior in Linter documented above. Thus, they "pass". But with not-an-aardvark's changes above to remove that special case behavior, those three tests currently fail. Since the rule designers intended those cases to pass, I'm working on a PR to allow empty string to pass eol-last once that Linter change goes in (as a semver-major change).

[j-f1]

To allow people to use this feature without a breaking change, could we add a checkEmptyFiles option (false by default) in a minor release, then change the default to true in v5?

This would let people use the feature sooner rather than later, and also disable it if it breaks rules that haven’t been fixed to work on empty files.

[platinumazure]

This would let people use the feature sooner rather than later, and also disable it if it breaks rules that haven’t been fixed to work on empty files.

It would also allow us to gather more data on actual/potential breakage from users when we flip the switch in a major release. I definitely would support something like this.