Skip to content

Commit

Permalink
refactor(compiler): element.startSourceSpan is required (#38581)
Browse files Browse the repository at this point in the history 
Previously, the `startSourceSpan` property could be null
but in reality it is always well defined - except for a legacy
case in the old i18n extraction/merging code, where the
typings for source-spans are already being undermined.

Making this property non-null, simplifies code elsewhere
in the project.

PR Close #38581
  • Loading branch information
@petebacondarwin @atscott
petebacondarwin authored and atscott committed Sep 2, 2020
1 parent 86e11f1 commit a68f1a7
Show file tree
Hide file tree
Showing 14 changed files with 53 additions and 58 deletions.
4 changes: 2 additions & 2 deletions packages/compiler/src/i18n/extractor_merger.ts
View file
Expand Up @@ -124,7 +124,7 @@ class _Visitor implements html.Visitor {
this._translations = translations;

// Construct a single fake root element
const wrapper = new html.Element('wrapper', [], nodes, undefined!, undefined, undefined);
const wrapper = new html.Element('wrapper', [], nodes, undefined!, undefined!, undefined);

const translatedNode = wrapper.visit(this, null);

Expand Down Expand Up @@ -492,7 +492,7 @@ class _Visitor implements html.Visitor {
}

private _reportError(node: html.Node, msg: string): void {
this._errors.push(new I18nError(node.sourceSpan!, msg));
this._errors.push(new I18nError(node.sourceSpan, msg));
}
}

Expand Down
6 changes: 3 additions & 3 deletions packages/compiler/src/i18n/i18n_parser.ts
View file
Expand Up @@ -83,7 +83,7 @@ class _I18nVisitor implements html.Visitor {
const isVoid: boolean = getHtmlTagDefinition(el.name).isVoid;
const startPhName =
context.placeholderRegistry.getStartTagPlaceholderName(el.name, attrs, isVoid);
context.placeholderToContent[startPhName] = el.sourceSpan!.toString();
context.placeholderToContent[startPhName] = el.sourceSpan.toString();

let closePhName = '';

Expand All @@ -93,7 +93,7 @@ class _I18nVisitor implements html.Visitor {
}

const node = new i18n.TagPlaceholder(
el.name, attrs, startPhName, closePhName, children, isVoid, el.sourceSpan!);
el.name, attrs, startPhName, closePhName, children, isVoid, el.sourceSpan);
return context.visitNodeFn(el, node);
}

Expand All @@ -103,7 +103,7 @@ class _I18nVisitor implements html.Visitor {
}

visitText(text: html.Text, context: I18nMessageVisitorContext): i18n.Node {
const node = this._visitTextWithInterpolation(text.value, text.sourceSpan!, context);
const node = this._visitTextWithInterpolation(text.value, text.sourceSpan, context);
return context.visitNodeFn(text, node);
}

