Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Private members used only via [] notation trigger --noUnusedLocals error #14137

Closed
studds opened this issue Feb 17, 2017 · 5 comments · Fixed by #45974
Closed

Private members used only via [] notation trigger --noUnusedLocals error #14137

studds opened this issue Feb 17, 2017 · 5 comments · Fixed by #45974
Labels
Bug A bug in TypeScript Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this
Milestone

Comments

@studds
Copy link

studds commented Feb 17, 2017

TypeScript Version: 2.1.1 / nightly (2.2.0-dev.201xxxxx)

Version 2.1.6

Code

class PhoneBook {
   private sally: number = 123; // error TS6133: 'sally' is declared but never used.
   private sam: number =456; // error TS6133: 'sam' is declared but never used.
   getNumber(who: 'sally' | 'wendy'): number {
       return this[who]; // tsc is able to correctly deduce that this is a number
   }
}
const phoneBook = new PhoneBook();
console.log(phoneBook.getNumber('sally'));

Expected behavior:

Using a private member via [] notation should not trigger --noUnusedLocals errors.

Actual behavior:

A private member used only via [] triggers --noUnusedLocals errors.

@studds
Copy link
Author

studds commented Feb 17, 2017

It could reasonably be argued that this is not an obvious or perhaps common use case. And indeed, this can be worked around. Obviously, this example is contrived in the extreme. And yet, I would maintain that in some cases it may be a reasonable use case which obeys (I believe) all the rules of the language, and ought not trigger a compiler error.

@kemsky
Copy link

kemsky commented Mar 29, 2017

It does not work in a more simple case like object["property"].

@mysticatea
Copy link

I have encountered this issue. In my case, I use state property for dynamic dispatching.
Can I avoid those errors and keep dynamic dispatching?

type ParserState = "A_STATE" | "B_STATE" | "Z_STATE"

class Parser {
    private state: ParserState

    constructor() {
        this.state = "A_STATE"
    }

    parse() {
        while (this.state !== "Z_STATE") {
            this.state = this[this.state]()
        }
    }

    private A_STATE(): ParserState { /* .... */ } // 'A_STATE' declared but never used.
    private B_STATE(): ParserState { /* .... */ } // 'B_STATE' declared but never used.
    private Z_STATE(): ParserState { /* .... */ } // 'Z_STATE' declared but never used.
}

@Manusan42
Copy link

I have a similar problem like mysticatea...

@injectable()
export class MasterHandler {
  @inject(TypeKeys.AHandler) private _aHandler: Types.AHandler;
  @inject(TypeKeys.BHandler) private _bHandler: Types.BHandler;

  async process(request: Request): Promise<Response> {
    return await this[`_${request.scope}Handler`][request.action](request.payload);
  }
}

However, getting forced to remove some intentional unused locals like specific imports, I decided to use tslint till tsc supports warning messages. Besides that I just agree with alexeagle who said in issue #15953 :

So I believe TypeScript should avoid non-fatal diagnostics (and unused variable was a mistake).

To overcome the need of activated type info for tslint core rule no-unused-variable I came up with ajafff's tslint-consistent-codestyle custom rule no-unused which workes quiet fine (without type info) for many cases, just class members are ignored for now, but it's a brand new rule, so let's wait and see.

@mhegazy mhegazy added Design Limitation Constraints of the existing architecture prevent this from being fixed Bug A bug in TypeScript Help Wanted You can do this Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". and removed Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed labels Apr 12, 2018
@mhegazy mhegazy added this to the Community milestone Apr 12, 2018
@Dervol03
Copy link

+1

I opt for improving this as well, because the the dynamic dispatching (as named by @mysticatea) is a very efficient way of implementing the strategy pattern in TypeScript that I use a lot, as it removes the necessity of an extra object only existing to map a string to a certain function.

Here is an example of a class converting a JSON schema into a valid elasticsearch mapping, that only serves the purpose of demonstration, in a real world, I wouldn't actually implement it as a class.

Example:

class JsonToEsConverter {
  private _esMapping: EsMap = {}

  constructor(private _schema: JSONSchema) {}

  public get esMapping(): EsMap {
    if (_.isEmpty(this._esMapping)) {
      this.buildEsMapping();
    }
    return this._esMapping;
  }

  private buildEsMapping(): void {
    for (const prop in this._schema.properties) {
      this._esMapping[prop] = this[`${_schema.properties[prop]}ToEsMap](_schema.properties[prop]);
    }
  }

  private stringToEsMap(stringProp..){}
  private integerToEsMap(intProp..){}
  private arrayToEsMap(arrayProp...) {}
}

This is actually nice and concise. However, the compiler does not understand, that the -ToEsMap methods are actually used and throws an error.
Which forces me to either extract them into a separate module, which violates the separation of concerns principle and considerable augments the number of arguments the methods would have to take, or, to "manually" create an additional map property to hold these methods:

class JsonToEsConverter {
  private _esMapping: EsMap = {};
  private _converterMap: {[converterName: string]: Function};

  constructor(private _schema: JSONSchema) {
    this._converterMap = {
      stringToEsMap: this.stringToEsMap.bind(this);
      integerToEsMap: this.integerToEsMap.bind(this);
      arrayToEsMap: this.arrayToEsMap.bind(this);
    }
  }

  public get esMapping(): EsMap {
    if (_.isEmpty(this._esMapping)) {
      this.buildEsMapping();
    }
    return this._esMapping;
  }

  private buildEsMapping(): void {
    for (const prop in this._schema.properties) {
      this._esMapping[prop] = this[`${_schema.properties[prop]}ToEsMap](_schema.properties[prop]);
    }
  }

  private stringToEsMap(stringProp..){}
  private integerToEsMap(intProp..){}
  private arrayToEsMap(arrayProp...) {}
}

Which doesn't really help. Or worse, move everything to static methods, which creates the same problems as the ones with a separate module

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants