Repository: jshint/jshint
Pull request #3185 (2017-09-10 20:39:44)

[[DOCS]] Expand warning message
`export` declarations with empty export export clauses are not
necessarily superfluous: if they also specify a "from" clause, they will
cause the referenced module to be evaluated. Because of this, the
warning produced in these cases should be contextual--it should
recommend removal only if no "from" clause is present. In the presence
of such a clause, the warning should instead suggest use of a
binding-free `import` declaration (as this clearly communicates the
intention to evaluate a module without requesting any bindings).

Notes on compatibility:

- The previous warning behavior had not yet been published in a stable
  release (it was implemented at the same time as the parser was
  corrected to support `export` declarations without bindings). This
  makes this change in warning behavior backward-compatible with prior
  releases.
- This warning is relevant for `import` declarations as well. However,
  because prior releases of JSHint included correct parsing support for
  binding-free `import` declarations, introducing an equivalent warning
  would constitute a backwards-breaking change.

@rwaldron This is a follow-up to gh-2569 which I just landed today.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/13206600/badge)](https://coveralls.io/builds/13206600)

Coverage increased (+0.001%) to 98.746% when pulling **38695cd1179f00a31888619ca7a8949134caa8ed on jugglinmike:empty-warnings** into **aed66d7abe1220e4e6afa9ce6ff23a24a97302de on jshint:master**.


Patch:
diff --git a/src/jshint.js b/src/jshint.js
index 17d29cd6..3a50af0b 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -3412,7 +3412,7 @@ var JSHINT = (function() {
         advance("[");
       }
       if (checkPunctuator(state.tokens.next, "]")) {
-        warning("W137", state.tokens.curr, "destructuring");
+        warning("W137", state.tokens.curr);
       }
       var element_after_rest = false;
       while (!checkPunctuator(state.tokens.next, "]")) {
@@ -3444,7 +3444,7 @@ var JSHINT = (function() {
         advance("{");
       }
       if (checkPunctuator(state.tokens.next, "}")) {
-        warning("W137", state.tokens.curr, "destructuring");
+        warning("W137", state.tokens.curr);
       }
       while (!checkPunctuator(state.tokens.next, "}")) {
         assignmentProperty();
@@ -4634,10 +4634,6 @@ var JSHINT = (function() {
     } else {
       // ImportClause :: NamedImports
       advance("{");
-      // Breaking change
-      // if (checkPunctuator(state.tokens.next, "}")) {
-      //   warning("W137", state.tokens.next, "import");
-      // }
       for (;;) {
         if (state.tokens.next.value === "}") {
           advance("}");
@@ -4676,6 +4672,15 @@ var JSHINT = (function() {
     // FromClause
     advance("from");
     advance("(string)");
+
+    // Support for ES2015 modules was released without warning for `import`
+    // declarations that lack bindings. Issuing a warning would therefor
+    // constitute a breaking change.
+    // TODO: enable this warning in JSHint 3
+    // if (hasBindings) {
+    //   warning("W142", this, "import", moduleSpecifier);
+    // }
+
     return this;
   }).exps = true;
 
@@ -4683,6 +4688,7 @@ var JSHINT = (function() {
     var ok = true;
     var token;
     var identifier;
+    var moduleSpecifier;
 
     if (!state.inES6()) {
       warning("W119", state.tokens.curr, "export", "6");
@@ -4735,9 +4741,6 @@ var JSHINT = (function() {
     if (state.tokens.next.value === "{") {
       // ExportDeclaration :: export ExportClause
       advance("{");
-      if (checkPunctuator(state.tokens.next, "}")) {
-        warning("W137", state.tokens.next, "export");
-      }
       var exportedTokens = [];
       while (!checkPunctuator(state.tokens.next, "}")) {
         if (!state.tokens.next.identifier) {
@@ -4763,12 +4766,22 @@ var JSHINT = (function() {
       if (state.tokens.next.value === "from") {
         // ExportDeclaration :: export ExportClause FromClause
         advance("from");
+        moduleSpecifier = state.tokens.next;
         advance("(string)");
       } else if (ok) {
         exportedTokens.forEach(function(token) {
           state.funct["(scope)"].setExported(token.value, token);
         });
       }
+
+      if (exportedTokens.length === 0) {
+        if (moduleSpecifier) {
+          warning("W142", this, "export", moduleSpecifier.value);
+        } else {
+          warning("W141", this, "export");
+        }
+      }
+
       return this;
     }
 
diff --git a/src/messages.js b/src/messages.js
index bf7aa2e5..665e71da 100644
--- a/src/messages.js
+++ b/src/messages.js
@@ -222,10 +222,12 @@ var warnings = {
   W134: "The '{a}' option is only available when linting ECMAScript {b} code.",
   W135: "{a} may not be supported by non-browser environments.",
   W136: "'{a}' must be in function scope.",
-  W137: "Empty {a}: this is unnecessary and can be removed.",
+  W137: "Empty destructuring: this is unnecessary and can be removed.",
   W138: "Regular parameters should not come after default parameters.",
   W139: "Function expressions should not be used as the second operand to instanceof.",
-  W140: "Missing comma."
+  W140: "Missing comma.",
+  W141: "Empty {a}: this is unnecessary and can be removed.",
+  W142: "Empty {a}: consider replacing with `import '{b}';`."
 };
 
 var info = {


Test patch:
diff --git a/tests/unit/core.js b/tests/unit/core.js
index d61ab709..f8a1f7a1 100644
--- a/tests/unit/core.js
+++ b/tests/unit/core.js
@@ -828,7 +828,8 @@ exports.testES6Modules = function (test) {
   ];
 
   var testRun = TestRun(test)
-    .addError(74, 9, "Empty export: this is unnecessary and can be removed.");
+    .addError(74, 1, "Empty export: this is unnecessary and can be removed.")
+    .addError(75, 1, "Empty export: consider replacing with `import 'source';`.");
   importConstErrors.forEach(function(error) { testRun.addError.apply(testRun, error); });
   testRun.test(src, {esnext: true});
 
@@ -867,7 +868,8 @@ exports.testES6Modules = function (test) {
     .addError(70, 1, "'export' is only available in ES6 (use 'esversion: 6').")
     .addError(71, 1, "'import' is only available in ES6 (use 'esversion: 6').")
     .addError(74, 1, "'export' is only available in ES6 (use 'esversion: 6').")
-    .addError(75, 1, "'import' is only available in ES6 (use 'esversion: 6').")
+    .addError(75, 1, "'export' is only available in ES6 (use 'esversion: 6').")
+    .addError(76, 1, "'import' is only available in ES6 (use 'esversion: 6').")
     .test(src);
 
   var src2 = [
diff --git a/tests/unit/fixtures/es6-import-export.js b/tests/unit/fixtures/es6-import-export.js
index 53594887..fe0088a7 100644
--- a/tests/unit/fixtures/es6-import-export.js
+++ b/tests/unit/fixtures/es6-import-export.js
@@ -72,4 +72,5 @@ import { x, } from "source";
 
 // Empty
 export {};
+export {} from "source";
 import {} from "source";