Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring of Template Expression Parsing for Stability #31898

Open
ayazhafiz opened this issue Jul 29, 2019 · 0 comments

Comments

@ayazhafiz
Copy link
Contributor

commented Jul 29, 2019

馃殌 feature request

Relevant Package

@angular/compiler
@angular/compiler-cli

Description

The Angular template expression parser, while working well for Angular compilations, has several issues that make the AST it generates difficult to use for other tasks; notably source mapping and indexing. This is because in many cases, the expression ASTs generated by the expression parser are not representative of the raw source code the expression is defined in.

Design Doc (with examples of issues)

This issue is meant to track development of the parser to better support use cases of source mapping and indexing. An overview of a plan-of-action is available on the design doc, as are examples of the issues currently experienced with expression ASTs.

Work in this domain has already been started with at least #30181 and #31391.

Presentation of issues

Preemptive discarding of whitespace

The template parsing pipeline discards whitespace in a template before generating the Render3Ast (the combined markup and expression AST):

if (!preserveWhitespaces) {
rootNodes = html.visitAll(new WhitespaceVisitor(), rootNodes);
// run i18n meta visitor again in case we remove whitespaces, because
// that might affect generated i18n message content. During this pass
// i18n IDs generated at the first pass will be preserved, so we can mimic
// existing extraction process (ng xi18n)
rootNodes = html.visitAll(
new I18nMetaVisitor(interpolationConfig, /* keepI18nAttrs */ false), rootNodes);
}
const {nodes, errors, styleUrls, styles} = htmlAstToRender3Ast(rootNodes, bindingParser);
if (errors && errors.length > 0) {
return {errors, nodes: [], styleUrls: [], styles: []};
}

This leads to a couple issues. The primary one is that the AST is not representative of the source code unless the entire pipeline is run without preserving whitespaces. The second is codes like

<div>{{ '    '.length }}</div>

are folded and ultimately evaluated to

<div>0</div>

A proposed solution is to eventually move the whitespace removal step to after the R3Ast is already generated. This involves writing a new tree rewriter.

Overlapping spans with leadingTriviaChars

This issue has been previously recorded in #30181.
Templates parsed with leading trivia characters can wind up having overlapping spans between template HTML nodes and template expressions when whitespace discarding is disabled.

<div>  \n {{foo}}</div>
^^^^^^^^^                   recorded span of opening `div`
     ^^^^^^^^^^^^           recorded span of node text

A proposed solution is to record the absolute span of such expressions in the source code so that there is no need to add source spans to compute expression spans in a template.

Misleading AstWithSource spans

Consider a template code

<div *ngFor="let _ of all"></div>

Abbreviated for example, the AST of this template is

BoundAttribute {
    name: 'ngForOf',
    value: AstWithSource {
        source: 'all',
        span: {start: 0, end: 3},
        ast: PropertyRead {
            name: 'all',
            span: {start: 9, end: 12},
        },
    }
}

The issue here is that AstWithSource records the same source the PropertyRead does, but in a different context. It's difficult to tell the best way to resolve this:

  • Should we ignore AstWithSource and look only at the PropertyRead span?
    • This breaks cases where AstWithSource is worth looking at, e.g. in most non-template-binding ASTs
  • Should AstWithSource provide the span for all of let _ of all?
    • This is not necessarily the goal of AstWithSource and breaks many existing consumers of AstWithSource.
  • Should PropertyRead have a span relative to AstWithSource?
    • This requires updating AstWithSource's source span calculation and also breaks many existing consumers.

The least ambiguous resolution is to record the source span of AstWithSource and the PropertyRead relative to the entire source code, so that calculation via span summation is not needed at all.

Proposed Plan of Attack

These issues can be reconciled by several goals. These goals are ordered by increasing size of impact, not necessarily by linearity.

  1. Record absolute source spans and byte offsets of expressions.
    • Move to expression ASTs recording the absolute location of an expression in a template file, not relative to the HTML node they are contained in as is being done now.
    • Introduce a new AbsoluteSourceSpan to represent this.
  2. Create a new ExpressionIdentifier (name variable) class to augment the name field of an expression AST. This will contain more accurate information about an expression, including
    • Its name
    • Its raw/original form
    • Its logical/computed form
  3. Move AST whitespace folding to after the R3 AST has already been created.
    • Create a new tree rewriter that can perform this.

@ngbot ngbot bot modified the milestone: Backlog Jul 29, 2019

ayazhafiz added a commit to ayazhafiz/angular that referenced this issue Jul 29, 2019

feat(compiler): record absolute span of template expressions in parser
Currently, the spans of expressions are recorded only relative to the
template node that they reside in, not their source file.

Introduce a `sourceSpan` property on expression ASTs that records the
location of an expression relative to the entire source code file that
it is in. This may allow for reducing duplication of effort in
ngtsc/typecheck/src/diagnostics later on as well.

Child of angular#31898
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can鈥檛 perform that action at this time.