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;
}
}
}
