refactor(schematics): external resource nodes have incorrect offsets (#13289)

* Since TSLint is technically not supporting failures for source files which don't belong to the given rule walker, we need to create the `RuleFailure` objects ourself. Currently this results in using an inconsistent and unclear way of adding failures. With this change, the `ComponentWalker` includes utility functions which allow adding external rule failures consistently and easy.
* Fixes that the external resource `ts.SourceFile`'s have incorrect offsets for `getEnd()`, `.end`. This might cause unexpected results in the future.
* Fixes that `externalResource.getText()` is returning content that does not match with the original source file content.
This commit is contained in:
Paul Gschwendtner
2018-09-24 20:11:37 +02:00
committed by Jeremy Elbourn
parent f418f78978
commit 21bc9b1c2d
19 changed files with 177 additions and 160 deletions
@@ -36,7 +36,7 @@ class Walker extends RuleWalker {
return;
}
const literalText = literal.getFullText();
const literalText = literal.getText();
this.data.forEach(selector => {
findAllSubstringIndices(literalText, selector.replace)
@@ -13,10 +13,6 @@ import {attributeSelectors} from '../../material/data/attribute-selectors';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {findAllSubstringIndices} from '../../typescript/literal';
/**
@@ -41,15 +37,15 @@ export class Walker extends ComponentWalker {
}
visitInlineStylesheet(literal: ts.StringLiteralLike) {
this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
this._createReplacementsForContent(literal, literal.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalStylesheet(node: ExternalResource) {
this._createReplacementsForContent(node, node.getFullText())
.map(data => createExternalReplacementFailure(node, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -13,10 +13,6 @@ import {attributeSelectors} from '../../material/data/attribute-selectors';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {findAllSubstringIndices} from '../../typescript/literal';
/**
@@ -34,16 +30,16 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], attributeSelectors);
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineTemplate(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalTemplate(template: ExternalResource) {
this._createReplacementsForContent(template, template.getFullText())
.map(data => createExternalReplacementFailure(template, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
visitExternalTemplate(node: ExternalResource) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -26,15 +26,15 @@ export class Rule extends Rules.AbstractRule {
export class Walker extends ComponentWalker {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createFailuresForContent(template, template.getText())
.forEach(data => this.addFailureFromStartToEnd(data.start, data.end, data.message));
visitInlineTemplate(node: ts.StringLiteralLike) {
this._createFailuresForContent(node, node.getText()).forEach(data => {
this.addFailureFromStartToEnd(data.start, data.end, data.message);
});
}
visitExternalTemplate(template: ExternalResource) {
this._createFailuresForContent(template, template.getFullText()).forEach(data => {
this.addFailure(new RuleFailure(
template, data.start, data.end, data.message, this.getRuleName()));
visitExternalTemplate(node: ExternalResource) {
this._createFailuresForContent(node, node.getText()).forEach(data => {
this.addExternalFailureFromStartToEnd(node, data.start, data.end, data.message);
});
}
@@ -33,7 +33,7 @@ export class Walker extends RuleWalker {
return;
}
const textContent = node.getFullText();
const textContent = node.getText();
this.data.forEach(data => {
if (data.whitelist && !data.whitelist.strings) {
@@ -13,10 +13,6 @@ import {cssSelectors} from '../../material/data/css-selectors';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {findAllSubstringIndices} from '../../typescript/literal';
/**
@@ -39,16 +35,16 @@ export class Walker extends ComponentWalker {
this._reportExtraStylesheetFiles(options.ruleArguments[1]);
}
visitInlineStylesheet(literal: ts.StringLiteralLike) {
this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineStylesheet(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalStylesheet(node: ExternalResource) {
this._createReplacementsForContent(node, node.getFullText())
.map(data => createExternalReplacementFailure(node, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -13,10 +13,6 @@ import {cssSelectors} from '../../material/data/css-selectors';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {findAllSubstringIndices} from '../../typescript/literal';
/**
@@ -34,16 +30,16 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], cssSelectors);
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineTemplate(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalTemplate(template: ExternalResource) {
this._createReplacementsForContent(template, template.getFullText())
.map(data => createExternalReplacementFailure(template, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
visitExternalTemplate(node: ExternalResource) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -33,7 +33,7 @@ export class Walker extends RuleWalker {
return;
}
const textContent = node.getFullText();
const textContent = node.getText();
this.data.forEach(selector => {
findAllSubstringIndices(textContent, selector.replace)
@@ -13,10 +13,6 @@ import {elementSelectors} from '../../material/data/element-selectors';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {findAllSubstringIndices} from '../../typescript/literal';
/**
@@ -39,16 +35,16 @@ export class Walker extends ComponentWalker {
this._reportExtraStylesheetFiles(options.ruleArguments[1]);
}
visitInlineStylesheet(literal: ts.StringLiteralLike) {
this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineStylesheet(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalStylesheet(node: ExternalResource) {
this._createReplacementsForContent(node, node.getFullText())
.map(data => createExternalReplacementFailure(node, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -13,10 +13,6 @@ import {elementSelectors} from '../../material/data/element-selectors';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {findAllSubstringIndices} from '../../typescript/literal';
/**
@@ -34,16 +30,16 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], elementSelectors);
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineTemplate(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalTemplate(template: ExternalResource) {
this._createReplacementsForContent(template, template.getFullText())
.map(data => createExternalReplacementFailure(template, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
visitExternalTemplate(node: ExternalResource) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -13,10 +13,6 @@ import {inputNames} from '../../material/data/input-names';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {findAllSubstringIndices} from '../../typescript/literal';
/**
@@ -44,16 +40,16 @@ export class Walker extends ComponentWalker {
this._reportExtraStylesheetFiles(options.ruleArguments[1]);
}
visitInlineStylesheet(literal: ts.StringLiteralLike) {
this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineStylesheet(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalStylesheet(node: ExternalResource) {
this._createReplacementsForContent(node, node.getFullText())
.map(data => createExternalReplacementFailure(node, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -14,10 +14,6 @@ import {inputNames} from '../../material/data/input-names';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
/**
* Rule that walks through every inline or external HTML template and switches changed input
@@ -34,16 +30,16 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], inputNames);
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineTemplate(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalTemplate(template: ExternalResource) {
this._createReplacementsForContent(template, template.getFullText())
.map(data => createExternalReplacementFailure(template, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
visitExternalTemplate(node: ExternalResource) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -10,10 +10,6 @@ import {Replacement, RuleFailure, Rules} from 'tslint';
import * as ts from 'typescript';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
import {
convertSpeedFactorToDuration,
createSpeedFactorConvertExpression,
@@ -45,16 +41,16 @@ export class Rule extends Rules.AbstractRule {
export class Walker extends ComponentWalker {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineTemplate(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalTemplate(template: ExternalResource) {
this._createReplacementsForContent(template, template.getFullText())
.map(data => createExternalReplacementFailure(template, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
visitExternalTemplate(node: ExternalResource) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
private _createReplacementsForContent(node: ts.Node, templateText: string) {
@@ -14,10 +14,6 @@ import {outputNames} from '../../material/data/output-names';
import {getChangesForTarget} from '../../material/transform-change-data';
import {ExternalResource} from '../../tslint/component-file';
import {ComponentWalker} from '../../tslint/component-walker';
import {
addFailureAtReplacement,
createExternalReplacementFailure,
} from '../../tslint/rule-failures';
/**
* Rule that walks through every inline or external HTML template and switches changed output
@@ -34,16 +30,16 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], outputNames);
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
visitInlineTemplate(node: ts.StringLiteralLike) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addFailureAtReplacement(data.failureMessage, data.replacement);
});
}
visitExternalTemplate(template: ExternalResource) {
this._createReplacementsForContent(template, template.getFullText())
.map(data => createExternalReplacementFailure(template, data.failureMessage,
this.getRuleName(), data.replacement))
.forEach(failure => this.addFailure(failure));
visitExternalTemplate(node: ExternalResource) {
this._createReplacementsForContent(node, node.getText()).forEach(data => {
this.addExternalFailureAtReplacement(node, data.failureMessage, data.replacement);
});
}
/**
@@ -16,12 +16,20 @@ export type ExternalResource = ts.SourceFile;
*/
export function createComponentFile(filePath: string, content: string): ExternalResource {
const sourceFile = ts.createSourceFile(filePath, `\`${content}\``, ts.ScriptTarget.ES5);
const _getFullText = sourceFile.getFullText;
// Subtract two characters because the string literal quotes are only needed for parsing
// and are not part of the actual source file.
sourceFile.end = sourceFile.end - 2;
// Note: This does not affect the way TSLint applies replacements for external resource files.
// At the time of writing, TSLint loads files manually if the actual rule source file is not
// equal to the source file of the replacement. This means that the replacements need proper
// offsets without the string literal quote symbols.
sourceFile.getFullText = function() {
const text = _getFullText.apply(sourceFile);
return text.substring(1, text.length - 1);
}.bind(sourceFile);
return sourceFile.text.substring(1, sourceFile.text.length - 1);
};
sourceFile.getText = sourceFile.getFullText;
return sourceFile;
}
@@ -125,7 +125,7 @@ describe('ComponentWalker', () => {
const stylesheetPath = './shared-styles/global.css';
spyOn(walker, 'visitExternalStylesheet').and.callFake(node => {
expect(node.getFullText()).toBe('external');
expect(node.getText()).toBe('external');
});
mockFs({[stylesheetPath]: 'external'});
@@ -146,7 +146,7 @@ describe('ComponentWalker', () => {
const stylePath = join(dirname(sourceFile.fileName), 'my-component.css');
spyOn(walker, 'visitExternalStylesheet').and.callFake(node => {
expect(node.getFullText()).toBe('external');
expect(node.getText()).toBe('external');
});
mockFs({[stylePath]: 'external'});
@@ -156,6 +156,37 @@ describe('ComponentWalker', () => {
expect(walker.visitExternalStylesheet).toHaveBeenCalledTimes(1);
});
it('should create external source files with proper offsets', () => {
const sourceFile = createSourceFile(`
@Component({
styleUrls: ['./my-component.css']
})
export class MyComponent {}
`);
const walker = new ComponentWalker(sourceFile, defaultRuleOptions);
const stylePath = join(dirname(sourceFile.fileName), 'my-component.css');
const styleContent = 'external stylesheet';
// Since the component walker is an enhanced TSLint rule walker and needs to report
// the external styles and templates as a TypeScript source file in order to apply
// replacements, the offsets of the fake TypeScript source file should not differentiate
// from the original source file. Meaning that the string literal quotes, which are needed to
// parse the resource file content as TypeScript node, should not invalidate the `width` and
// `end` offsets of the source file.
spyOn(walker, 'visitExternalStylesheet').and.callFake(node => {
expect(node.getStart()).toBe(0);
expect(node.getEnd()).toBe(styleContent.length);
expect(node.getWidth()).toBe(styleContent.length);
expect(node.getFullWidth()).toBe(styleContent.length);
});
mockFs({[stylePath]: styleContent});
walker.walk(sourceFile);
expect(walker.visitExternalStylesheet).toHaveBeenCalledTimes(1);
});
});
/** TypeScript source file content that includes a component with inline styles. */
@@ -8,15 +8,15 @@
import {existsSync, readFileSync} from 'fs';
import {dirname, resolve} from 'path';
import {RuleWalker} from 'tslint';
import * as ts from 'typescript';
import {createComponentFile, ExternalResource} from './component-file';
import {ExternalFailureWalker} from './external-failure-walker';
/**
* Custom TSLint rule walker that identifies Angular components and visits specific parts of
* the component metadata.
*/
export class ComponentWalker extends RuleWalker {
export class ComponentWalker extends ExternalFailureWalker {
visitInlineTemplate(_template: ts.StringLiteralLike) {}
visitInlineStylesheet(_stylesheet: ts.StringLiteralLike) {}
@@ -0,0 +1,45 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {Fix, Replacement, RuleFailure, RuleWalker} from 'tslint';
import {ExternalResource} from './component-file';
/**
* Enhanced TSLint rule walker that makes it easier to create rule failures that don't belong to
* the source file that has been passed to the rule walker.
*/
export class ExternalFailureWalker extends RuleWalker {
/** Adds a failure for the external resource at the specified position with the given width. */
addExternalFailureAt(node: ExternalResource, start: number, width: number, message: string,
fix?: Fix) {
this.addFailure(new RuleFailure(node, start, start + width, message, this.getRuleName(), fix));
}
/** Adds a failure at the specified range for the external resource. */
addExternalFailureFromStartToEnd(node: ExternalResource, start: number, end: number,
message: string, fix?: Fix) {
this.addFailure(new RuleFailure(node, start, end, message, this.getRuleName(), fix));
}
/** Adds a failure for the whole external resource node. */
addExternalFailure(node: ExternalResource, message: string, fix?: Fix) {
this.addExternalFailureAt(node, node.getStart(), node.getFullWidth(), message, fix);
}
/** Adds a failure to the external resource at the location of the specified replacement. */
addExternalFailureAtReplacement(node: ExternalResource, message: string,
replacement: Replacement) {
this.addExternalFailureAt(node, replacement.start, replacement.end, message, replacement);
}
/** Adds a failure at the location of the specified replacement. */
addFailureAtReplacement(message: string, replacement: Replacement) {
this.addFailureAt(replacement.start, replacement.end, message, replacement);
}
}
@@ -1,27 +0,0 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {Replacement, RuleFailure, RuleWalker} from 'tslint';
import {ExternalResource} from './component-file';
/**
* Creates a standalone rule failure that can be used to apply a replacement for an
* external resource from the custom ComponentWalker.
*/
export function createExternalReplacementFailure(node: ExternalResource, failureMessage: string,
ruleName: string, replacement: Replacement) {
return new RuleFailure(node, replacement.start, replacement.end, failureMessage,
ruleName, replacement);
}
/** Adds a failure to the given walker at the location of the specified replacement. */
export function addFailureAtReplacement(walker: RuleWalker, failureMessage: string,
replacement: Replacement) {
walker.addFailureAt(replacement.start, replacement.end, failureMessage, replacement);
}