Make // eslint-disable-line bad text when no rule is provided

[zepumph]

@samreid and I just found 2 cases where an //eslint-disable-line was hiding a real problem in the code. Can we prevent this from creating more technical debt?

Problems:

Should have a return type but was being hidden:
https://github.com/phetsims/axon/blob/56a3d733eb7cd1dbba328f7d6e8796d3b4623e2f/js/TinyProperty.ts#L171

Should not return any:
https://github.com/phetsims/axon/blob/6ad1413c68e5b46de464144952ce4ae3571980e4/js/TReadOnlyProperty.ts#L22

[samreid]

I fixed the easy part.

[samreid]

I'll be deleting unlinkAttribute next. Having the type parameter in the argument part of a return value callback is causing type errors in places where we could otherwise use Property<unknown>.

[samreid]

Fixed, closing.

[zepumph]

This issue was meant to be a place to discuss a general rule that we shouldn't be able to have // eslint-disable-line allowed, and that instead we should have to specify the exact rules that are disabled. There are currently 300 usages of eslint-disable-line$ and 0 usages of eslint-disable-next-line$. I would recommend removing or adding the specific lint rule to all of them, and I bet that we will find a fair number of places where we were hiding failures accidentally.

[jonathanolson]

Dev meeting: this sounds valuable, initial work should proceed to see if it needs to be a chip-away issue.

[samreid]

For a process, I would recommend using a regex to detect all // eslint-disable-line that ends there (maybe it is a $), query-replace them all with the empty string. Run lint-everything in WebStorm popup window so you can tap on links to visit each case, then add-back the line with the specific rule that is failing there. I'll self-assign to remove this from my todo list, but please reach out to me or we can collaborate on this during our scheduled meetings.

[marlitas]

I attempted to implement this unicorn no-abusive-eslint-disable rule, but unfortunately it applies to eslint-disable for a full file on top of eslint-disable-line and eslint-disable-next-line. This implicated many sound and mip-map files where providing specific lint rule comments begins to get out of hand (over 1,000 errors reported). I think the next step is to look at making this a bad text rule as originally suggested.

I've spent a lot of time on this the past two days, and need to address other issues / work. Will put on the docket to cycle back through once all that is caught up.

[samreid]

Adding a bad-text entry sounds great, bad-text supports regular expressions and we have a working regex for that pattern. And we can avoid bringing in a package.json dependency.

[marlitas]

We discovered weird behavior with bad-text today. It was not reporting our rule as anticipated. To replicate:

• apply this patch
• run grunt lint --disable-eslint-cache in mean-share-and-balance

Subject: [PATCH] Create a preferences demo and content node, see: https://github.com/phetsims/joist/issues/842
---
Index: mean-share-and-balance/js/common/model/MeanShareAndBalanceModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/mean-share-and-balance/js/common/model/MeanShareAndBalanceModel.ts b/mean-share-and-balance/js/common/model/MeanShareAndBalanceModel.ts
--- a/mean-share-and-balance/js/common/model/MeanShareAndBalanceModel.ts	(revision 1601d4fd16ec9ab0d4aaa4b91b546d220d91a263)
+++ b/mean-share-and-balance/js/common/model/MeanShareAndBalanceModel.ts	(date 1670607520495)
@@ -20,6 +20,9 @@
 
   protected constructor( providedOptions: MeanShareAndBalanceModelOptions ) {
     // Here for potential future use.
+
+    const m = 7; // eslint-disable-line
+    console.log( m );
   }
 
   /**
Index: chipper/eslint/rules/getBadTextTester.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/rules/getBadTextTester.js b/chipper/eslint/rules/getBadTextTester.js
--- a/chipper/eslint/rules/getBadTextTester.js	(revision 61eee81754f65dd2a1f01cfc99fe69c2d2e5b128)
+++ b/chipper/eslint/rules/getBadTextTester.js	(date 1670607520489)
@@ -35,6 +35,8 @@
      */
     const reportBadText = ( lineNumber, columnIndex, lineLength, message ) => {
 
+      console.log( 'REPORT BAD TEXT' );
+
       // esprima Token loc object, see https://esprima.readthedocs.io/en/latest/lexical-analysis.html
       const loc = {
         start: { line: lineNumber, column: columnIndex },
@@ -46,6 +48,8 @@
         loc: loc,
         message: `Line contains bad text: '${message}'`
       } );
