Typescript: += on private property doesn't count as a read for --noUnusedLocals

[DaveCousineau]

TypeScript Version: 2.9.2

Search Terms:
--noUnusedLocals
+=
private property

Code
tsconfig.json:

{
   "compileOnSave": true,
   "compilerOptions": {
      "target": "es6",
      "module": "none",
      "noImplicitAny": true,
      "removeComments": false,
      "outFile": "test.js",
      "declaration": false,
      "noEmitOnError": true,
      "sourceMap": false,
      "strictNullChecks": true,
      "noUnusedLocals": true,
      "noUnusedParameters": true
   }
}

test.ts

export class TestClass {
   private mNumber: number = 0;
   private get Number(): number { return this.mNumber; }
   private set Number(value: number) { this.mNumber = value; }

   public DoSomething() {
      this.Number += 1;
   }
}

Result

c:\Temp>tsc
test.ts(3,16): error TS6133: 'Number' is declared but its value is never read.

Expected behavior:
+= should count as both a read and a write

Actual behavior:
+= seems to count as a write but not a read. though this problem doesn't occur with a public property.

Playground Link:
I don't think I can set --noUnusedLocals on the playground

Related Issues:
I found this "noUnusedLocals checks don't work with private modifier": https://github.com/Microsoft/TypeScript/issues/15775
but it was closed

[mhegazy]

PRs welcomed

[DanielRosenwasser]

I still feel like this is somewhat questionable - the getter's value is still never observed outside of the assignment.

[AnyhowStep]

A better error message, then?

[DaveCousineau]

@DanielRosenwasser

In my particular case, the reason that this comes up is that the property setter recalculates something else when the value changes. I otherwise use the field to read the value, except for the one += usage. I actually temporarily removed the getter, since it was supposedly not used, but then ended up getting NaN showing up in places when += was used with no getter. What I've had to do is replace the += with this.Number = this.Number + 1;. Maybe it should be redesigned slightly and this is an indication of a code smell, but I don't see that x = x + 1 should count as a read, but x += 1 shouldn't.

[DaveCousineau]

Here is a more realistic example that's a little closer to what I really have:

export class YesNoCounter {
   private mYes: number = 0;
   private get Yes(): number { return this.mYes; }
   private set Yes(value: number) { this.mYes = value; /* raise value changed event */ }

   private mNo: number = 0;
   private get No(): number { return this.mNo; }
   private set No(value: number) { this.mNo = value; /* raise value changed event */ }

   private get Total(): number { return this.mYes + this.mNo; }

   public get LabelText(): string { return `${this.mYes}/${this.Total}`; }

   public AddYes() { this.Yes += 1; }
   public AddNo() { this.No += 1; }
}

There are many ways to avoid the error, but I don't see a good reason why there should be an error.

[DanielRosenwasser]

It seems reasonable to relax it, but since this is a private declaration, I'd think we should be able to special-case the getter/setter case.

[moccaplusplus]

Could I take this issue?
This would be my first contribution here, so any assistance is welcome.

[hikarinotomadoi]

Since there's no activity recently on this issue, I've created a PR fixing this.
By this PR, write-only access to class member with a setter will be treated as a reference.
Without setter, noUnusedLocals warning will still appear.
https://github.com/microsoft/TypeScript/pull/35922

[markusjohnsson]

@DaveCousineau your second example is actually flawed. The Yes and No properties are still not read anywhere else. You can make the error go away by using the property to read them in Total and LabelText:

export class YesNoCounter {
   private mYes: number = 0;
   private get Yes(): number { return this.mYes; }
   private set Yes(value: number) { this.mYes = value; /* raise value changed event */ }

   private mNo: number = 0;
   private get No(): number { return this.mNo; }
   private set No(value: number) { this.mNo = value; /* raise value changed event */ }

   private get Total(): number { return this.Yes + this.No; }

   public get LabelText(): string { return `${this.Yes}/${this.Total}`; }

   public AddYes() { this.Yes += 1; }
   public AddNo() { this.No += 1; }
}

[DaveCousineau]

@markusjohnsson I know there are ways to avoid the error, but I don't believe it should be an error in the first place. this.Yes += 1; should count as a read. Otherwise the compiler tells me the property is never read, which means I should be able to remove the getter. Except that if I remove the getter, then I end up with NaN in the field due to += not working with no getter. (It may additionally be a problem that I can compile += with no getter.)

[AnyhowStep]

It just seems like a code smell that you would have a private getter only for the += and not actually use the getter anywhere else. Instead, choosing to use mYes over Yes.

In your specific case, sure, you're actually reading it.

But I think in the more general case, if someone had a getter that was only used for +=, they're probably doing something wrong. Probably.

---

At the moment, TS gives you an error with --noUnusedLocals.
But TS can be seen as just-a-linter-with-strong-opinions at the end of the day.

Making one user use @ts-ignore seems safer than opening the gateway to suspicious looking code for everyone else who creates private getters and forgets to actually use them.

---

TS' goal isn't to admit all possible safe code. But to admit as much as it can while being dev-friendly. I feel like your request is just not dev-friendly in the general case.

Compiling += without a getter is definitely not ideal, though. Definitely agree on that.

[hikarinotomadoi]

So, adding an error message on "+= without getter" will be a solution?
Keeping the current behavior of noUnusedLocals.
I personally think further discussion can be done at ESLint or else.

