Add an in-config ConformanceRule whitelist. This whitelist is file-granular, based on the jscompiler Conformance configs. PiperOrigin-RevId: 252021771
diff --git a/internal/tsetse/failure.ts b/internal/tsetse/failure.ts index 20126ac..7db67ef 100644 --- a/internal/tsetse/failure.ts +++ b/internal/tsetse/failure.ts
@@ -36,9 +36,9 @@ } toString(): string { - return `Failure{sourceFile:${ + return `{ sourceFile:${ this.sourceFile ? this.sourceFile.fileName : - 'unknown'}, start:${this.start}, end:${this.end}}`; + 'unknown'}, start:${this.start}, end:${this.end} }`; } }
diff --git a/internal/tsetse/tests/ban_conformance_pattern/whitelist_test.ts b/internal/tsetse/tests/ban_conformance_pattern/whitelist_test.ts new file mode 100644 index 0000000..a1c7342 --- /dev/null +++ b/internal/tsetse/tests/ban_conformance_pattern/whitelist_test.ts
@@ -0,0 +1,122 @@ +import 'jasmine'; +import {ConformancePatternRule, PatternKind} from '../../rules/conformance_pattern_rule'; +import {WhitelistReason} from '../../util/pattern_config'; +import {compileAndCheck, customMatchers, getTempDirForWhitelist} from '../../util/testing/test_support'; + +const tmpPrefixForWhitelist = getTempDirForWhitelist(); +const tmpRegexpForWhitelist = + `^(?:${getTempDirForWhitelist().replace(/\\/g, '\\\\')})`; + + +describe('ConformancePatternRule', () => { + describe('whitelist handling', () => { + const source = `export {};\n` + + `const q = document.createElement('q');\n` + + `q.cite = 'some example string';\n`; + + // The initial config off which we run those checks. + const baseConfig = { + errorMessage: 'do not cite', + kind: PatternKind.BANNED_PROPERTY_WRITE, + values: ['HTMLQuoteElement.prototype.cite'], + }; + + it('matches if no whitelist (sanity check)', () => { + const config = {...baseConfig, whitelistEntries: []}; + const rule = new ConformancePatternRule(config); + const results = compileAndCheck(rule, source); + + expect(results).toHaveNFailures(1, config); + }); + + it('matches if there is an empty whitelist group', () => { + const config = { + ...baseConfig, + whitelistEntries: [{ + reason: WhitelistReason.UNSPECIFIED, + }] + }; + const rule = new ConformancePatternRule(config); + const results = compileAndCheck(rule, source); + + expect(results).toHaveNFailures(1, config); + }); + + it('respects prefix-based whitelists (matching test)', () => { + const config = { + ...baseConfig, + whitelistEntries: [{ + reason: WhitelistReason.UNSPECIFIED, + prefix: [tmpPrefixForWhitelist], + }] + }; + const rule = new ConformancePatternRule(config); + const results = compileAndCheck(rule, source); + + expect(results).toHaveNFailures(0, config); + }); + + it('respects prefix-based whitelists (non-matching test)', () => { + const config = { + ...baseConfig, + whitelistEntries: [{ + reason: WhitelistReason.UNSPECIFIED, + prefix: ['/nowhere in particular/'], + }] + }; + const rule = new ConformancePatternRule(config); + const results = compileAndCheck(rule, source); + + expect(results).toHaveNFailures(1, config); + }); + + it('respects regex-based whitelists', () => { + const config = { + ...baseConfig, + whitelistEntries: [{ + reason: WhitelistReason.UNSPECIFIED, + regexp: [`${tmpRegexpForWhitelist}.+/file_0\\.ts`] + }] + }; + const rule = new ConformancePatternRule(config); + const results = compileAndCheck(rule, source); + + expect(results).toHaveNFailures(0, config); + }); + + it('accepts several regex-based whitelists', () => { + const config = { + ...baseConfig, + whitelistEntries: [{ + reason: WhitelistReason.UNSPECIFIED, + regexp: [ + `${tmpRegexpForWhitelist}.+/file_0\\.ts`, + `${tmpRegexpForWhitelist}.+/file_1\\.ts` + ] + }] + }; + const rule = new ConformancePatternRule(config); + // Testing two times the same file so that both regexps match. + const results = compileAndCheck(rule, source, source); + + expect(results).toHaveNFailures(0, config); + }); + + it('throws on creation of invalid regexps', () => { + const config = { + ...baseConfig, + whitelistEntries: [{ + reason: WhitelistReason.UNSPECIFIED, + regexp: ['(', '/tmp/', 'foo'], + }] + }; + expect(() => { + new ConformancePatternRule(config); + }).toThrowError(/Invalid regular expression/); + }); + }); +}); + +beforeEach(() => { + jasmine.addMatchers(customMatchers); +});
diff --git a/internal/tsetse/util/pattern_config.ts b/internal/tsetse/util/pattern_config.ts index 76e2d71..c8c0927 100644 --- a/internal/tsetse/util/pattern_config.ts +++ b/internal/tsetse/util/pattern_config.ts
@@ -15,6 +15,7 @@ */ export interface Config<P extends PatternKind> { kind: P; + /** * Values have a pattern-specific syntax. * @@ -22,8 +23,60 @@ * for examples. */ values: string[]; + /** The error message this pattern will create. */ errorMessage: string; + + /** A list of whitelist blocks. */ + whitelistEntries?: WhitelistEntry[]; +} + +/** + * A whitelist entry, corresponding to a logical whitelisting rule. Use these to + * distinguish between various logical reasons for whitelisting something: for + * instance, tie these to particular bugs that needed whitelisting, per legacy + * project, manually reviewed entries, and so on. + * + * Whitelists are based on the file paths provided by the TS compiler, with both + * regexp-based checks and prefix-based checks. + * + * + * Follows the logic in + * https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/conformance.proto. + */ +export interface WhitelistEntry { + /** The category corresponding to this entry. */ + reason: WhitelistReason; + /** Why is this okay to whitelist. */ + explanation?: string; + + /** + * Regexps for the paths of files that will be ignored by the + * ConformancePattern. Beware, escaping can be tricky. + */ + regexp?: string[]; + /** + * Prefixes for the paths of files that will be ignored by the + * ConformancePattern. + */ + prefix?: string[]; +} + +/** + * The categories of whitelist entries. + */ +export enum WhitelistReason { + /** No reason. */ + UNSPECIFIED, + /** Code that has to be grandfathered in (no guarantees). */ + LEGACY, + /** + * Code that does not enter the scope of this particular check (no + * guarantees). + */ + OUT_OF_SCOPE, + /** Manually reviewed exceptions (supposedly okay). */ + MANUALLY_REVIEWED } /** Maps the type of nodes that each `PatternType` produces. */
diff --git a/internal/tsetse/util/pattern_engines/pattern_engine.ts b/internal/tsetse/util/pattern_engines/pattern_engine.ts index 5dbbddc..6e01420 100644 --- a/internal/tsetse/util/pattern_engines/pattern_engine.ts +++ b/internal/tsetse/util/pattern_engines/pattern_engine.ts
@@ -1,14 +1,85 @@ +import * as ts from 'typescript'; import {Checker} from '../../checker'; +import {Fix} from '../../failure'; import {Fixer} from '../../util/fixer'; import {Config, MatchedNodeTypes, PatternKind} from '../../util/pattern_config'; +import {shouldExamineNode} from '../ast_tools'; /** * A patternEngine is the logic that handles a specific PatternKind. */ export abstract class PatternEngine<P extends PatternKind> { + private readonly whitelistedPrefixes: string[] = []; + private readonly whitelistedRegExps: RegExp[] = []; + private readonly whitelistMemoizer: Map<string, boolean> = new Map(); + constructor( protected readonly config: Config<P>, - protected readonly fixer?: Fixer<MatchedNodeTypes[P]>) {} + protected readonly fixer?: Fixer<MatchedNodeTypes[P]>) { + if (config.whitelistEntries) { + for (const e of config.whitelistEntries) { + if (e.prefix) { + this.whitelistedPrefixes = + this.whitelistedPrefixes.concat(...e.prefix); + } + if (e.regexp) { + this.whitelistedRegExps = this.whitelistedRegExps.concat( + ...e.regexp.map(r => new RegExp(r))); + } + } + } + } + /** + * `register` will be called by the ConformanceRule to tell Tsetse the + * PatternEngine will handle matching. Implementations should use + *`checkAndFilterResults` as a wrapper for `check`. + **/ abstract register(checker: Checker): void; + + /** + * `check` is the PatternEngine subclass-specific matching logic. Overwrite + * with what the engine looks for, i.e., AST matching. The whitelisting logic + * and fix generation are handled in `checkAndFilterResults`. + */ + abstract check(tc: ts.TypeChecker, n: MatchedNodeTypes[P]): + MatchedNodeTypes[P]|undefined; + + /** + * A wrapper for `check` that handles aspects of the analysis that are not + * engine-specific, and which defers to the subclass-specific logic + * afterwards. + */ + checkAndFilterResults(c: Checker, n: MatchedNodeTypes[P]) { + if (!shouldExamineNode(n) || n.getSourceFile().isDeclarationFile) { + return; + } + const matchedNode = this.check(c.typeChecker, n); + if (matchedNode && !this.isWhitelisted(matchedNode)) { + const fix: Fix|undefined = + this.fixer ? this.fixer.getFixForFlaggedNode(matchedNode) : undefined; + c.addFailureAtNode(matchedNode, this.config.errorMessage, fix); + } + } + + isWhitelisted(n: ts.Node): boolean { + const name: string = n.getSourceFile().fileName; + if (this.whitelistMemoizer.has(name)) { + return this.whitelistMemoizer.get(name)!; + } + for (const p of this.whitelistedPrefixes) { + if (name.indexOf(p) == 0) { + this.whitelistMemoizer.set(name, true); + return true; + } + } + for (const re of this.whitelistedRegExps) { + if (re.test(name)) { + this.whitelistMemoizer.set(name, true); + return true; + } + } + this.whitelistMemoizer.set(name, false); + return false; + } }
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 index 9e26cd7..d9d3a97 100644 --- a/internal/tsetse/util/pattern_engines/property_non_constant_write_engine.ts +++ b/internal/tsetse/util/pattern_engines/property_non_constant_write_engine.ts
@@ -1,8 +1,7 @@ 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 {debugLog, isPropertyWriteExpression} from '../ast_tools'; import {Fixer} from '../fixer'; import {isLiteral} from '../is_literal'; import {PropertyMatcher} from '../match_symbol'; @@ -32,27 +31,25 @@ register(checker: Checker) { checker.on( - ts.SyntaxKind.BinaryExpression, this.check.bind(this), + ts.SyntaxKind.BinaryExpression, this.checkAndFilterResults.bind(this), ErrorCode.CONFORMANCE_PATTERN); } - check(c: Checker, n: ts.BinaryExpression) { - if (!shouldExamineNode(n) || n.getSourceFile().isDeclarationFile || - !isPropertyWriteExpression(n)) { + check(tc: ts.TypeChecker, n: MatchedNodeTypes[BanKind]): + MatchedNodeTypes[BanKind]|undefined { + if (!isPropertyWriteExpression(n)) { return; } debugLog(`inspecting ${n.getFullText().trim()}`); - if (!this.matcher.matches(n.left, c.typeChecker)) { + if (!this.matcher.matches(n.left, tc)) { debugLog('Not an assignment to the right property'); return; } - if (isLiteral(c.typeChecker, n.right)) { + if (isLiteral(tc, 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); + return n; } }
diff --git a/internal/tsetse/util/pattern_engines/property_write_engine.ts b/internal/tsetse/util/pattern_engines/property_write_engine.ts index ea90e60..3b92f2d 100644 --- a/internal/tsetse/util/pattern_engines/property_write_engine.ts +++ b/internal/tsetse/util/pattern_engines/property_write_engine.ts
@@ -1,22 +1,22 @@ 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 {debugLog, isPropertyWriteExpression} from '../ast_tools'; import {Fixer} from '../fixer'; import {PropertyMatcher} from '../match_symbol'; import {Config, MatchedNodeTypes, PatternKind} from '../pattern_config'; import {PatternEngine} from '../pattern_engines/pattern_engine'; +// Just for conciseness. +type BanKind = PatternKind.BANNED_PROPERTY_WRITE; + /** * The engine for BANNED_PROPERTY_WRITE. */ -export class PropertyWriteEngine extends - PatternEngine<PatternKind.BANNED_PROPERTY_WRITE> { +export class PropertyWriteEngine extends PatternEngine<BanKind> { private readonly matcher: PropertyMatcher; constructor( - config: Config<PatternKind.BANNED_PROPERTY_WRITE>, - fixer?: Fixer<MatchedNodeTypes[PatternKind.BANNED_PROPERTY_WRITE]>) { + 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. @@ -29,22 +29,21 @@ register(checker: Checker) { checker.on( - ts.SyntaxKind.BinaryExpression, this.check.bind(this), + ts.SyntaxKind.BinaryExpression, this.checkAndFilterResults.bind(this), ErrorCode.CONFORMANCE_PATTERN); } - check(c: Checker, n: ts.BinaryExpression) { - if (!shouldExamineNode(n) || n.getSourceFile().isDeclarationFile || - !isPropertyWriteExpression(n)) { + check(tc: ts.TypeChecker, n: MatchedNodeTypes[BanKind]): + MatchedNodeTypes[BanKind]|undefined { + if (!isPropertyWriteExpression(n)) { return; } debugLog(`inspecting ${n.getText().trim()}`); - if (this.matcher.matches(n.left, c.typeChecker)) { - const fix: Fix|undefined = - this.fixer ? this.fixer.getFixForFlaggedNode(n) : undefined; - debugLog(`Match. Reporting failure (boundaries: ${n.getStart()}, ${ - n.getEnd()}] on node [${n.getText()}]`); - c.addFailureAtNode(n, this.config.errorMessage, fix); + if (!this.matcher.matches(n.left, tc)) { + return; } + debugLog(`Match. Reporting failure (boundaries: ${n.getStart()}, ${ + n.getEnd()}] on node [${n.getText()}]`); + return n; } }
diff --git a/internal/tsetse/util/testing/test_support.ts b/internal/tsetse/util/testing/test_support.ts index 8e3d6d0..7b93b7f 100644 --- a/internal/tsetse/util/testing/test_support.ts +++ b/internal/tsetse/util/testing/test_support.ts
@@ -3,11 +3,13 @@ import * as crypto from 'crypto'; import * as fs from 'fs'; import * as os from 'os'; +import * as path from 'path'; import * as ts from 'typescript'; import {Checker} from '../../checker'; import {Failure} from '../../failure'; import {AbstractRule} from '../../rule'; +import {Config} from '../pattern_config'; @@ -17,11 +19,11 @@ * the `sourceCode` array. */ export function compile(...sourceCode: string[]): ts.Program { - const temporaryFolder = os.tmpdir() + - `/tslint_test_input_${crypto.randomBytes(16).toString('hex')}`; + const temporaryFolder = os.tmpdir() + path.sep + + `tslint_test_input_${crypto.randomBytes(16).toString('hex')}`; const fullPaths: string[] = []; sourceCode.forEach((s, i) => { - fullPaths.push(`${temporaryFolder}/file_${i}.ts`); + fullPaths.push(`${temporaryFolder}${path.sep}file_${i}.ts`); }); let error: Error|undefined = undefined; @@ -66,8 +68,43 @@ return check(rule, program); } +/** Turns a Failure to a fileName. */ +export function toFileName(f: Failure) { + const file = f.toDiagnostic().file; + return file ? file.fileName : 'unknown'; +} + +export function getTempDirForWhitelist() { + // TS uses forward slashes on Windows ¯\_(ツ)_/¯ + return os.platform() == 'win32' ? os.tmpdir().replace(/\\/g, '/') : + os.tmpdir(); +} + // Custom matcher for Jasmine, for a better experience matching fixes. export const customMatchers: jasmine.CustomMatcherFactories = { + + toHaveNFailures(): jasmine.CustomMatcher { + return { + compare: (actual: Failure[], expected: Number, config?: Config<any>) => { + if (actual.length === expected) { + return {pass: true}; + } else { + let message = + `Expected ${expected} Failures, but found ${actual.length}.`; + if (actual.length) { + message += '\n' + actual.map(f => f.toString()).join('\n'); + } + if (config) { + message += `\nConfig: {kind:${config.kind}, values:${ + JSON.stringify(config.values)}, whitelist:${ + JSON.stringify(config.whitelistEntries)} }`; + } + return {pass: false, message}; + } + } + }; + }, + toBeFailureMatching(): jasmine.CustomMatcher { return { compare: (actualFailure: Failure, exp: { @@ -135,6 +172,8 @@ end?: number, matchedCode?: string, }): void; + + toHaveNFailures(expected: Number, config?: Config<any>): void; } } }