Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Added prefer-readonly rule with fixes #2896

Merged
merged 22 commits into from
Nov 28, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
"resolve": "^1.3.2",
"semver": "^5.3.0",
"tslib": "^1.7.1",
"tsutils": "^2.3.0"
"tsutils": "^2.4.0"
},
"peerDependencies": {
"typescript": ">=2.1.0 || >=2.1.0-dev || >=2.2.0-dev || >=2.3.0-dev || >=2.4.0-dev"
Expand Down
1 change: 1 addition & 0 deletions src/configs/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export const rules = {
"no-var-requires": true,
"only-arrow-functions": true,
"prefer-for-of": true,
"prefer-readonly": true,
"promise-function-async": true,
"typedef": [
true,
Expand Down
2 changes: 1 addition & 1 deletion src/language/rule/abstractRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export abstract class AbstractRule implements IRule {
protected readonly ruleSeverity: RuleSeverity;
public ruleName: string;

constructor(private options: IOptions) {
constructor(private readonly options: IOptions) {
this.ruleName = options.ruleName;
this.ruleArguments = options.ruleArguments;
this.ruleSeverity = options.ruleSeverity;
Expand Down
18 changes: 9 additions & 9 deletions src/language/rule/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ export class Replacement {
}

export class RuleFailurePosition {
constructor(private position: number, private lineAndCharacter: ts.LineAndCharacter) {
constructor(private readonly position: number, private readonly lineAndCharacter: ts.LineAndCharacter) {
}

public getPosition() {
Expand Down Expand Up @@ -238,18 +238,18 @@ export type Fix = Replacement | Replacement[];
export type FixJson = ReplacementJson | ReplacementJson[];

export class RuleFailure {
private fileName: string;
private startPosition: RuleFailurePosition;
private endPosition: RuleFailurePosition;
private rawLines: string;
private readonly fileName: string;
private readonly startPosition: RuleFailurePosition;
private readonly endPosition: RuleFailurePosition;
private readonly rawLines: string;
private ruleSeverity: RuleSeverity;

constructor(private sourceFile: ts.SourceFile,
constructor(private readonly sourceFile: ts.SourceFile,
start: number,
end: number,
private failure: string,
private ruleName: string,
private fix?: Fix) {
private readonly failure: string,
private readonly ruleName: string,
private readonly fix?: Fix) {

this.fileName = sourceFile.fileName;
this.startPosition = this.createFailurePosition(start);
Expand Down
2 changes: 1 addition & 1 deletion src/language/walker/blockScopeAwareRuleWalker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {ScopeAwareRuleWalker} from "./scopeAwareRuleWalker";
* are a superset of regular scopes (new block scopes are created more frequently in a program).
*/
export abstract class BlockScopeAwareRuleWalker<T, U> extends ScopeAwareRuleWalker<T> {
private blockScopeStack: U[];
private readonly blockScopeStack: U[];

constructor(sourceFile: ts.SourceFile, options: IOptions) {
super(sourceFile, options);
Expand Down
4 changes: 2 additions & 2 deletions src/language/walker/programAwareRuleWalker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import {IOptions} from "../rule/rule";
import {RuleWalker} from "./ruleWalker";

export class ProgramAwareRuleWalker extends RuleWalker {
private typeChecker: ts.TypeChecker;
private readonly typeChecker: ts.TypeChecker;

constructor(sourceFile: ts.SourceFile, options: IOptions, private program: ts.Program) {
constructor(sourceFile: ts.SourceFile, options: IOptions, private readonly program: ts.Program) {
super(sourceFile, options);

this.typeChecker = program.getTypeChecker();
Expand Down
10 changes: 5 additions & 5 deletions src/language/walker/ruleWalker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ import {SyntaxWalker} from "./syntaxWalker";
import {IWalker} from "./walker";

export class RuleWalker extends SyntaxWalker implements IWalker {
private limit: number;
private options?: any[];
private failures: RuleFailure[];
private ruleName: string;
private readonly limit: number;
private readonly options?: any[];
private readonly failures: RuleFailure[];
private readonly ruleName: string;

constructor(private sourceFile: ts.SourceFile, options: IOptions) {
constructor(private readonly sourceFile: ts.SourceFile, options: IOptions) {
super();

this.failures = [];
Expand Down
2 changes: 1 addition & 1 deletion src/language/walker/scopeAwareRuleWalker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import {RuleWalker} from "./ruleWalker";
* }
*/
export abstract class ScopeAwareRuleWalker<T> extends RuleWalker {
private scopeStack: T[];
private readonly scopeStack: T[];

constructor(sourceFile: ts.SourceFile, options: IOptions) {
super(sourceFile, options);
Expand Down
2 changes: 1 addition & 1 deletion src/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class Linter {
return program.getSourceFiles().map((s) => s.fileName).filter((l) => l.substr(-5) !== ".d.ts");
}

constructor(private options: ILinterOptions, private program?: ts.Program) {
constructor(private readonly options: ILinterOptions, private readonly program?: ts.Program) {
if (typeof options !== "object") {
throw new Error(`Unknown Linter options type: ${typeof options}`);
}
Expand Down
2 changes: 1 addition & 1 deletion src/rules/completedDocsRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ class ClassRequirement extends Requirement<IClassRequirementDescriptor> {
}

class CompletedDocsWalker extends Lint.ProgramAwareRuleWalker {
private static modifierAliases: { [i: string]: string } = {
private static readonly modifierAliases: { [i: string]: string } = {
export: "exported",
};

Expand Down
2 changes: 1 addition & 1 deletion src/rules/noInferredEmptyObjectTypeRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class Rule extends Lint.Rules.TypedRule {
}

class NoInferredEmptyObjectTypeRule extends Lint.AbstractWalker<void> {
constructor(sourceFile: ts.SourceFile, ruleName: string, private checker: ts.TypeChecker) {
constructor(sourceFile: ts.SourceFile, ruleName: string, private readonly checker: ts.TypeChecker) {
super(sourceFile, ruleName, undefined);
}

Expand Down
2 changes: 1 addition & 1 deletion src/rules/noReferenceImportRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class Rule extends Lint.Rules.AbstractRule {
}

class NoReferenceImportWalker extends Lint.AbstractWalker<void> {
private imports = new Set<string>();
private readonly imports = new Set<string>();
public walk(sourceFile: ts.SourceFile) {
if (sourceFile.typeReferenceDirectives.length === 0) {
return;
Expand Down
120 changes: 120 additions & 0 deletions src/rules/prefer-readonly/classScope.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/**
* @license
* Copyright 2017 Palantir Technologies, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import * as utils from "tsutils";
import * as ts from "typescript";

const OUTSIDE_CONSTRUCTOR = -1;
const DIRECTLY_INSIDE_CONSTRUCTOR = 0;

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (type.symbol === undefined || parentType.symbol === undefined) {
return false;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;
}
}

return false;
}

export class ClassScope {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private readonly privateModifiableMembers = new Map<string, ParameterOrPropertyDeclaration>();
private readonly privateModifiableStatics = new Map<string, ParameterOrPropertyDeclaration>();
private readonly memberVariableModifications = new Set<string>();
private readonly staticVariableModifications = new Set<string>();

private readonly typeChecker: ts.TypeChecker;
private readonly classType: ts.Type;

private constructorScopeDepth = OUTSIDE_CONSTRUCTOR;

public constructor(classNode: ts.Node, typeChecker: ts.TypeChecker) {
this.classType = typeChecker.getTypeAtLocation(classNode);
this.typeChecker = typeChecker;
}

public addDeclaredVariable(node: ParameterOrPropertyDeclaration) {
if (!utils.isModfierFlagSet(node, ts.ModifierFlags.Private)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to still use the misnamed isModfierFlagSet?

|| utils.isModfierFlagSet(node, ts.ModifierFlags.Readonly)
|| node.name.kind === ts.SyntaxKind.ComputedPropertyName) {
return;
}

if (utils.isModfierFlagSet(node, ts.ModifierFlags.Static)) {
this.privateModifiableStatics.set(node.name.getText(), node);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else {
this.privateModifiableMembers.set(node.name.getText(), node);
}
}

public addVariableModification(node: ts.PropertyAccessExpression) {
const modifierType = this.typeChecker.getTypeAtLocation(node.expression);
if (modifierType.symbol === undefined || !typeIsOrHasBaseType(modifierType, this.classType)) {
return;
}

const toStatic = utils.isObjectType(modifierType) && utils.isObjectFlagSet(modifierType, ts.ObjectFlags.Anonymous);
if (!toStatic && this.constructorScopeDepth === DIRECTLY_INSIDE_CONSTRUCTOR) {
return;
}

const variable = node.name.text;

(toStatic ? this.staticVariableModifications : this.memberVariableModifications).add(variable);
}

public enterConstructor() {
this.constructorScopeDepth = DIRECTLY_INSIDE_CONSTRUCTOR;
}

public exitConstructor() {
this.constructorScopeDepth = OUTSIDE_CONSTRUCTOR;
}

public enterNonConstructorScope() {
if (this.constructorScopeDepth !== OUTSIDE_CONSTRUCTOR) {
this.constructorScopeDepth += 1;
}
}

public exitNonConstructorScope() {
if (this.constructorScopeDepth !== OUTSIDE_CONSTRUCTOR) {
this.constructorScopeDepth -= 1;
}
}

public finalizeUnmodifiedPrivateNonReadonlys() {
this.memberVariableModifications.forEach((variableName) => {
this.privateModifiableMembers.delete(variableName);
});

this.staticVariableModifications.forEach((variableName) => {
this.privateModifiableStatics.delete(variableName);
});

return [
...Array.from(this.privateModifiableMembers.values()),
...Array.from(this.privateModifiableStatics.values()),
];
}
}
Loading