Skip to content

Commit

Permalink
fix(language-service): Always attempt HTML AST to template AST conver…
Browse files Browse the repository at this point in the history
…sion for LS (#41068)

The current logic in the compiler is to bail when there are errors when
parsing a template into an HTML AST or when there are errors in the i18n
metadata. As a result, a template with these types of parse errors
_will not have any information for the language service_. This is because we
never attempt to conver the HTML AST to a template AST in these
scenarios, so there are no template AST nodes for the language service
to look at for information. In addition, this also means that the errors
are never displayed in the template to the user because there are no
nodes to map the error to.

This commit adds an option to the template parser to temporarily ignore
the html parse and i18n meta errors and always perform the template AST
conversion. At the end, the i18n and HTML parse errors are appended to
the returned errors list. While this seems risky, it at least provides
us with more information than we had before (which was 0) and it's only
done in the context of the language service, when the compiler is
configured to use poisoned data (HTML parse and i18n meta errors can be
interpreted as a "poisoned" template).

fixes angular/vscode-ng-language-service#1140

PR Close #41068
  • Loading branch information
atscott authored and zarend committed Mar 3, 2021
1 parent f09e7ab commit 6dd5497
Show file tree
Hide file tree
Showing 6 changed files with 97 additions and 5 deletions.
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -975,6 +975,7 @@ export class ComponentDecoratorHandler implements
enableI18nLegacyMessageIdFormat: this.enableI18nLegacyMessageIdFormat,
i18nNormalizeLineEndingsInICUs,
isInline: template.isInline,
alwaysAttemptHtmlToR3AstConversion: this.usePoisonedData,
});

// Unfortunately, the primary parse of the template above may not contain accurate source map
Expand Down Expand Up @@ -1002,6 +1003,7 @@ export class ComponentDecoratorHandler implements
i18nNormalizeLineEndingsInICUs,
leadingTriviaChars: [],
isInline: template.isInline,
alwaysAttemptHtmlToR3AstConversion: this.usePoisonedData,
});

return {
Expand Down
25 changes: 21 additions & 4 deletions packages/compiler/src/render3/view/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2070,6 +2070,22 @@ export interface ParseTemplateOptions {
* Whether the template was inline.
*/
isInline?: boolean;

/**
* Whether to always attempt to convert the parsed HTML AST to an R3 AST, despite HTML or i18n
* Meta parse errors.
*
*
* This option is useful in the context of the language service, where we want to get as much
* information as possible, despite any errors in the HTML. As an example, a user may be adding
* a new tag and expecting autocomplete on that tag. In this scenario, the HTML is in an errored
* state, as there is an incomplete open tag. However, we're still able to convert the HTML AST
* nodes to R3 AST nodes in order to provide information for the language service.
*
* Note that even when `true` the HTML parse and i18n errors are still appended to the errors
* output, but this is done after converting the HTML AST to R3 AST.
*/
alwaysAttemptHtmlToR3AstConversion?: boolean;
}

/**
Expand All @@ -2089,9 +2105,8 @@ export function parseTemplate(
template, templateUrl,
{leadingTriviaChars: LEADING_TRIVIA_CHARS, ...options, tokenizeExpansionForms: true});

if (parseResult.errors && parseResult.errors.length > 0) {
// TODO(ayazhafiz): we may not always want to bail out at this point (e.g. in
// the context of a language service).
if (!options.alwaysAttemptHtmlToR3AstConversion && parseResult.errors &&
parseResult.errors.length > 0) {
return {
interpolationConfig,
preserveWhitespaces,
Expand All @@ -2117,7 +2132,8 @@ export function parseTemplate(
enableI18nLegacyMessageIdFormat);
const i18nMetaResult = i18nMetaVisitor.visitAllWithErrors(rootNodes);

if (i18nMetaResult.errors && i18nMetaResult.errors.length > 0) {
if (!options.alwaysAttemptHtmlToR3AstConversion && i18nMetaResult.errors &&
i18nMetaResult.errors.length > 0) {
return {
interpolationConfig,
preserveWhitespaces,
Expand Down Expand Up @@ -2149,6 +2165,7 @@ export function parseTemplate(

const {nodes, errors, styleUrls, styles, ngContentSelectors} =
htmlAstToRender3Ast(rootNodes, bindingParser);
errors.push(...parseResult.errors, ...i18nMetaResult.errors);

return {
interpolationConfig,
Expand Down
13 changes: 13 additions & 0 deletions packages/compiler/test/render3/r3_template_transform_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,19 @@ describe('R3 template transform', () => {
});

describe('Nodes without binding', () => {
it('should parse incomplete tags terminated by EOF', () => {
expectFromHtml('<a', true /* ignoreError */).toEqual([
['Element', 'a'],
]);
});