+
+      console.log( message );
     };
 
     /**
@@ -80,6 +84,9 @@
             }
             else if ( forbiddenText.predicate ) {
               const ok = forbiddenText.predicate( lineString );
+              if ( lineString.includes( 'eslint-disable' ) ) {
+                console.log( lineString, ok );
+              }
               if ( !ok ) {
                 reportBadText( badLineNumber, 0, lineString.length, forbiddenText.id );
               }
Index: chipper/eslint/rules/bad-text.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/rules/bad-text.js b/chipper/eslint/rules/bad-text.js
--- a/chipper/eslint/rules/bad-text.js	(revision 61eee81754f65dd2a1f01cfc99fe69c2d2e5b128)
+++ b/chipper/eslint/rules/bad-text.js	(date 1670607643759)
@@ -8,130 +8,23 @@
  * @author Sam Reid (PhET Interactive Simulations)
  */
 
-
-module.exports = function( context ) {
-
-  const getBadTextTester = require( './getBadTextTester' );
+const getBadTextTester = require( './getBadTextTester' );
 
+module.exports = function( context ) {
+
+
   // see getBadTextTester for schema
   const forbiddenTextObjects = [
 
-    // Proper casing for *boxes
-
-    // toolbox is one word
-    'toolBox', // prefer toolbox
-    'ToolBox', // prefer Toolbox
-    'TOOL_BOX', // prefer TOOLBOX
-
-    // checkbox is one word
-    'checkBox', // prefer checkbox
-    'CheckBox', // prefer Checkbox
-    'CHECK_BOX', // prefer CHECKBOX
-
-    // combo box is two words
-    'combobox', // prefer combo box
-    'Combobox', // prefer Combo Box
-    'COMBOBOX', // prefer COMBO_BOX
-
-    'Overriden', // should have 2 "d"s
-    'overriden', // should have 2 "d"s
-
-    'iFrame', // should be iframe
-
-    // event.keyCode according to spec, rather than event.keycode
-    'keycode',
-
-    // prefer hotkey (one word)
-    'hot key',
-    'hotKey',
-    'HotKey',
-    'HOT_KEY',
-
-    // embarrassingly enough, zepumph is so bad at typing this word that he needs these lint rules, see https://github.com/phetsims/friction/issues/234
-    'respones',
-    'Respones',
-
-    // Avoid string literal in assert predicate, see https://github.com/phetsims/assert/issues/7
-    'assert( \'',
-
-    // In ES6, extending object causes methods to be dropped
-    { id: 'extends Object ', codeTokens: [ 'extends', 'Object' ] },
-
-    // Forbid common duplicate words
-    ' the the ',
-    ' a a ',
-    'dipose', // happens more than you'd think
-
-    // For phet-io use PHET_IO in constants
-    'PHETIO',
-    'PHET-IO',
-    'Phet-iO',
-    ' Phet ',
-    'phetio element', // use "phet-io element" or "PhET-iO element"
-    'Phet-iO',
-    'Property.PropertyIO', // Use PropertyIO directly
-    { id: 'IO type', regex: /\bIO type/ }, // https://github.com/phetsims/chipper/issues/977
-
-    '@return ',
-
-    // see https://thenewstack.io/words-matter-finally-tech-looks-at-removing-exclusionary-language/ and https://github.com/phetsims/special-ops/issues/221
-    {
-      id: 'words matter',
-      regex: /(slave|Slave|blacklist|Blacklist|BlackList)/
-    },
-
-    // Any instances of youtube.com should enforce that we use youtube-nocookie.com
-    // https://github.com/phetsims/website/issues/1376
-    // https://support.google.com/youtube/answer/171780?hl=en#zippy=%2Cturn-on-privacy-enhanced-mode
+    // eslint disable line directives must have an explanation
     {
-      id: 'require youtube privacy enhanced mode',
-      regex: /youtube(?:(?!-nocookie).)*\.com/
-    },
-
-    'Util = require( \'', // Utils should now be plural, see https://github.com/phetsims/tasks/issues/966
-
-    // if on a one line arrow function returning something, prefer instead `() => theReturn`, see https://github.com/phetsims/chipper/issues/790
-    ' => { return ',
-
-    'define( function( require ) {', // use define( require => { to standardize before es6 module migration
-
-    // optional 'options' should use brackets and required 'config' shouldn't use brackets, see https://github.com/phetsims/chipper/issues/859
-    '@param {Object} options',
-    '@param {Object} [config]',
-
-    // PhET prefers to use the term "position" to refer to the physical (x,y) position of objects.
-    // The lint rule can be disabled for occurrences where we do prefer locationProperty, for instance if we
-    // had a sim about people that are from three different named locations.
-    'locationProperty',
-
-    // optionize cannot infer its type arguments, pass them like `optionize<MyOptions. . .>()()
-    'optionize(',
-
-    // In general, you should not duplicate QueryStringMachine getting phetioDebug, instead just use Client.prototype.getDebugModeEnabled(), see https://github.com/phetsims/phet-io/issues/1859
-    'phetioDebug:',
-
-    {
-      id: 'Import statements require a *.js suffix',
+      id: 'eslint-disable-line-directives-must-have-explanation',
       predicate: line => {
-        if ( line.trim().indexOf( 'import \'' ) === 0 && line.indexOf( ';' ) >= 0 && line.indexOf( '.js' ) === -1 ) {
-          return false;
-        }
-        return true;
+        return !line.trim().endsWith( 'eslint-disable-line' );
       }
-    },
 
-    {
-      id: 'Export statements should not have a register call',
-      predicate: line => {
-        if ( line.trim().indexOf( 'export default' ) === 0 && line.indexOf( '.register(' ) >= 0 ) {
-          return false;
-        }
-        return true;
-      }
-    },
-
-    // Should have a period before "<", see https://github.com/phetsims/chipper/issues/1005 and https://github.com/phetsims/chipper/issues/1003
-    { id: 'Type<Parameter> (add a dot)', regex: /{[^\n:]*[A-z]<[A-z][|'<>A-z]+>[^\n:{}]*}}/ }
+      // regex: /eslint-disable-line/
+    }
   ];
 
   return {
Index: chipper/eslint/rules/bad-typescript-text.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/rules/bad-typescript-text.js b/chipper/eslint/rules/bad-typescript-text.js
--- a/chipper/eslint/rules/bad-typescript-text.js	(revision 61eee81754f65dd2a1f01cfc99fe69c2d2e5b128)
+++ b/chipper/eslint/rules/bad-typescript-text.js	(date 1670607865834)
@@ -7,49 +7,14 @@
  *
  * @author Michael Kauzmann (PhET Interactive Simulations)
  */
-
-module.exports = function( context ) {
-
-  const getBadTextTester = require( './getBadTextTester' );
+const getBadTextTester = require( './getBadTextTester' );
 
+module.exports = function( context ) {
+
+
   // see getBadTextTester for schema.
   const forbiddenTextObjects = [
 
-    // Don't lie to yourself.
-    'JavaScript is much, much better than TypeScript.',
-
-    // Typescript handles this for us, please refrain from providing visibility annotations via jsdoc (unless you have
-    // to, disabling this rule).
-    '@public',
-    '@protected',
-    '@private',
-
-    'options = merge',
-
-    // To convert javascript files to typescript, you do not need to include a nocheck directive, just commit locally
-    // before converting to preserve history, see https://github.com/phetsims/sun/issues/732#issuecomment-995330513
-    '@ts-nocheck',
-
-    // combineOptions should always specify the type parameter like combineOptions<...>.
-    'combineOptions(',
-
-    // The type parameters should be inferred rather than specified
-    '.multilink<',
-    '.lazyMultilink<',
-    'new DerivedProperty<',
-
-    'const simOptions = {',
-
-    // Typescript files should not use jsdoc for parameters
-    '@param {',
-
-    // Don't export SelfOptions, https://github.com/phetsims/chipper/issues/1263
-    'export type SelfOptions',
-
-    {
-      id: '@returns with type and/or without extra doc',
-      regex: /(@returns \{)|(@returns *$)/
-    }
   ];
 
   return {

Assigning to @samreid and @zepumph for potential unblocking.

[samreid]

I have a hypothesis that explains all of the bad-text.js problems we saw today. We tried to catch lines that have // eslint-disable-line without an explanation, but the problem was that linting was disabled for those lines. That’s also why our rule caught // eslint-disableline with one hyphen, because lint wasn’t disabled.

[samreid]

OK I added a new option lineNumberDelta so the error can be reported on a different line. That prevents the ignore from ignoring the bad-text. It seems to be working OK, and I added text to the remaining eslist-disable-lines. I think this issue can be closed, but perhaps a spot check from @marlitas would be good.

[marlitas]

@samreid I'm not sure what the ??? in eslint/rules/require-statement-match.js means. Can that be deleted since it's all commented out?

I'm added a rule for eslint-disable-next-line as well.

[samreid]

I removed that extra eslint-disable-line in the comment, and reviewed the behavior of eslint-disable-next-line. Everything seems good. So I think this issue can be closed. Please reopen if there's more to do.