Add a property_write_non_constant engine to Tsetse's ConformancePattern. Same as property_write, but only triggers on writing non-constant values. PiperOrigin-RevId: 249633880
diff --git a/internal/tsetse/rules/conformance_pattern_rule.ts b/internal/tsetse/rules/conformance_pattern_rule.ts index c78f90c..b683355 100644 --- a/internal/tsetse/rules/conformance_pattern_rule.ts +++ b/internal/tsetse/rules/conformance_pattern_rule.ts
@@ -4,6 +4,7 @@ import {Fixer} from '../util/fixer'; import {Config, MatchedNodeTypes, PatternKind} from '../util/pattern_config'; import {PatternEngine} from '../util/pattern_engines/pattern_engine'; +import {PropertyNonConstantWriteEngine} from '../util/pattern_engines/property_non_constant_write_engine'; import {PropertyWriteEngine} from '../util/pattern_engines/property_write_engine'; /** @@ -27,7 +28,13 @@ let engine: PatternEngine<any>; switch (config.kind) { case PatternKind.BANNED_PROPERTY_WRITE: - engine = new PropertyWriteEngine(config, fixer); + engine = new PropertyWriteEngine( + config as Config<PatternKind.BANNED_PROPERTY_WRITE>, fixer); + break; + case PatternKind.BANNED_PROPERTY_NON_CONSTANT_WRITE: + engine = new PropertyNonConstantWriteEngine( + config as Config<PatternKind.BANNED_PROPERTY_NON_CONSTANT_WRITE>, + fixer); break; default: throw new Error('Config type not recognized, or not implemented yet.');
diff --git a/internal/tsetse/tests/ban_conformance_pattern/property_non_constant_write_test.ts b/internal/tsetse/tests/ban_conformance_pattern/property_non_constant_write_test.ts new file mode 100644 index 0000000..8b51796 --- /dev/null +++ b/internal/tsetse/tests/ban_conformance_pattern/property_non_constant_write_test.ts
@@ -0,0 +1,27 @@ +import 'jasmine'; +import {ConformancePatternRule, PatternKind} from '../../rules/conformance_pattern_rule'; +import {compileAndCheck, customMatchers} from '../../util/testing/test_support'; + +describe('BANNED_PROPERTY_NON_CONSTANT_WRITE', () => { + it('matches a trivial example', () => { + const source = `const q = document.createElement('q');\n` + + `q.cite = 'some example string';\n` + // literal + `q.cite = window.name;\n`; // non-literal + const rule = new ConformancePatternRule({ + errorMessage: 'do not cite dynamically', + kind: PatternKind.BANNED_PROPERTY_NON_CONSTANT_WRITE, + values: ['HTMLQuoteElement.prototype.cite'] + }); + const results = compileAndCheck(rule, source); + + expect(results.length).toBe(1); + expect(results[0]) + .toBeFailureMatching( + {start: 71, end: 91, errorMessage: 'do not cite dynamically'}); + }); +}); + + +beforeEach(() => { + jasmine.addMatchers(customMatchers); +});
diff --git a/internal/tsetse/tests/ban_conformance_pattern/util_test.ts b/internal/tsetse/tests/ban_conformance_pattern/util_test.ts index 0588b47..94ab234 100644 --- a/internal/tsetse/tests/ban_conformance_pattern/util_test.ts +++ b/internal/tsetse/tests/ban_conformance_pattern/util_test.ts
@@ -5,8 +5,9 @@ import 'jasmine'; import {ConformancePatternRule, PatternKind} from '../../rules/conformance_pattern_rule'; -import {setDebug} from '../../util/ast_tools'; -import {compileAndCheck} from '../../util/testing/test_support'; +import {isInStockLibraries, setDebug} from '../../util/ast_tools'; +import {isLiteral} from '../../util/is_literal'; +import {compile, compileAndCheck} from '../../util/testing/test_support'; describe('Debug output', () => { it('turns on and off', () => { @@ -35,3 +36,62 @@ expect(logs).toEqual([`inspecting obj.prop = 'foo'`]); }); }); + +describe('The constant-ness logic', () => { + it('understands constants', () => { + // Keep these to single-expression programs. + const constantExpressionsSources = [ + `'hello'`, + `'hello' + 'hi'`, + `1`, + `1 + 1`, + '`abcdef`', + '`abcd${"ef"}`', + '`abcd${1+1}`+`hi`', + `1 ? 'hi' : 'hello'`, + `window.name ? 'hi' : 'hello'`, + ]; + + // We don't bother with a rule for this one. + const constantProgram = compile(...constantExpressionsSources); + const constantCompiledSources = constantProgram.getSourceFiles(); + const constantTc = constantProgram.getTypeChecker(); + const constantExpressions = + constantCompiledSources.filter(s => !isInStockLibraries(s)) + .map(s => s.statements[0].getChildren()[0]); + + for (const expr of constantExpressions) { + expect(isLiteral(constantTc, expr)) + .toBe( + true, + `Expected "${expr.getFullText()}" to be considered constant.`); + } + }); + + + it('understands non-constants', () => { + const nonconstantExpressionsSources = [ + `window.name`, + `'hello' + window.name`, + `window.name + 'hello'`, + '`abcd${window.name}`', + `1 ? window.name : 'hello'`, + `1 ? 'hello' : window.name`, + ]; + + const nonconstantProgram = compile(...nonconstantExpressionsSources); + const nonconstantCompiledSources = nonconstantProgram.getSourceFiles(); + const nonconstantTc = nonconstantProgram.getTypeChecker(); + const nonconstantExpressions = + nonconstantCompiledSources.filter(s => !isInStockLibraries(s)) + .map(s => s.statements[0].getChildren()[0]); + + for (const expr of nonconstantExpressions) { + expect(isLiteral(nonconstantTc, expr)) + .toBe( + false, + `Expected "${ + expr.getFullText()}" not to be considered constant.`); + } + }); +});
diff --git a/internal/tsetse/util/pattern_config.ts b/internal/tsetse/util/pattern_config.ts index c2748d9..76e2d71 100644 --- a/internal/tsetse/util/pattern_config.ts +++ b/internal/tsetse/util/pattern_config.ts
@@ -7,6 +7,7 @@ */ export enum PatternKind { BANNED_PROPERTY_WRITE = 'banned-property-write', + BANNED_PROPERTY_NON_CONSTANT_WRITE = 'banned-property-non-constant-write' } /** @@ -30,4 +31,7 @@ [PatternKind.BANNED_PROPERTY_WRITE]: ts.BinaryExpression&{ left: ts.PropertyAccessExpression; }; + [PatternKind.BANNED_PROPERTY_NON_CONSTANT_WRITE]: ts.BinaryExpression&{ + left: ts.PropertyAccessExpression; + }; }
diff --git a/internal/tsetse/util/pattern_engines/property_non_constant_write_engine.ts b/internal/tsetse/util/pattern_engines/property_non_constant_write_engine.ts new file mode 100644 index 0000000..9e26cd7 --- /dev/null +++ b/internal/tsetse/util/pattern_engines/property_non_constant_write_engine.ts
@@ -0,0 +1,58 @@ +import * as ts from 'typescript'; +import {Checker} from '../../checker'; +import {ErrorCode} from '../../error_code'; +import {Fix} from '../../failure'; +import {debugLog, isPropertyWriteExpression, shouldExamineNode} from '../ast_tools'; +import {Fixer} from '../fixer'; +import {isLiteral} from '../is_literal'; +import {PropertyMatcher} from '../match_symbol'; +import {Config, MatchedNodeTypes, PatternKind} from '../pattern_config'; +import {PatternEngine} from './pattern_engine'; + +// Just for conciseness. +type BanKind = PatternKind.BANNED_PROPERTY_NON_CONSTANT_WRITE; + +/** + * The engine for BANNED_PROPERTY_NON_CONSTANT_WRITE. + */ +export class PropertyNonConstantWriteEngine extends PatternEngine<BanKind> { + private readonly matcher: PropertyMatcher; + constructor( + config: Config<BanKind>, fixer?: Fixer<MatchedNodeTypes[BanKind]>) { + super(config, fixer); + // TODO: Support more than one single value here, or even build a + // multi-pattern engine. This would help for performance. + if (this.config.values.length !== 1) { + throw new Error( + `BANNED_PROPERTY_NON_CONSTANT_WRITE expects one value, got(${ + this.config.values.join(',')})`); + } + this.matcher = PropertyMatcher.fromSpec(this.config.values[0]); + } + + register(checker: Checker) { + checker.on( + ts.SyntaxKind.BinaryExpression, this.check.bind(this), + ErrorCode.CONFORMANCE_PATTERN); + } + + check(c: Checker, n: ts.BinaryExpression) { + if (!shouldExamineNode(n) || n.getSourceFile().isDeclarationFile || + !isPropertyWriteExpression(n)) { + return; + } + debugLog(`inspecting ${n.getFullText().trim()}`); + if (!this.matcher.matches(n.left, c.typeChecker)) { + debugLog('Not an assignment to the right property'); + return; + } + if (isLiteral(c.typeChecker, n.right)) { + debugLog(`Assigned value (${ + n.right.getFullText()}) is a compile-time constant.`); + return; + } + const fix: Fix|undefined = + this.fixer ? this.fixer.getFixForFlaggedNode(n) : undefined; + c.addFailureAtNode(n, this.config.errorMessage, fix); + } +}
diff --git a/internal/tsetse/util/testing/test_support.ts b/internal/tsetse/util/testing/test_support.ts index 010ad8c..8adb90a 100644 --- a/internal/tsetse/util/testing/test_support.ts +++ b/internal/tsetse/util/testing/test_support.ts
@@ -10,7 +10,13 @@ import {AbstractRule} from '../../rule'; -function compile(...sourceCode: string[]): ts.Program { + +/** + * Turns the provided source (as strings) into a ts.Program. The source files + * will be named `.../file_${n}.ts`, with n the index of the source file in + * the `sourceCode` array. + */ +export function compile(...sourceCode: string[]): ts.Program { const temporaryFolder = os.tmpdir() + `/tslint_test_input_${crypto.randomBytes(16).toString('hex')}`; const fullPaths: string[] = [];