it('should parse incomplete tags terminated by another tag', () => {
expectFromHtml('<a <span></span>', true /* ignoreError */).toEqual([
['Element', 'a'],
['Element', 'span'],
]);
});

it('should parse text nodes', () => {
expectFromHtml('a').toEqual([
['Text', 'a'],
Expand Down
21 changes: 21 additions & 0 deletions packages/language-service/ivy/test/completions_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,23 @@ describe('completions', () => {
expect(ts.displayPartsToString(details.documentation!)).toEqual('This is another component.');
});

it('should return completions for an incomplete tag', () => {
const OTHER_CMP = {
'OtherCmp': `
/** This is another component. */
@Component({selector: 'other-cmp', template: 'unimportant'})
export class OtherCmp {}
`,
};
const {templateFile} = setup(`<other`, '', OTHER_CMP);
templateFile.moveCursorToText('<other¦');

const completions = templateFile.getCompletionsAtPosition();
expectContain(
completions, unsafeCastDisplayInfoKindToScriptElementKind(DisplayInfoKind.COMPONENT),
['other-cmp']);
});

it('should return completions with a blank open tag', () => {
const OTHER_CMP = {
'OtherCmp': `
Expand Down Expand Up @@ -397,6 +414,10 @@ describe('completions', () => {

const completions = templateFile.getCompletionsAtPosition();
expect(completions).toBeUndefined();


const details = templateFile.getCompletionEntryDetails('other-cmp')!;
expect(details).toBeUndefined();
});

describe('element attribute scope', () => {
Expand Down
30 changes: 29 additions & 1 deletion packages/language-service/ivy/test/diagnostic_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('getSemanticDiagnostics', () => {
env = LanguageServiceTestEnv.setup();
});

it('should not produce error for a minimal component defintion', () => {
it('should not produce error for a minimal component definition', () => {
const files = {
'app.ts': `
import {Component, NgModule} from '@angular/core';
Expand Down Expand Up @@ -124,6 +124,34 @@ describe('getSemanticDiagnostics', () => {
`Parser Error: Bindings cannot contain assignments at column 8 in [{{nope = true}}]`);
});

it('reports html parse errors along with typecheck errors as diagnostics', () => {
const files = {
'app.ts': `
import {Component, NgModule} from '@angular/core';
@Component({
templateUrl: './app.html'
})
export class AppComponent {
nope = false;
}
`,
'app.html': '<dne'
};

const project = createModuleAndProjectWithDeclarations(env, 'test', files);
const diags = project.getDiagnosticsForFile('app.html');
expect(diags.length).toBe(2);

expect(diags[0].category).toBe(ts.DiagnosticCategory.Error);
expect(diags[0].file?.fileName).toBe('/test/app.html');
expect(diags[0].messageText).toContain(`'dne' is not a known element`);

expect(diags[1].category).toBe(ts.DiagnosticCategory.Error);
expect(diags[1].file?.fileName).toBe('/test/app.html');
expect(diags[1].messageText).toContain(`Opening tag "dne" not terminated.`);
});

it('should report parse errors of components defined in the same ts file', () => {
const files = {
'app.ts': `
Expand Down
11 changes: 11 additions & 0 deletions packages/language-service/ivy/test/legacy/template_target_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,23 @@ function parse(template: string): ParseResult {
// `ComponentDecoratorHandler._parseTemplate`.
leadingTriviaChars: [],
preserveWhitespaces: true,
alwaysAttemptHtmlToR3AstConversion: true,
}),
position,
};
}

describe('getTargetAtPosition for template AST', () => {
it('should locate incomplete tag', () => {
const {errors, nodes, position} = parse(`<div¦`);
expect(errors?.length).toBe(1);
expect(errors![0].msg).toContain('Opening tag "div" not terminated.');
const {context} = getTargetAtPosition(nodes, position)!;
const {node} = context as SingleNodeTarget;
expect(isTemplateNode(node!)).toBe(true);
expect(node).toBeInstanceOf(t.Element);
});

it('should locate element in opening tag', () => {
const {errors, nodes, position} = parse(`<di¦v></div>`);
expect(errors).toBe(null);
Expand Down

0 comments on commit 6dd5497

Please sign in to comment.