Swap the verbosity mechanism for ConformancePatternRule from an argument to a global switch. PiperOrigin-RevId: 249425522
diff --git a/internal/tsetse/rules/conformance_pattern_rule.ts b/internal/tsetse/rules/conformance_pattern_rule.ts index ec3c8bf..c78f90c 100644 --- a/internal/tsetse/rules/conformance_pattern_rule.ts +++ b/internal/tsetse/rules/conformance_pattern_rule.ts
@@ -20,16 +20,14 @@ private readonly engine: PatternEngine<P>; - constructor( - config: Config<P>, fixer?: Fixer<MatchedNodeTypes[P]>, - verbose?: boolean) { + constructor(config: Config<P>, fixer?: Fixer<MatchedNodeTypes[P]>) { // TODO(rjamet): This cheats a bit with the typing, as TS doesn't realize // that P is Config.kind. // tslint:disable-next-line:no-any See above. let engine: PatternEngine<any>; switch (config.kind) { case PatternKind.BANNED_PROPERTY_WRITE: - engine = new PropertyWriteEngine(config, fixer, verbose); + engine = new PropertyWriteEngine(config, fixer); break; default: throw new Error('Config type not recognized, or not implemented yet.');
diff --git a/internal/tsetse/tests/ban_conformance_pattern/util_test.ts b/internal/tsetse/tests/ban_conformance_pattern/util_test.ts new file mode 100644 index 0000000..0588b47 --- /dev/null +++ b/internal/tsetse/tests/ban_conformance_pattern/util_test.ts
@@ -0,0 +1,37 @@ +/** + * @fileoverview Tests for the various utilities that are not tied to a single + * rule. + */ + +import 'jasmine'; +import {ConformancePatternRule, PatternKind} from '../../rules/conformance_pattern_rule'; +import {setDebug} from '../../util/ast_tools'; +import {compileAndCheck} from '../../util/testing/test_support'; + +describe('Debug output', () => { + it('turns on and off', () => { + const source = `const obj = {prop:'val'}; obj.prop = 'foo';`; + const rule = new ConformancePatternRule({ + errorMessage: 'does not matter', + kind: PatternKind.BANNED_PROPERTY_WRITE, + values: ['HTMLQuoteElement.prototype.cite'] + }); + + const logs: string[] = []; + const realConsoleLog = console.log; + spyOn(console, 'log').and.callFake((s: string) => { + logs.push(s); + realConsoleLog(s); + }); + setDebug(true); + compileAndCheck(rule, source); + + expect(logs).toEqual([`inspecting obj.prop = 'foo'`]); + + setDebug(false); + compileAndCheck(rule, source); + + // Nothing more appended: debug was false + expect(logs).toEqual([`inspecting obj.prop = 'foo'`]); + }); +});
diff --git a/internal/tsetse/util/ast_tools.ts b/internal/tsetse/util/ast_tools.ts index 44d26ee..cb0f0cb 100644 --- a/internal/tsetse/util/ast_tools.ts +++ b/internal/tsetse/util/ast_tools.ts
@@ -6,6 +6,25 @@ import * as ts from 'typescript'; /** + * Triggers increased verbosity in the rules. + */ +let DEBUG = false; + +/** + * Turns on or off logging for ConformancePatternRules. + */ +export function setDebug(state: boolean) { + DEBUG = state; +} + +/** + * Debug helper. + */ +export function debugLog(msg: string) { + if (DEBUG) console.log(msg); +} + +/** * Returns `n`'s parents in order. */ export function parents(n: ts.Node): ts.Node[] { @@ -146,13 +165,6 @@ } /** - * Debug helper. - */ -export function debugLog(verbose: boolean|undefined, msg: string) { - if (verbose) console.info(msg); -} - -/** * If verbose, logs the given error that happened while walking n, with a * stacktrace. */ @@ -163,7 +175,6 @@ } catch { } debugLog( - verbose, `Walking node ${nodeText} failed with error ${e}.\n` + - `Stacktrace:\n${e.stack}`); + `Stacktrace:\n${e.stack}`); }
diff --git a/internal/tsetse/util/fixer.ts b/internal/tsetse/util/fixer.ts index 19de12a..f9b784e 100644 --- a/internal/tsetse/util/fixer.ts +++ b/internal/tsetse/util/fixer.ts
@@ -52,20 +52,19 @@ */ export function maybeAddNamedImport( source: ts.SourceFile, importWhat: string, fromFile: string, - importAs?: string, tazeComment?: string, v?: boolean): IndividualChange| - undefined { + importAs?: string, tazeComment?: string): IndividualChange|undefined { const importStatements = source.statements.filter(ts.isImportDeclaration); const importSpecifier = importAs ? `${importWhat} as ${importAs}` : importWhat; for (const iDecl of importStatements) { - const parsedDecl = maybeParseImportNode(iDecl, v); + const parsedDecl = maybeParseImportNode(iDecl); if (!parsedDecl || parsedDecl.fromFile !== fromFile) { // Not an import from the right file, or couldn't understand the import. continue; // Jump to the next import. } if (ts.isNamespaceImport(parsedDecl.namedBindings)) { - debugLog(v, `... but it's a wildcard import`); + debugLog(`... but it's a wildcard import`); continue; // Jump to the next import. } @@ -78,12 +77,12 @@ iSpec.name.getText() === importWhat); // import {foo} if (foundRightImport) { - debugLog(v, `"${iDecl.getFullText()}" imports ${importWhat} as we want.`); + debugLog(`"${iDecl.getFullText()}" imports ${importWhat} as we want.`); return; // Our request is already imported under the right name. } // Else, insert our symbol in the list of imports from that file. - debugLog(v, `No named imports from that file, generating new fix`); + debugLog(`No named imports from that file, generating new fix`); return { start: parsedDecl.namedBindings.elements[0].getStart(), end: parsedDecl.namedBindings.elements[0].getStart(), @@ -117,27 +116,27 @@ */ export function maybeAddNamespaceImport( source: ts.SourceFile, fromFile: string, importAs: string, - tazeComment?: string, v?: boolean): IndividualChange|undefined { + tazeComment?: string): IndividualChange|undefined { const importStatements = source.statements.filter(ts.isImportDeclaration); const hasTheRightImport = importStatements.some(iDecl => { - const parsedDecl = maybeParseImportNode(iDecl, v); + const parsedDecl = maybeParseImportNode(iDecl); if (!parsedDecl || parsedDecl.fromFile !== fromFile) { // Not an import from the right file, or couldn't understand the import. return false; } - debugLog(v, `"${iDecl.getFullText()}" is an import from the right file`); + debugLog(`"${iDecl.getFullText()}" is an import from the right file`); if (ts.isNamedImports(parsedDecl.namedBindings)) { - debugLog(v, `... but it's a named import`); + debugLog(`... but it's a named import`); return false; // irrelevant to our namespace imports } // Else, bindings is a NamespaceImport. if (parsedDecl.namedBindings.name.getText() !== importAs) { - debugLog(v, `... but not the right name, we need to reimport`); + debugLog(`... but not the right name, we need to reimport`); return false; } - debugLog(v, `... and the right name, no need to reimport`); + debugLog(`... and the right name, no need to reimport`); return true; }); @@ -162,14 +161,13 @@ * parts, undefined if the import declaration is valid but not understandable by * the checker. */ -function maybeParseImportNode(iDecl: ts.ImportDeclaration, v?: boolean): { +function maybeParseImportNode(iDecl: ts.ImportDeclaration): { namedBindings: ts.NamedImportBindings|ts.NamespaceImport, fromFile: string }|undefined { if (!iDecl.importClause) { // something like import "./file"; - debugLog( - v, `Ignoring import without imported symbol: ${iDecl.getFullText()}`); + debugLog(`Ignoring import without imported symbol: ${iDecl.getFullText()}`); return; } if (iDecl.importClause.name || !iDecl.importClause.namedBindings) { @@ -177,11 +175,11 @@ // Not much we can do with that when trying to get a hold of some symbols, // so just ignore that line (worst case, we'll suggest another import // style). - debugLog(v, `Ignoring import: ${iDecl.getFullText()}`); + debugLog(`Ignoring import: ${iDecl.getFullText()}`); return; } if (!ts.isStringLiteral(iDecl.moduleSpecifier)) { - debugLog(v, `Ignoring import whose module specifier is not literal`); + debugLog(`Ignoring import whose module specifier is not literal`); return; } return {
diff --git a/internal/tsetse/util/match_symbol.ts b/internal/tsetse/util/match_symbol.ts index 1a05c72..5df53b1 100644 --- a/internal/tsetse/util/match_symbol.ts +++ b/internal/tsetse/util/match_symbol.ts
@@ -35,23 +35,23 @@ } } - matches(n: ts.Node, tc: ts.TypeChecker, verbose?: boolean): boolean { + matches(n: ts.Node, tc: ts.TypeChecker): boolean { // Get the symbol (or the one at the other end of this alias) that we're // looking at. const s = dealias(tc.getSymbolAtLocation(n), tc); if (!s) { - debugLog(verbose, `cannot get symbol`); + debugLog(`cannot get symbol`); return false; } // The TS-provided FQN tells us the full identifier, and the origin file // in some circumstances. const fqn = tc.getFullyQualifiedName(s); - debugLog(verbose, `got FQN ${fqn}`); + debugLog(`got FQN ${fqn}`); // Name-based check if (!(fqn.endsWith('.' + this.bannedName) || fqn === this.bannedName)) { - debugLog(verbose, `FQN ${fqn} doesn't match name ${this.bannedName}`); + debugLog(`FQN ${fqn} doesn't match name ${this.bannedName}`); return false; // not a use of the symbols we want } @@ -61,7 +61,7 @@ // and bad fixes. const p = n.parent; if (p && (isDeclaration(p) || isPartOfImportStatement(p))) { - debugLog(verbose, `We don't flag symbol declarations`); + debugLog(`We don't flag symbol declarations`); return false; } @@ -74,18 +74,17 @@ // We need to trace things back, so get declarations of the symbol. const declarations = s.getDeclarations(); if (!declarations) { - debugLog(verbose, `Symbol never declared?`); + debugLog(`Symbol never declared?`); return false; } if (!declarations.some(isAmbientDeclaration) && !declarations.some(isInStockLibraries)) { - debugLog( - verbose, `Symbol neither ambient nor from the stock libraries`); + debugLog(`Symbol neither ambient nor from the stock libraries`); return false; } } - debugLog(verbose, `all clear, report finding`); + debugLog(`all clear, report finding`); return true; } } @@ -123,8 +122,7 @@ /** * @param n The PropertyAccessExpression we're looking at. */ - matches( - n: ts.PropertyAccessExpression, tc: ts.TypeChecker, verbose?: boolean) { + matches(n: ts.PropertyAccessExpression, tc: ts.TypeChecker) { return n.name.text === this.bannedProperty && this.typeMatches(tc.getTypeAtLocation(n.expression)); }
diff --git a/internal/tsetse/util/pattern_engines/pattern_engine.ts b/internal/tsetse/util/pattern_engines/pattern_engine.ts index 4d315b5..5dbbddc 100644 --- a/internal/tsetse/util/pattern_engines/pattern_engine.ts +++ b/internal/tsetse/util/pattern_engines/pattern_engine.ts
@@ -8,8 +8,7 @@ export abstract class PatternEngine<P extends PatternKind> { constructor( protected readonly config: Config<P>, - protected readonly fixer?: Fixer<MatchedNodeTypes[P]>, - protected readonly verbose?: boolean) {} + protected readonly fixer?: Fixer<MatchedNodeTypes[P]>) {} abstract register(checker: Checker): void; }
diff --git a/internal/tsetse/util/pattern_engines/property_write_engine.ts b/internal/tsetse/util/pattern_engines/property_write_engine.ts index ea8fe35..7af7ccf 100644 --- a/internal/tsetse/util/pattern_engines/property_write_engine.ts +++ b/internal/tsetse/util/pattern_engines/property_write_engine.ts
@@ -16,9 +16,8 @@ private readonly matcher: PropertyMatcher; constructor( config: Config<PatternKind.BANNED_PROPERTY_WRITE>, - fixer?: Fixer<MatchedNodeTypes[PatternKind.BANNED_PROPERTY_WRITE]>, - verbose?: boolean) { - super(config, fixer, verbose); + fixer?: Fixer<MatchedNodeTypes[PatternKind.BANNED_PROPERTY_WRITE]>) { + 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) { @@ -39,8 +38,8 @@ !isPropertyWriteExpression(n)) { return; } - debugLog(this.verbose, `inspecting ${n.getFullText().trim()}`); - if (this.matcher.matches(n.left, c.typeChecker, this.verbose)) { + debugLog(`inspecting ${n.getFullText().trim()}`); + if (this.matcher.matches(n.left, c.typeChecker)) { const fix: Fix|undefined = this.fixer ? this.fixer.getFixForFlaggedNode(n) : undefined; c.addFailureAtNode(n, this.config.errorMessage, fix);