no-unused-vars typescript-eslint rule

[pixelzoom]

Related to #1114, I temporarily enabled this rule in chipper/eslint/.eslintrc.js:

'@typescript-eslint/no-unused-vars': 'error'

...and there are 254 errors.

This lint rule is described at https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unused-vars.md. It has the same options as ESLint's no-unused-vars rule, see https://eslint.org/docs/rules/no-unused-vars#options.

Most of the errors in the PhET codebase are like these examples, where the code is conforming to an API (defined by a superclass, interface, or duck-typing) or defining a callback that conforms to an API. In the callback case, PhET has allowed unused parameters at developer discretion, and I think that we should continue to do so -- I strongly prefer a callback that looks like the API that it's supposed to satisfy.

/// BendingLightCircle.ts -- both params are unused
  getRotatedInstance( angle: number, rotationCenter: Vector2 ) {
    return this;
  }

// PressListener.ts -- param event is unused
  up( event: PressListenerEvent ) {
    sceneryLog && sceneryLog.InputListener && sceneryLog.InputListener( `PressListener#${this._id} up` );
    sceneryLog && sceneryLog.InputListener && sceneryLog.push();

    // Recalculate over/hovering Properties.
    this.invalidateOver();
    this.invalidateHovering();

    sceneryLog && sceneryLog.InputListener && sceneryLog.pop();
  }

// LightSpotNode.ts -- param projectionScreenPosition is unused
    projectionScreen.positionProperty.link( projectionScreenPosition => {
      const projectionScreenShape = projectionScreen.getScreenShapeTranslated();
      fillAndStrokeNode.clipArea = modelViewTransform.modelToViewShape( projectionScreenShape );
    } );

On the other hand, there are examples like this one where the unused parameter looks like a potential bug:

// KickButtonGroup.ts -- param tandem is unused 
    const createLabel = ( label: string, tandem: Tandem ) => {
      const text = new Text( label, {
        maxWidth: TEXT_MAX_WIDTH,
        font: CAVConstants.BUTTON_FONT
      } );
      return {
        label: alignGroup.createBox( text ),
        text: text
      };
    };

It would nice if we could identify rule setting that identify potential bugs, but allow unsed params when a method/function is conforming to an API. I'm not sure what to recommend.

[chrisklus]

From 5/9 supplemental typescript dev meeting:

One option for next steps:

Turn on the lint rule to find out the distribution of cases. If most can have the param removed, then we like this rule. But we identified cases where the API should not be changed, so perhaps there are many of those cases too. In that scenario, there are a few options we could proceed with. The lint rule has some options like none for the args option, or argsIgnorePattern. We could use a suffix like Unused, so varNameUnused.

@zepumph volunteered to take a look, thanks!

[pixelzoom]

@zepumph FYI, you're going to run into some cases in simula-rasa where the "Unused" suffix is going to be problematic. For example, in SimulaRasaModel.ts:

  constructor( providedOptions: SimulaRasaModelOptions ) {
    //TODO
  }
...
  public step( dt: number ): void {
    //TODO
  }

If we change these to providedOptionsUnused and dtUnused respectively, then either the sim developer will need to remove the "Unused" suffix, or (worse) they may start using a parameter whose name is "Unused."

I'm not sure what to recommend, but wanted to point it out before you get too far along.

[pixelzoom]

The rule config for "Unused" suffix is probably:

       '@typescript-eslint/no-unused-vars': [ 'error', {
          argsIgnorePattern: 'Unused$'
        } ],

[zepumph]

I got pretty blocked on needed the unused callback args for type inference in Multilink and DerivedProperty, see phetsims/axon#395, but for now I feel like this is largely on hold. I can add the variable side of things for now, I'll take a look.

[zepumph]

Using this rule, I found 7 sports with errors in Ratio and Proportion, 3 were bugs, and 4 were false positives from needing them for DerivedProperty and Multilink.

        '@typescript-eslint/no-unused-vars': [ 'error', {
          args: 'after-used'
        } ],

[zepumph]

Ok, it is on for variables, but this isn't that helpful because we already had this as an eslint rule. We can work on this more when we figure out what we want to do with phetsims/axon#395

[pixelzoom]

In .eslintrc:

        '@typescript-eslint/no-unused-vars': [ 'error', {
          args: 'none' // TODO: we want to turn this for arguments in https://github.com/phetsims/chipper/issues/1230
        } ],

@zepumph is the missing word in the TODO comment "on" or "off"?

[zepumph]

We discussed this today, but because of this example, we don't want to turn on the rule as it is implemented for "args":

class X {

  // The signature defined by the base class.
  public doSomething( n: number ): void {
    console.log( n );
  }
}
          
class Y extends X {
            
  // This is a lint error to have n defined but unused.
  // But if you remove it, Z.doSomething has a type error.
  public override doSomething( n:number): void {}
}
          
class Z extends Y {
  public override doSomething( n: number ): void {
    console.log( n );
  }
}

[marlitas]

It seems like we have come to a decision to not implement this rule and that this issue is now completed. Closing, feel free to re-open if that is not the case.