Expand Down
12 changes: 6 additions & 6 deletions packages/compiler/src/i18n/serializers/xliff.ts
View file
Expand Up @@ -231,9 +231,9 @@ class XliffParser implements ml.Visitor {
break;

case _TARGET_TAG:
const innerTextStart = element.startSourceSpan!.end.offset;
const innerTextStart = element.startSourceSpan.end.offset;
const innerTextEnd = element.endSourceSpan!.start.offset;
const content = element.startSourceSpan!.start.file.content;
const content = element.startSourceSpan.start.file.content;
const innerText = content.slice(innerTextStart, innerTextEnd);
this._unitMlString = innerText;
break;
Expand Down Expand Up @@ -264,7 +264,7 @@ class XliffParser implements ml.Visitor {
visitExpansionCase(expansionCase: ml.ExpansionCase, context: any): any {}

private _addError(node: ml.Node, message: string): void {
this._errors.push(new I18nError(node.sourceSpan!, message));
this._errors.push(new I18nError(node.sourceSpan, message));
}
}

Expand All @@ -288,14 +288,14 @@ class XmlToI18n implements ml.Visitor {
}

visitText(text: ml.Text, context: any) {
return new i18n.Text(text.value, text.sourceSpan!);
return new i18n.Text(text.value, text.sourceSpan);
}

visitElement(el: ml.Element, context: any): i18n.Placeholder|ml.Node[]|null {
if (el.name === _PLACEHOLDER_TAG) {
const nameAttr = el.attrs.find((attr) => attr.name === 'id');
if (nameAttr) {
return new i18n.Placeholder('', nameAttr.value, el.sourceSpan!);
return new i18n.Placeholder('', nameAttr.value, el.sourceSpan);
}

this._addError(el, `<${_PLACEHOLDER_TAG}> misses the "id" attribute`);
Expand Down Expand Up @@ -332,7 +332,7 @@ class XmlToI18n implements ml.Visitor {
visitAttribute(attribute: ml.Attribute, context: any) {}

private _addError(node: ml.Node, message: string): void {
this._errors.push(new I18nError(node.sourceSpan!, message));
this._errors.push(new I18nError(node.sourceSpan, message));
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/compiler/src/i18n/serializers/xliff2.ts
View file
Expand Up @@ -246,9 +246,9 @@ class Xliff2Parser implements ml.Visitor {
break;

case _TARGET_TAG:
const innerTextStart = element.startSourceSpan!.end.offset;
const innerTextStart = element.startSourceSpan.end.offset;
const innerTextEnd = element.endSourceSpan!.start.offset;
const content = element.startSourceSpan!.start.file.content;
const content = element.startSourceSpan.start.file.content;
const innerText = content.slice(innerTextStart, innerTextEnd);
this._unitMlString = innerText;
break;
Expand Down
12 changes: 6 additions & 6 deletions packages/compiler/src/i18n/serializers/xtb.ts
View file
Expand Up @@ -130,9 +130,9 @@ class XtbParser implements ml.Visitor {
if (this._msgIdToHtml.hasOwnProperty(id)) {
this._addError(element, `Duplicated translations for msg ${id}`);
} else {
const innerTextStart = element.startSourceSpan!.end.offset;
const innerTextStart = element.startSourceSpan.end.offset;
const innerTextEnd = element.endSourceSpan!.start.offset;
const content = element.startSourceSpan!.start.file.content;
const content = element.startSourceSpan.start.file.content;
const innerText = content.slice(innerTextStart!, innerTextEnd!);
this._msgIdToHtml[id] = innerText;
}
Expand All @@ -155,7 +155,7 @@ class XtbParser implements ml.Visitor {
visitExpansionCase(expansionCase: ml.ExpansionCase, context: any): any {}

private _addError(node: ml.Node, message: string): void {
this._errors.push(new I18nError(node.sourceSpan!, message));
this._errors.push(new I18nError(node.sourceSpan, message));
}
}

Expand All @@ -179,7 +179,7 @@ class XmlToI18n implements ml.Visitor {
}

visitText(text: ml.Text, context: any) {
return new i18n.Text(text.value, text.sourceSpan!);
return new i18n.Text(text.value, text.sourceSpan);
}

visitExpansion(icu: ml.Expansion, context: any) {
Expand All @@ -203,7 +203,7 @@ class XmlToI18n implements ml.Visitor {
if (el.name === _PLACEHOLDER_TAG) {
const nameAttr = el.attrs.find((attr) => attr.name === 'name');
if (nameAttr) {
return new i18n.Placeholder('', nameAttr.value, el.sourceSpan!);
return new i18n.Placeholder('', nameAttr.value, el.sourceSpan);
}

this._addError(el, `<${_PLACEHOLDER_TAG}> misses the "name" attribute`);
Expand All @@ -218,6 +218,6 @@ class XmlToI18n implements ml.Visitor {
visitAttribute(attribute: ml.Attribute, context: any) {}

private _addError(node: ml.Node, message: string): void {
this._errors.push(new I18nError(node.sourceSpan!, message));
this._errors.push(new I18nError(node.sourceSpan, message));
}
}
2 changes: 1 addition & 1 deletion packages/compiler/src/ml_parser/ast.ts
View file
Expand Up @@ -64,7 +64,7 @@ export class Attribute extends NodeWithI18n {
export class Element extends NodeWithI18n {
constructor(
public name: string, public attrs: Attribute[], public children: Node[],
sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan|null = null,
sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
public endSourceSpan: ParseSourceSpan|null = null, i18n?: I18nMeta) {
super(sourceSpan, i18n);
}
Expand Down
11 changes: 3 additions & 8 deletions packages/compiler/src/render3/r3_ast.ts
View file
Expand Up @@ -79,13 +79,8 @@ export class Element implements Node {
constructor(
public name: string, public attributes: TextAttribute[], public inputs: BoundAttribute[],
public outputs: BoundEvent[], public children: Node[], public references: Reference[],
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan|null,
public endSourceSpan: ParseSourceSpan|null, public i18n?: I18nMeta) {
// If the element is empty then the source span should include any closing tag
if (children.length === 0 && startSourceSpan && endSourceSpan) {
this.sourceSpan = new ParseSourceSpan(sourceSpan.start, endSourceSpan.end);
}
}
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
public endSourceSpan: ParseSourceSpan|null, public i18n?: I18nMeta) {}
visit<Result>(visitor: Visitor<Result>): Result {
return visitor.visitElement(this);
}
Expand All @@ -96,7 +91,7 @@ export class Template implements Node {
public tagName: string, public attributes: TextAttribute[], public inputs: BoundAttribute[],
public outputs: BoundEvent[], public templateAttrs: (BoundAttribute|TextAttribute)[],
public children: Node[], public references: Reference[], public variables: Variable[],
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan|null,
public sourceSpan: ParseSourceSpan, public startSourceSpan: ParseSourceSpan,
public endSourceSpan: ParseSourceSpan|null, public i18n?: I18nMeta) {}
visit<Result>(visitor: Visitor<Result>): Result {
return visitor.visitTemplate(this);
Expand Down
32 changes: 16 additions & 16 deletions packages/compiler/src/template_parser/template_parser.ts
View file
Expand Up @@ -244,9 +244,9 @@ class TemplateParseVisitor implements html.Visitor {
visitText(text: html.Text, parent: ElementContext): any {
const ngContentIndex = parent.findNgContentIndex(TEXT_CSS_SELECTOR())!;
const valueNoNgsp = replaceNgsp(text.value);
const expr = this._bindingParser.parseInterpolation(valueNoNgsp, text.sourceSpan!);
return expr ? new t.BoundTextAst(expr, ngContentIndex, text.sourceSpan!) :
new t.TextAst(valueNoNgsp, ngContentIndex, text.sourceSpan!);
const expr = this._bindingParser.parseInterpolation(valueNoNgsp, text.sourceSpan);
return expr ? new t.BoundTextAst(expr, ngContentIndex, text.sourceSpan) :
new t.TextAst(valueNoNgsp, ngContentIndex, text.sourceSpan);
}

visitAttribute(attribute: html.Attribute, context: any): any {
Expand Down Expand Up @@ -335,14 +335,14 @@ class TemplateParseVisitor implements html.Visitor {
const boundDirectivePropNames = new Set<string>();
const directiveAsts = this._createDirectiveAsts(
isTemplateElement, element.name, directiveMetas, elementOrDirectiveProps,
elementOrDirectiveRefs, element.sourceSpan!, references, boundDirectivePropNames);
elementOrDirectiveRefs, element.sourceSpan, references, boundDirectivePropNames);
const elementProps: t.BoundElementPropertyAst[] = this._createElementPropertyAsts(
element.name, elementOrDirectiveProps, boundDirectivePropNames);
const isViewRoot = parent.isTemplateElement || hasInlineTemplates;

const providerContext = new ProviderElementContext(
this.providerViewContext, parent.providerContext!, isViewRoot, directiveAsts, attrs,
references, isTemplateElement, queryStartIndex, element.sourceSpan!);
references, isTemplateElement, queryStartIndex, element.sourceSpan);

const children: t.TemplateAst[] = html.visitAll(
preparsedElement.nonBindable ? NON_BINDABLE_VISITOR : this, element.children,
Expand All @@ -360,26 +360,26 @@ class TemplateParseVisitor implements html.Visitor {
if (preparsedElement.type === PreparsedElementType.NG_CONTENT) {
// `<ng-content>` element
if (element.children && !element.children.every(_isEmptyTextNode)) {
this._reportError(`<ng-content> element cannot have content.`, element.sourceSpan!);
this._reportError(`<ng-content> element cannot have content.`, element.sourceSpan);
}

parsedElement = new t.NgContentAst(
this.ngContentCount++, hasInlineTemplates ? null! : ngContentIndex, element.sourceSpan!);
this.ngContentCount++, hasInlineTemplates ? null! : ngContentIndex, element.sourceSpan);
} else if (isTemplateElement) {
// `<ng-template>` element
this._assertAllEventsPublishedByDirectives(directiveAsts, events);
this._assertNoComponentsNorElementBindingsOnTemplate(
directiveAsts, elementProps, element.sourceSpan!);
directiveAsts, elementProps, element.sourceSpan);

parsedElement = new t.EmbeddedTemplateAst(
attrs, events, references, elementVars, providerContext.transformedDirectiveAsts,
providerContext.transformProviders, providerContext.transformedHasViewContainer,
providerContext.queryMatches, children, hasInlineTemplates ? null! : ngContentIndex,
element.sourceSpan!);
element.sourceSpan);
} else {
// element other than `<ng-content>` and `<ng-template>`
this._assertElementExists(matchElement, element);
this._assertOnlyOneComponent(directiveAsts, element.sourceSpan!);
this._assertOnlyOneComponent(directiveAsts, element.sourceSpan);

const ngContentIndex =
hasInlineTemplates ? null : parent.findNgContentIndex(projectionSelector);
Expand All @@ -397,22 +397,22 @@ class TemplateParseVisitor implements html.Visitor {
const {directives} = this._parseDirectives(this.selectorMatcher, templateSelector);
const templateBoundDirectivePropNames = new Set<string>();
const templateDirectiveAsts = this._createDirectiveAsts(
true, elName, directives, templateElementOrDirectiveProps, [], element.sourceSpan!, [],
true, elName, directives, templateElementOrDirectiveProps, [], element.sourceSpan, [],
templateBoundDirectivePropNames);
const templateElementProps: t.BoundElementPropertyAst[] = this._createElementPropertyAsts(
elName, templateElementOrDirectiveProps, templateBoundDirectivePropNames);
this._assertNoComponentsNorElementBindingsOnTemplate(
templateDirectiveAsts, templateElementProps, element.sourceSpan!);
templateDirectiveAsts, templateElementProps, element.sourceSpan);
const templateProviderContext = new ProviderElementContext(
this.providerViewContext, parent.providerContext!, parent.isTemplateElement,
templateDirectiveAsts, [], [], true, templateQueryStartIndex, element.sourceSpan!);
templateDirectiveAsts, [], [], true, templateQueryStartIndex, element.sourceSpan);
templateProviderContext.afterElement();

parsedElement = new t.EmbeddedTemplateAst(
[], [], [], templateElementVars, templateProviderContext.transformedDirectiveAsts,
templateProviderContext.transformProviders,
templateProviderContext.transformedHasViewContainer, templateProviderContext.queryMatches,
[parsedElement], ngContentIndex, element.sourceSpan!);
[parsedElement], ngContentIndex, element.sourceSpan);
}

return parsedElement;
Expand Down Expand Up @@ -707,7 +707,7 @@ class TemplateParseVisitor implements html.Visitor {
errorMsg +=
`2. To allow any element add 'NO_ERRORS_SCHEMA' to the '@NgModule.schemas' of this component.`;
}
this._reportError(errorMsg, element.sourceSpan!);
this._reportError(errorMsg, element.sourceSpan);
}
}

Expand Down Expand Up @@ -815,7 +815,7 @@ class NonBindableVisitor implements html.Visitor {

visitText(text: html.Text, parent: ElementContext): t.TextAst {
const ngContentIndex = parent.findNgContentIndex(TEXT_CSS_SELECTOR())!;
return new t.TextAst(text.value, ngContentIndex, text.sourceSpan!);
return new t.TextAst(text.value, ngContentIndex, text.sourceSpan);
}

visitExpansion(expansion: html.Expansion, context: any): any {
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler/test/ml_parser/ast_spec_utils.ts
View file
Expand Up @@ -42,7 +42,7 @@ class _Humanizer implements html.Visitor {
visitElement(element: html.Element, context: any): any {
const res = this._appendContext(element, [html.Element, element.name, this.elDepth++]);
if (this.includeSourceSpan) {
res.push(element.startSourceSpan?.toString() ?? null);
res.push(element.startSourceSpan.toString() ?? null);
res.push(element.endSourceSpan?.toString() ?? null);
}
this.result.push(res);
Expand Down Expand Up @@ -82,7 +82,7 @@ class _Humanizer implements html.Visitor {

private _appendContext(ast: html.Node, input: any[]): any[] {
if (!this.includeSourceSpan) return input;
input.push(ast.sourceSpan!.toString());
input.push(ast.sourceSpan.toString());
return input;
}
}
4 changes: 2 additions & 2 deletions packages/compiler/test/ml_parser/html_parser_spec.ts
View file
Expand Up @@ -667,8 +667,8 @@ import {humanizeDom, humanizeDomSourceSpans, humanizeLineColumn} from './ast_spe
it('should set the start and end source spans', () => {
const node = <html.Element>parser.parse('<div>a</div>', 'TestComp').rootNodes[0];

expect(node.startSourceSpan!.start.offset).toEqual(0);
expect(node.startSourceSpan!.end.offset).toEqual(5);
expect(node.startSourceSpan.start.offset).toEqual(0);
expect(node.startSourceSpan.end.offset).toEqual(5);

expect(node.endSourceSpan!.start.offset).toEqual(6);
expect(node.endSourceSpan!.end.offset).toEqual(12);
Expand Down
14 changes: 7 additions & 7 deletions packages/compiler/test/ml_parser/icu_ast_expander_spec.ts
View file
Expand Up @@ -56,10 +56,10 @@ import {humanizeNodes} from './ast_spec_utils';
const nodes = expand(`{messages.length, plural,=0 {<b>bold</b>}}`).nodes;

const container: html.Element = <html.Element>nodes[0];
expect(container.sourceSpan!.start.col).toEqual(0);
expect(container.sourceSpan!.end.col).toEqual(42);
expect(container.startSourceSpan!.start.col).toEqual(0);
expect(container.startSourceSpan!.end.col).toEqual(42);
expect(container.sourceSpan.start.col).toEqual(0);
expect(container.sourceSpan.end.col).toEqual(42);
expect(container.startSourceSpan.start.col).toEqual(0);
expect(container.startSourceSpan.end.col).toEqual(42);
expect(container.endSourceSpan!.start.col).toEqual(0);
expect(container.endSourceSpan!.end.col).toEqual(42);

Expand All @@ -68,15 +68,15 @@ import {humanizeNodes} from './ast_spec_utils';
expect(switchExp.sourceSpan.end.col).toEqual(16);

const template: html.Element = <html.Element>container.children[0];
expect(template.sourceSpan!.start.col).toEqual(25);
expect(template.sourceSpan!.end.col).toEqual(41);
expect(template.sourceSpan.start.col).toEqual(25);
expect(template.sourceSpan.end.col).toEqual(41);

const switchCheck = template.attrs[0];
expect(switchCheck.sourceSpan.start.col).toEqual(25);
expect(switchCheck.sourceSpan.end.col).toEqual(28);

const b: html.Element = <html.Element>template.children[0];
expect(b.sourceSpan!.start.col).toEqual(29);
expect(b.sourceSpan.start.col).toEqual(29);
expect(b.endSourceSpan!.end.col).toEqual(40);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/test/template_parser/template_parser_spec.ts
View file
Expand Up @@ -140,7 +140,7 @@ class TemplateHumanizer implements TemplateAstVisitor {

private _appendSourceSpan(ast: TemplateAst, input: any[]): any[] {
if (!this.includeSourceSpan) return input;
input.push(ast.sourceSpan!.toString());
input.push(ast.sourceSpan.toString());
return input;
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/language-service/src/completions.ts
View file
Expand Up @@ -65,7 +65,7 @@ function getBoundedWordSpan(
// The HTML tag may include `-` (e.g. `app-root`),
// so use the HtmlAst to get the span before ayazhafiz refactor the code.
return {
start: templateInfo.template.span.start + ast.startSourceSpan!.start.offset + 1,
start: templateInfo.template.span.start + ast.startSourceSpan.start.offset + 1,
length: ast.name.length
};
}
Expand Down
2 changes: 1 addition & 1 deletion ...calize/src/tools/src/translate/translation_files/translation_parsers/translation_utils.ts
View file
Expand Up @@ -48,7 +48,7 @@ export function parseInnerRange(element: Element): ParseTreeResult {
* @param element The element whose inner range we want to compute.
*/
function getInnerRange(element: Element): LexerRange {
const start = element.startSourceSpan!.end;
const start = element.startSourceSpan.end;
const end = element.endSourceSpan!.start;
return {
startPos: start.offset,
Expand Down

0 comments on commit a68f1a7

Please sign in to comment.