[DaveCousineau]

Well then the compiler is going to tell me both that the property is not read _and_ that I can't remove the getter? That would be contradictory. It is just factually incorrect to say that += is not a read, regardless of the code quality.

Additionally, what if there were side-effects in the getter, such as raising an event? Referring to the field internally would avoid the event. You end up having to go through hoops to satisfy the condition that += doesn't count as a read when it literally is a read.

If it's not fixed then I will just have to know to use x = x + 1 in these cases, but IMO that's a workaround for a compilation bug.

[DaveCousineau]

Here's another example:

export class MyClass {
   private mField: number = 0;
   private get Property(): number { return this.mField; console.log("Property was read"); }
   private set Property(value: number) { this.mField = value; console.log("Property was written"); }

   public DoSomething() {
      ...
      this.Property += 1;
      ...
   }
}

[markusjohnsson]

This is not specific to setter/getters, this code has the same "problem":

function foo() {
   let x = 0; // 'x' is declared but its value is never read.(6133)
   for (var i = 0; i < 100; i++) {
      x += 1;
   }
}

As the variable is not read anywhere else, it is considered unused.

However, removing the variable declaration would yield an error at the increment. In the getter/setter case, it actually changes runtime behaviour without reporting further errors. Starting to see the problem 😕

[DaveCousineau]

@markusjohnsson I think in that case, you haven't read the variable except to increment it, so what are you using it for? (It still should count as a read IMO, but it is still finding an issue you can improve in that case. You can remove x.)

In the getter/setter case, there can be side effects in the property, and there can also be a backing field, so it's a bit more complicated than just an unnecessary local variable.

[AnyhowStep]

Let's take a step back.

This,

Additionally, what if there were side-effects in the getter, such as raising an event?

And this,

Referring to the field internally would avoid the event.

So, you have a private getter. That has side-effects.
And you only want to refer to this private-getter-with-side-effects when you use +=?

How is this not a serious code smell?

Getters with side-effects are a crazy code smell.
Private getters that are only intended to trigger side-effects when you use += internally is even worse.

[DaveCousineau]

Properties with side-effects are one of the reasons to use getters/setters rather than fields in the first place. If I really am not allowed to use that as an example, then how about this. (Keeping in mind that it's _just an example_.)

export class MyClass {
   private mField: number = 0;
   private get Property(): number { return this.mField + 1; }
   private set Property(value: number) { this.mField = value; }

   public DoSomething() {
      ...
      this.Property += 1;
      ...
   }
}

[AnyhowStep]

Still looks like a code smell to me.

This should be a no-op (if I were to see such a statement without looking at the getter and setter),

this.Property = this.Property

But with what you have, it's an increment operation.

[DaveCousineau]

Yes, this last one doesn't make much sense. But, if you're saying that I can neither have side-effects in my property nor have a computed property with a setter, then what reason am I left with to use a getter/setter at all?

[AnyhowStep]

There may be valid use cases for saying += counts as using a private-getter but I'm just not seeing them at all. I'm not saying you're absolutely wrong; I'm just having trouble agreeing. Not that you need a random internet person to agree with you.

---

what reason am I left with to use a getter/setter at all?

It's not like I have a list of scenarios where getters/setters are appropriate, or how they should be used. I'm just looking at these things for the first time and pointing out stuff that seems suspicious to me.

I don't use getters and setters, in general. I prefer to be explicit and have getX() and setX() functions. I'm not against the idea of getters and setters being used, though. They can make APIs more usable.

But if I had to think about "valid" uses of getters and setters...

1. Getter-only that computes result but should have no outside-observable side-effects (Your this.mField+1 example)
2. Getter-only that lazy inits result but should have no outside-observable side-effects
3. Getter-and-setter
   
   
   
   • No outside-observable side-effects on getter
   
   
   • this.Property = this.Property; should be a no-op
   
   
   • Side-effects on setter are okay
   
   
4. Setter-only (highly questionable, in my opinion; just use a setYes() function)
   
   
   
   • Side-effects on setter are okay
   
   

The list is non-exhaustive, I can't think of stuff on the spot =x

---

I'm just thinking,

If I had getX() and setX(value) methods, how would I expect them to behave?

And using them to guide how I'd expect getters and setters to behave (and what would be a code smell)

[DaveCousineau]

The scenario that I was originally using was a private getter/setter where the setter raises a public ValueChanged event. This seems to match your third option. (Well aside from self-assignment being no-op. I think you mean that the value shouldn't change, otherwise you're excluding all side-effects by saying no-op.)

[AnyhowStep]

Except, you have a private-getter that is only used for +=.

Your specific case may work out; but it just looks suspicious, in general. (to me)

[DaveCousineau]

Except, you have a private-getter that is only used for +=.

Because I would like to raise the event there.

There are lots of ways out of the situation, the simplest being to use x = x + 1. (In dealing with events, I've decided since then that I think it's best to raise events at specific times and not to put them in properties like this, since any particular situation may or may not call for an event.)

It just strikes me as blatantly incorrect to say that the property is not read, and indeed, removing the getter leads immediately to problems. I think this was overlooked because in the local variable case, you can just remove the variable with no issue.

If the consensus is that this cannot be used in a non-smelly way, then I think the error message should be changed for this case. "+= only use of private property deemed unnecessary" or something. It's factually incorrect to say that the property is not read, and there are immediate consequences to that inaccuracy.