Added prefer-readonly rule with fixes #2896

JoshuaKGoldberg · 2017-06-07T02:40:41Z

PR checklist

Addresses an existing issue: readonly variables #1628
New feature, bugfix, or enhancement
- Includes tests
Documentation update

Overview of change:

Adds a prefer-readonly rule for private member variables that are never re-assigned.

Is there anything you'd like reviewers to focus on?

Am I missing any relevant test cases?

CHANGELOG.md entry:

[new-rule] prefer-readonly

Deceptively simple... I look forward to contributors finding new and exciting test cases.

ajafff · 2017-06-07T13:29:48Z

src/rules/preferReadonlyRule.ts

+ this.handlePropertyDeclaration(node as ts.PropertyDeclaration);
+ break;
+
+ case ts.SyntaxKind.PropertyAccessExpression:


~~Checking only property access may be enough for the initial implemenation, but we should consider using the type checker later to find all references~~
consider using the type checker to find all references

I'd really prefer not to make this a typed-only rule. Seems like an unnecessary restriction, especially given the performance concerns. Would you be ok with it not doing that?

There's no other choice. You need to know if the property is assigned on an object of a (sub)type of the current class. Consider the following code:

class A { private foo; private bar; private baz; public copyTo(other: A) { other.foo = this.foo; // this one needs to be recognized as assignment } public copy(other: {[key: string]: any} { // both should not be handled as assignments to `A#bar` or `A#baz` other.bar = this.bar; other.baz = this.baz; } }

That's really interesting. I didn't think of the copyTo, thanks.

ajafff · 2017-06-07T13:30:38Z

src/rules/preferReadonlyRule.ts

+ this.scope = parentScope;
+ }
+
+ private handleConstructor(node: ts.ConstructorDeclaration) {


handle parameter properties here

ajafff · 2017-06-07T13:31:46Z

src/rules/preferReadonlyRule.ts

+ private handleConstructor(node: ts.ConstructorDeclaration) {
+ this.scope.inConstructor = true;
+ ts.forEachChild(node, this.visitNode);
+ this.scope.inConstructor = false;


that's not sufficient, functions inside the constructor cannot assign readonly properties

ajafff · 2017-06-07T14:24:31Z

src/rules/preferReadonlyRule.ts

+ }
+
+ private handlePropertyAccessExpression(node: ts.PropertyAccessExpression, parent: ts.Node | undefined) {
+ if (parent !== undefined) {


parent will never be undefined here

ajafff · 2017-06-07T14:26:22Z

src/rules/preferReadonlyRule.ts

+ }
+
+ private handlePropertyAccessExpression(node: ts.PropertyAccessExpression, parent: ts.Node | undefined) {
+ if (parent !== undefined) {


this function should really check if node.expression is a subtype of the current class

ajafff · 2017-06-07T14:27:02Z

src/rules/preferReadonlyRule.ts

+
+ private readonly visitNode = (node: ts.Node) => {
+ switch (node.kind) {
+ case ts.SyntaxKind.ClassDeclaration:


what about ClassExpression?

ajafff · 2017-06-07T14:27:33Z

src/rules/preferReadonlyRule.ts

+ private handlePropertyDeclaration(node: ts.PropertyDeclaration) {
+ if (this.scope !== undefined
+ && utils.isModfierFlagSet(node, ts.ModifierFlags.Private)
+ && !utils.isModfierFlagSet(node, ts.ModifierFlags.Readonly)) {


needs special handling for static members

ajafff · 2017-06-09T07:05:36Z

src/rules/preferReadonlyRule.ts

+ private handlePropertyAccessExpression(node: ts.PropertyAccessExpression, parent: ts.Node | undefined) {
+ if (parent !== undefined) {
+ switch (parent.kind) {
+ case ts.SyntaxKind.BinaryExpression:


also needs to handle Postfix and PrefixUnaryExpression to handle this.foo-- and ++this.bar

As suggested: * This now uses the type checker to see if types match * Keeps a stackable state for whether the scope is in a constructor (to deal with nested functions) * Adds a few more corner cases, such as parameter declarations, --/++ expressions, and overlapping classes The variable scoping logic was bleeding pretty heavily into the walker, so I moved it into its own class. Much clearer separation of concerns now.

Also added a test for IIFEs used to set them.

ajafff

first quick review of the latest changes

ajafff · 2017-06-09T10:07:13Z

src/rules/preferReadonlyRule.ts

+
+ case ts.SyntaxKind.PostfixUnaryExpression:
+ case ts.SyntaxKind.PrefixUnaryExpression:
+ this.handlePostfixOrPrefixUnaryExpression(node as ts.PostfixUnaryExpression | ts.PrefixUnaryExpression);


this should be moved into handlePropertyAccessExpression

ajafff · 2017-06-09T10:08:16Z

src/rules/preferReadonlyRule.ts

+
+ case ts.SyntaxKind.ClassDeclaration:
+ case ts.SyntaxKind.ClassExpression:
+ this.handleClassDeclarationOrExpression(node as ts.ClassDeclaration | ts.ClassExpression);


minor nit: ts.ClassLikeDeclaration

ajafff · 2017-06-09T10:09:12Z

src/rules/preferReadonlyRule.ts

+ return;
+ }
+
+ this.scope.addVariableModification(node.operand as ts.PropertyAccessExpression);


needs to check if node.operator is PlusPlusToken or MinusMinusToken

ajafff · 2017-06-09T10:16:57Z

src/rules/preferReadonlyRule.ts

+ }
+
+ private complainOnNode(node: ParameterOrPropertyDeclaration) {
+ const fix = Lint.Replacement.appendText(node.name.getStart(), "readonly ");


just a suggestion: you could replace this with Lint.Replacement.appendText(node.modifiers!.end, " readonly")

Sure. Is there a reason you have a preference for that?

the new modifier is automatically placed next to the others. you don't have to worry about comments, line breaks, etc.

ajafff · 2017-06-09T10:20:22Z

src/rules/preferReadonlyRule.ts

+ private readonly visitNode = (node: ts.Node) => {
+ switch (node.kind) {
+ case ts.SyntaxKind.ArrowFunction:
+ case ts.SyntaxKind.FunctionExpression:


better replace this with utils.isFunctionScopeBoundary

Sorry, how come? I'm not seeing any benefit?

There are a few more kinds to check here: ClassDeclaration, ClassExpression, FunctionDeclaration, GetAccessor, SetAccessor, MethodDeclaration and maybe some more:
http://www.typescriptlang.org/play/#src=class%20Foo%20%7B%0A%20%20%20%20private%20readonly%20foo%3A%20number%3B%0A%0A%20%20%20%20constructor()%20%7B%0A%20%20%20%20%20%20%20%20this.foo%20%3D%202%3B%0A%20%20%20%20%20%20%20%20let%20self%20%3D%20this%3B%0A%20%20%20%20%20%20%20%20class%20Inner%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20prop%20%3D%20self.foo%2B%2B%3B%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%20%20%20%20let%20innerObj%20%3D%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20doStuff()%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%2B%2Bself.foo%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%2C%0A%0A%20%20%20%20%20%20%20%20%20%20%20%20get%20foo()%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20return%20self.foo%2B%2B%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%2C%0A%0A%20%20%20%20%20%20%20%20%20%20%20%20set%20foo(foo)%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20self.foo%20%3D%20foo%3B%0A%20%20%20%20%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%20%20%20%20%7D%0A%0A%20%20%20%20%20%20%20%20function%20fn()%20%7B%0A%20%20%20%20%20%20%20%20%20%20%20%20self.foo%2B%2B%3B%0A%20%20%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%7D

ajafff · 2017-06-09T10:24:45Z

src/rules/prefer-readonly/classScope.ts

+ }
+
+ public exitNonConstructorScope() {
+ if (this.constructorScopeDepth > DIRECTLY_INSIDE_CONSTRUCTOR) {


if (this.constructorScopeDepth !== OUTSIDE_CONSTRUCTOR) --this.constructorScopeDepth;

ajafff · 2017-06-09T10:25:57Z

src/rules/prefer-readonly/classScope.ts

+ }
+
+ public addDeclaredVariable(node: ParameterOrPropertyDeclaration) {
+ if (!utils.isModfierFlagSet(node, ts.ModifierFlags.Private)


i'll release a new version of tsutils today, so you can adjust this to the correct name

Is there a reason to still use the misnamed isModfierFlagSet?

ajafff · 2017-06-09T10:28:50Z

src/rules/prefer-readonly/classScope.ts

+ }
+
+ if (utils.isModfierFlagSet(node, ts.ModifierFlags.Static)) {
+ this.privateModifiableStatics.set(node.name.getText(), node);


the rule should probably just ignore properties with computed names

also the current implementation would consider these to be equal:

private foo; private 'foo'; private "foo";

If you ignore computed property names, you can simply use node.name.text here

ajafff · 2017-06-09T10:32:58Z

src/rules/prefer-readonly/classScope.ts

+ }
+
+ for (const baseType of [type, ...type.getBaseTypes()]) {
+ if (baseType.symbol !== undefined && baseType.symbol.name === parentType.symbol.name) {


out of curiosity, is comparing by name really the best thing we can do here?
That could lead to false positives

Agreed. I didn't find a better alternative - would welcome suggestions!

That's probably ok for now.
There's the internal typescript function isTypeInstanceOf in checker.ts that does exactly what we need here. We should ask to expose this functionality.

ajafff · 2017-06-09T11:07:35Z

src/rules/prefer-readonly/classScope.ts

+export class ClassScope {
+ private readonly privateModifiableMembers = new Map<string, ParameterOrPropertyDeclaration>();
+ private readonly privateModifiableStatics = new Map<string, ParameterOrPropertyDeclaration>();
+ private readonly variableModifications = new Map<string, VariableModifications>();


you could consider making two separate lists of assigned instance members and one for static members

* Split variableModifications into two Sets * Corrected computed property names * Corrected logic around constructor scope depths * Stopped recursing on nodes with declare keywords * ++/-- unary expressions only as modifiers

CircleCI is failing.

Also brought in tsutils@2.4.0. That should fix any caching shenanigans.

…er-readonly

adidahiya · 2017-06-16T21:30:55Z

@ajafff mind taking another look here?

ajafff

LGTM with suggestions

ajafff · 2017-06-18T10:33:08Z

src/rules/preferReadonlyRule.ts

+ description: "Requires that private variables are marked as `readonly` if they're never modified outside of the constructor.",
+ descriptionDetails: Lint.Utils.dedent`
+ If a private variable is only assigned to in the constructor, it should be declared as \`readonly\`.
+ `,


this rule could use a rationale to explain why private members should be readonly. I can't come up with a good reason

ajafff · 2017-06-18T10:38:00Z

src/rules/prefer-readonly/classScope.ts

+ }
+
+ public addDeclaredVariable(node: ParameterOrPropertyDeclaration) {
+ if (!utils.isModfierFlagSet(node, ts.ModifierFlags.Private)


Is there a reason to still use the misnamed isModfierFlagSet?

ajafff · 2017-06-18T10:52:33Z

src/rules/prefer-readonly/classScope.ts

+ }
+
+ for (const baseType of [type, ...type.getBaseTypes()]) {
+ if (baseType.symbol !== undefined && baseType.symbol.name === parentType.symbol.name) {


That's probably ok for now.
There's the internal typescript function isTypeInstanceOf in checker.ts that does exactly what we need here. We should ask to expose this functionality.

ajafff · 2017-06-18T10:55:01Z

src/rules/preferReadonlyRule.ts

+ }
+
+ private handleConstructor(node: ts.ConstructorDeclaration) {
+ if (this.scope !== undefined) {


this.scope should never be undefined, because a constructor can only occur inside of a class

ajafff · 2017-06-18T10:55:41Z

src/rules/preferReadonlyRule.ts

+ }
+
+ private handlePropertyDeclaration(node: ts.PropertyDeclaration) {
+ if (this.scope !== undefined) {


same as above, this.scope will not be undefined

ajafff · 2017-06-18T10:57:34Z

src/rules/preferReadonlyRule.ts

+ }
+
+ private handleDeleteExpression(node: ts.PropertyAccessExpression) {
+ if (this.scope !== undefined) {


you could move this check to handlePropertyAccessExpression, so you don't have to check again here and in the other methods

adidahiya

mostly looks good

adidahiya · 2017-06-19T15:59:53Z

src/rules/prefer-readonly/classScope.ts

+
+export type ParameterOrPropertyDeclaration = ts.ParameterDeclaration | ts.PropertyDeclaration;
+
+function typeIsOrHasBaseType(type: ts.Type, parentType: ts.Type) {


this probably belongs in a more generic utils file? maybe src/language/typeUtils.ts

adidahiya · 2017-06-19T16:03:11Z

src/rules/prefer-readonly/classScope.ts

+ return false;
+}
+
+export class ClassScope {


I know there's one bit of readonly-specific logic here, but I'd slightly prefer moving this class to src/language/classScope.ts

I know I mentioned in another thread that it's cool to have multiple files for a rule implementation, but in this case I think we can reasonably avoid it.

JoshuaKGoldberg · 2017-06-19T20:37:30Z

In the AppVeyor build:

src/linter.ts(199,18): error TS2540: Cannot assign to 'program' because it is a constant or a read-only property.

Unrelated?

# Conflicts: # package.json # src/language/walker/blockScopeAwareRuleWalker.ts

ajafff · 2017-09-15T20:05:42Z

src/language/classScope.ts

@@ -0,0 +1,108 @@
+/**


I don't think this file belongs here...
I know @adidahiya suggested moving it, but this is not reusable at all.
I'd prefer to include it in the rule's source file. It will only get you to a total of about 300 lines, which is still reasonable.

ajafff · 2017-09-15T20:22:45Z

src/rules/preferReadonlyRule.ts

+ optionsDescription: "Not configurable.",
+ rationale: Lint.Utils.dedent`
+ Marking never-modified variables as readonly helps enforce the code's intent of keeping them as never-modified.
+ It can also help prevent accidental changes of members not meant to be changed.`,


In general I declare properties readonly if I don't want them to be modified, regardless of the access modifier.
This rule forces me to declare all private properties as readonly that are currently not modified. If later on I decide to modify such a property, I need to edit 2 locations in the file, which bloats the diff.
I can imagine my co-workers not making a change (or making a really awkward one instead), because they think the property is readonly on purpose and needs to stay that way. I think this may cause more confusion than good.

I think this may cause more confusion than good.

Funny, I've had the opposite problem: coworkers reassigning properties that were never meant to be reassigned.

Partially to alleviate that concern, I've added an only-inline-lambdas option that stops checking everything except private member = () => {}-style arrow functions.

# Conflicts: # src/language/rule/rule.ts # src/rules/noReferenceImportRule.ts

# Conflicts: # src/rules/completedDocsRule.ts

JoshuaKGoldberg · 2017-11-24T08:28:14Z

Ping @adidahiya ?

adidahiya · 2017-11-27T18:37:54Z

lgtm -- CI status is not being reported, maybe because of the typescript@next test runner

ajafff · 2017-11-28T08:56:05Z

@adidahiya CI was successful, but did not report. That's weird. Maybe reopening the PR and/or manually starting CI helps.

Besides this issue, there is still a TODO for you: #3490 (comment)

Josh Goldberg added 5 commits June 6, 2017 19:06

Added prefer-readonly rule with fixes

267ad63

Deceptively simple... I look forward to contributors finding new and exciting test cases.

Added member variable functions as test cases

c88cdb2

Removed unused scope member

f7abbe0

Added prefer-readonly to tslint:all

e37e2e3

Fixed internal source violations of prefer-readonly

0c1fb66

ajafff suggested changes Jun 7, 2017

View reviewed changes

adidahiya added the PR: waiting for author label Jun 8, 2017

ajafff reviewed Jun 9, 2017

View reviewed changes

Josh Goldberg added 3 commits June 9, 2017 02:13

Added missing deletion tests

efea6ec

Fixed parameter propertie by checking in handleConstructor

569ac5a

Also added a test for IIFEs used to set them.

ajafff suggested changes Jun 9, 2017

View reviewed changes

Josh Goldberg added 6 commits June 11, 2017 17:16

Mostly addressed ajafff's feedback

f5fdb8e

* Split variableModifications into two Sets * Corrected computed property names * Corrected logic around constructor scope depths * Stopped recursing on nodes with declare keywords * ++/-- unary expressions only as modifiers

Went back to isModfier

42a336d

CircleCI is failing.

Fixed OUTSIDE_CONSTRUCTOR settings

ae96afb

Used utils.isFunctionScopeBoundary to better handle them

a8ea477

Also brought in tsutils@2.4.0. That should fix any caching shenanigans.

Cleaned ++/-- to be within property access checks

69543a4

Merge branch 'master' of https://github.com/palantir/tslint into pref…

b68d4cf

…er-readonly

adidahiya added PR: waiting for reviewer and removed PR: waiting for author labels Jun 16, 2017

ajafff approved these changes Jun 18, 2017

View reviewed changes

Josh Goldberg added 2 commits June 18, 2017 16:08

Feedback: rationale, this.scope!, isModifier update

4dd52ce

Fixed new build errors

b0c2b11

adidahiya suggested changes Jun 19, 2017

View reviewed changes

adidahiya removed the PR: waiting for reviewer label Jun 19, 2017

Moved classScope and typeUtils, as per feedback

1f2e78d

Merge branch 'master' into prefer-readonly

946b52e

# Conflicts: # package.json # src/language/walker/blockScopeAwareRuleWalker.ts

Made program not readonly

a20e2c8

ajafff reviewed Sep 15, 2017

View reviewed changes

Josh Goldberg added 3 commits November 4, 2017 10:27

Merge branch 'master' into prefer-readonly

fbdda51

# Conflicts: # src/language/rule/rule.ts # src/rules/noReferenceImportRule.ts

Converted to applyWithFunction; added only-inline-lambdas option

8efa5bc

Merge branch 'master' into prefer-readonly

6918b76

# Conflicts: # src/rules/completedDocsRule.ts

adidahiya approved these changes Nov 27, 2017

View reviewed changes

adidahiya merged commit 1bd48b5 into palantir:master Nov 28, 2017

JoshuaKGoldberg deleted the prefer-readonly branch November 28, 2017 16:35

adidahiya mentioned this pull request Jan 12, 2018

readonly variables #1628

Closed

HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018

Added prefer-readonly rule with fixes (palantir#2896)

d77a6da


		export type ParameterOrPropertyDeclaration = ts.ParameterDeclaration \| ts.PropertyDeclaration;

		function typeIsOrHasBaseType(type: ts.Type, parentType: ts.Type) {

Added prefer-readonly rule with fixes #2896

Added prefer-readonly rule with fixes #2896

Conversation

JoshuaKGoldberg commented Jun 7, 2017 • edited Loading

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

ajafff Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Jun 16, 2017

ajafff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Nov 24, 2017

adidahiya commented Nov 27, 2017

ajafff commented Nov 28, 2017

JoshuaKGoldberg commented Jun 7, 2017 •

edited

Loading

ajafff Jun 7, 2017 •

edited

Loading

JoshuaKGoldberg commented Jun 19, 2017 •

edited

Loading