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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics #31952

Closed
wants to merge 2 commits into from

Conversation

@alxhub
Copy link
Contributor

commented Aug 2, 2019

Historically, the Angular Compiler has produced both native TypeScript
diagnostics (called ts.Diagnostics) and its own internal Diagnostic format
(called an api.Diagnostic). This was done because TypeScript ts.Diagnostics
cannot be produced for files not in the ts.Program, and template type-
checking diagnostics are naturally produced for external .html template
files.

This design isn't optimal for several reasons:

  1. Downstream tooling (such as the CLI) must support multiple formats of
    diagnostics, adding to the maintenance burden.

  2. ts.Diagnostics have gotten a lot better in recent releases, with support
    for suggested changes, highlighting of the code in question, etc. None of
    these changes have been of any benefit for api.Diagnostics, which have
    continued to be reported in a very primitive fashion.

  3. A future plugin model will not support anything but ts.Diagnostics, so
    generating api.Diagnostics is a blocker for ngtsc-as-a-plugin.

  4. The split complicates both the typings and the testing of ngtsc.

To fix this issue, this commit changes template type-checking to produce
ts.Diagnostics instead. Instead of reporting a special kind of diagnostic
for external template files, errors in a template are always reported in
a ts.Diagnostic anchored on the template or templateUrl expression. If
necessary, the position of the error in the template is included in the
messageText of the ts.Diagnostic.

A template error can thus be reported in 3 separate ways, depending on how
the template was configured:

  1. For inline template strings which can be directly mapped to offsets in
    the TS code, ts.Diagnostics point to real ranges in the source.

This is the case if an inline template is used with a string literal or a
"no-substitution" string. For example:

@Component({..., template: `
<p>Bar: {{baz}}</p>
`})
export class TestCmp {
  bar: string;
}

The above template contains an error (no 'baz' property of TestCmp). The
error produced by TS will look like:

<p>Bar: {{baz}}</p>
          ~~~

Property 'baz' does not exist on type 'TestCmp'. Did you mean 'bar'?
  1. For template strings which cannot be directly mapped to offsets in the
    TS code, a logical offset into the template string will be included in
    the error message. For example:
const SOME_TEMPLATE = '<p>Bar: {{baz}}</p>';

@Component({..., template: SOME_TEMPLATE})
export class TestCmp {
  bar: string;
}

Because the template is a reference to another variable and is not an
inline string constant, the compiler will not be able to use "absolute"
positions when parsing the template. As a result, errors will report logical
offsets into the template string:

@Component({..., template: SOME_TEMPLATE})
                           ~~~~~~~~~~~~~

(1, 11): Property 'baz' does not exist on type 'TestCmp'. Did you mean
'bar'?

This error message states that the error corresponds to line 1, column 11 of
the template.

  1. For external templates (templateUrl), the error message is anchored on
    the templateUrl expression and contains the filename as well as the position
    information. For example, if the above example instead had its template in
    ./testcmp.html, the error would look like:
@Component({..., templateUrl: './testcmp.html'})
                              ~~~~~~~~~~~~~~~~

./testcmp.html(1, 11): Property 'baz' does not exist on type 'TestCmp'. Did
you mean 'bar'?

@alxhub alxhub requested a review from angular/fw-compiler as a code owner Aug 2, 2019

@googlebot googlebot added the cla: yes label Aug 2, 2019

@@ -208,6 +208,7 @@ function reportErrorsAndExit(
if (options && options.enableIvy === true) {
const ngDiagnostics = errorsAndWarnings.filter(api.isNgDiagnostic);
const tsDiagnostics = errorsAndWarnings.filter(api.isTsDiagnostic);
debugger;

This comment has been minimized.

Copy link
@ElianCordoba
@@ -177,7 +181,7 @@ export class TypeCheckContext {
calculateTemplateDiagnostics(
originalProgram: ts.Program, originalHost: ts.CompilerHost,
originalOptions: ts.CompilerOptions): {
diagnostics: Diagnostic[],
diagnostics: ts.Diagnostic[],

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 2, 2019

Member

We should be able to remove Diagnostic and friends from ./diagnostics.ts in its entirety.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 2, 2019

Author Contributor

👍

},
}

const diagnostics: ts.Diagnostic[] = [];

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 2, 2019

Member

clang-format goes haywire here 💥 due to the missing semicolon above (which is also causing the lint job to fail)

@@ -35,7 +35,7 @@ runInEachFileSystem(() => {
}]);

expect(messages).toEqual(
[`synthetic.html(9, 30): Type 'string' is not assignable to type 'number | undefined'.`]);
[`synthetic.html(1, 10): Type 'string' is not assignable to type 'number | undefined'.`]);

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 2, 2019

Member

All the column offsets look to be off-by-one. I believe the offsets in the original tests were correct, whereas they have now all been incremented by one. Was this intentional (and therefore my interpretation of these offsets incorrect)?

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 2, 2019

Author Contributor

The offsets given previously were 0-indexed. These are 1-indexed, so the origin of the template is (1, 1).

There is something weird going on though which I think is resulting in off-by-one errors for direct template mappings on lines after the first. I need to dig into it.

@ngbot ngbot bot added this to the needsTriage milestone Aug 8, 2019

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/true-diagnostics branch 2 times, most recently from c266a31 to a7894f6 Aug 15, 2019

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/true-diagnostics branch 2 times, most recently from 0f4f6d6 to 262bb95 Aug 16, 2019

program: ts.Program,
} {
const typeCheckSf = this.typeCheckFile.render();
require('fs').writeFileSync('/tmp/tc.ts', typeCheckSf.text);

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 18, 2019

Member

Remove

// TODO(alxhub): investigate creating a fake `ts.SourceFile` here instead of invoking the TS
// parser against the template (HTML is just really syntactically invalid TypeScript code ;).
const sf = ts.createSourceFile(
fileName, mapping.template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX);

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 18, 2019

Member

I love how you choose ts.ScriptKind.JSX, to ease the pain of the code not being TS at all.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 20, 2019

Author Contributor

😅

messageText: `Error occurs in the template of component ${componentName}.`,
}],
};
} else {

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 18, 2019

Member

Typically the leftover else is not necessary with union types, is there some limitation of g3 at play here? Currently, adding a new type to TemplateSourceMapping will fail to yield a compile error here, whereas it would error if this else wasn't here.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 20, 2019

Author Contributor

Yeah, I think it's needed for the same reason TS in g3 needs the casts - the version of TS we're on doesn't correctly figure out all cases of the union type are covered.

(mapping as ExternalTemplateSourceMapping).templateUrl;
// TODO(alxhub): investigate creating a fake `ts.SourceFile` here instead of invoking the TS
// parser against the template (HTML is just really syntactically invalid TypeScript code ;).
const sf = ts.createSourceFile(

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 20, 2019

Member

I forgot to mention something here: we are recreating the source file repeatedly when multiple diagnostics are reported in some template, which is quite wasteful. Could we move this over to the resolver somehow and let it parse the source file lazily and cache it?

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 20, 2019

Author Contributor

I added a bit to the TODO talking about that - I'll leave it for future work since right now translateDiagnostic is not stateful.

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/true-diagnostics branch 2 times, most recently from 1813472 to 0393e3c Aug 20, 2019

@JoostK
JoostK approved these changes Aug 21, 2019
Copy link
Member

left a comment

One last thing, could you remove Diagnostic and friends from packages/compiler-cli/src/ngtsc/typecheck/diagnostics.ts as they are no longer used?

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/true-diagnostics branch from 0393e3c to c867189 Aug 21, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

👍 done.

@alxhub

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/true-diagnostics branch from c867189 to 1f450ed Aug 21, 2019

feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics
Historically, the Angular Compiler has produced both native TypeScript
diagnostics (called ts.Diagnostics) and its own internal Diagnostic format
(called an api.Diagnostic). This was done because TypeScript ts.Diagnostics
cannot be produced for files not in the ts.Program, and template type-
checking diagnostics are naturally produced for external .html template
files.

This design isn't optimal for several reasons:

1) Downstream tooling (such as the CLI) must support multiple formats of
diagnostics, adding to the maintenance burden.

2) ts.Diagnostics have gotten a lot better in recent releases, with support
for suggested changes, highlighting of the code in question, etc. None of
these changes have been of any benefit for api.Diagnostics, which have
continued to be reported in a very primitive fashion.

3) A future plugin model will not support anything but ts.Diagnostics, so
generating api.Diagnostics is a blocker for ngtsc-as-a-plugin.

4) The split complicates both the typings and the testing of ngtsc.

To fix this issue, this commit changes template type-checking to produce
ts.Diagnostics instead. Instead of reporting a special kind of diagnostic
for external template files, errors in a template are always reported in
a ts.Diagnostic that highlights the portion of the template which contains
the error. When this template text is distinct from the source .ts file
(for example, when the template is parsed from an external resource file),
additional contextual information links the error back to the originating
component.

A template error can thus be reported in 3 separate ways, depending on how
the template was configured:

1) For inline template strings which can be directly mapped to offsets in
the TS code, ts.Diagnostics point to real ranges in the source.

This is the case if an inline template is used with a string literal or a
"no-substitution" string. For example:

```typescript
@component({..., template: `
<p>Bar: {{baz}}</p>
`})
export class TestCmp {
  bar: string;
}
```

The above template contains an error (no 'baz' property of `TestCmp`). The
error produced by TS will look like:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts:2:11 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. Did you mean 'bar'?
```

2) For template strings which cannot be directly mapped to offsets in the
TS code, a logical offset into the template string will be included in
the error message. For example:

```typescript
const SOME_TEMPLATE = '<p>Bar: {{baz}}</p>';

@component({..., template: SOME_TEMPLATE})
export class TestCmp {
  bar: string;
}
```

Because the template is a reference to another variable and is not an
inline string constant, the compiler will not be able to use "absolute"
positions when parsing the template. As a result, errors will report logical
offsets into the template string:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts (TestCmp template):2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:3:28
    @component({..., template: TEMPLATE})
                               ~~~~~~~~

    Error occurs in the template of component TestCmp.
```

This error message uses logical offsets into the template string, and also
gives a reference to the `TEMPLATE` expression from which the template was
parsed. This helps in locating the component which contains the error.

3) For external templates (templateUrl), the error message is delivered
within the HTML template file (testcmp.html) instead, and additional
information contextualizes the error on the templateUrl expression from
which the template file was determined:

```
<p>Bar: {{baz}}</p>
          ~~~

testcmp.html:2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:10:31
    @component({..., templateUrl: './testcmp.html'})
                                  ~~~~~~~~~~~~~~~~

    Error occurs in the template of component TestCmp.
```

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/true-diagnostics branch from 1f450ed to df211ba Aug 21, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

AndrewKushnir added a commit that referenced this pull request Aug 21, 2019
feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics (#31952)
Historically, the Angular Compiler has produced both native TypeScript
diagnostics (called ts.Diagnostics) and its own internal Diagnostic format
(called an api.Diagnostic). This was done because TypeScript ts.Diagnostics
cannot be produced for files not in the ts.Program, and template type-
checking diagnostics are naturally produced for external .html template
files.

This design isn't optimal for several reasons:

1) Downstream tooling (such as the CLI) must support multiple formats of
diagnostics, adding to the maintenance burden.

2) ts.Diagnostics have gotten a lot better in recent releases, with support
for suggested changes, highlighting of the code in question, etc. None of
these changes have been of any benefit for api.Diagnostics, which have
continued to be reported in a very primitive fashion.

3) A future plugin model will not support anything but ts.Diagnostics, so
generating api.Diagnostics is a blocker for ngtsc-as-a-plugin.

4) The split complicates both the typings and the testing of ngtsc.

To fix this issue, this commit changes template type-checking to produce
ts.Diagnostics instead. Instead of reporting a special kind of diagnostic
for external template files, errors in a template are always reported in
a ts.Diagnostic that highlights the portion of the template which contains
the error. When this template text is distinct from the source .ts file
(for example, when the template is parsed from an external resource file),
additional contextual information links the error back to the originating
component.

A template error can thus be reported in 3 separate ways, depending on how
the template was configured:

1) For inline template strings which can be directly mapped to offsets in
the TS code, ts.Diagnostics point to real ranges in the source.

This is the case if an inline template is used with a string literal or a
"no-substitution" string. For example:

```typescript
@component({..., template: `
<p>Bar: {{baz}}</p>
`})
export class TestCmp {
  bar: string;
}
```

The above template contains an error (no 'baz' property of `TestCmp`). The
error produced by TS will look like:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts:2:11 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. Did you mean 'bar'?
```

2) For template strings which cannot be directly mapped to offsets in the
TS code, a logical offset into the template string will be included in
the error message. For example:

```typescript
const SOME_TEMPLATE = '<p>Bar: {{baz}}</p>';

@component({..., template: SOME_TEMPLATE})
export class TestCmp {
  bar: string;
}
```

Because the template is a reference to another variable and is not an
inline string constant, the compiler will not be able to use "absolute"
positions when parsing the template. As a result, errors will report logical
offsets into the template string:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts (TestCmp template):2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:3:28
    @component({..., template: TEMPLATE})
                               ~~~~~~~~

    Error occurs in the template of component TestCmp.
```

This error message uses logical offsets into the template string, and also
gives a reference to the `TEMPLATE` expression from which the template was
parsed. This helps in locating the component which contains the error.

3) For external templates (templateUrl), the error message is delivered
within the HTML template file (testcmp.html) instead, and additional
information contextualizes the error on the templateUrl expression from
which the template file was determined:

```
<p>Bar: {{baz}}</p>
          ~~~

testcmp.html:2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:10:31
    @component({..., templateUrl: './testcmp.html'})
                                  ~~~~~~~~~~~~~~~~

    Error occurs in the template of component TestCmp.
```

PR Close #31952
ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics (angular#3…
…1952)

Historically, the Angular Compiler has produced both native TypeScript
diagnostics (called ts.Diagnostics) and its own internal Diagnostic format
(called an api.Diagnostic). This was done because TypeScript ts.Diagnostics
cannot be produced for files not in the ts.Program, and template type-
checking diagnostics are naturally produced for external .html template
files.

This design isn't optimal for several reasons:

1) Downstream tooling (such as the CLI) must support multiple formats of
diagnostics, adding to the maintenance burden.

2) ts.Diagnostics have gotten a lot better in recent releases, with support
for suggested changes, highlighting of the code in question, etc. None of
these changes have been of any benefit for api.Diagnostics, which have
continued to be reported in a very primitive fashion.

3) A future plugin model will not support anything but ts.Diagnostics, so
generating api.Diagnostics is a blocker for ngtsc-as-a-plugin.

4) The split complicates both the typings and the testing of ngtsc.

To fix this issue, this commit changes template type-checking to produce
ts.Diagnostics instead. Instead of reporting a special kind of diagnostic
for external template files, errors in a template are always reported in
a ts.Diagnostic that highlights the portion of the template which contains
the error. When this template text is distinct from the source .ts file
(for example, when the template is parsed from an external resource file),
additional contextual information links the error back to the originating
component.

A template error can thus be reported in 3 separate ways, depending on how
the template was configured:

1) For inline template strings which can be directly mapped to offsets in
the TS code, ts.Diagnostics point to real ranges in the source.

This is the case if an inline template is used with a string literal or a
"no-substitution" string. For example:

```typescript
@component({..., template: `
<p>Bar: {{baz}}</p>
`})
export class TestCmp {
  bar: string;
}
```

The above template contains an error (no 'baz' property of `TestCmp`). The
error produced by TS will look like:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts:2:11 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. Did you mean 'bar'?
```

2) For template strings which cannot be directly mapped to offsets in the
TS code, a logical offset into the template string will be included in
the error message. For example:

```typescript
const SOME_TEMPLATE = '<p>Bar: {{baz}}</p>';

@component({..., template: SOME_TEMPLATE})
export class TestCmp {
  bar: string;
}
```

Because the template is a reference to another variable and is not an
inline string constant, the compiler will not be able to use "absolute"
positions when parsing the template. As a result, errors will report logical
offsets into the template string:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts (TestCmp template):2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:3:28
    @component({..., template: TEMPLATE})
                               ~~~~~~~~

    Error occurs in the template of component TestCmp.
```

This error message uses logical offsets into the template string, and also
gives a reference to the `TEMPLATE` expression from which the template was
parsed. This helps in locating the component which contains the error.

3) For external templates (templateUrl), the error message is delivered
within the HTML template file (testcmp.html) instead, and additional
information contextualizes the error on the templateUrl expression from
which the template file was determined:

```
<p>Bar: {{baz}}</p>
          ~~~

testcmp.html:2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:10:31
    @component({..., templateUrl: './testcmp.html'})
                                  ~~~~~~~~~~~~~~~~

    Error occurs in the template of component TestCmp.
```

PR Close angular#31952
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics (angular#3…
…1952)

Historically, the Angular Compiler has produced both native TypeScript
diagnostics (called ts.Diagnostics) and its own internal Diagnostic format
(called an api.Diagnostic). This was done because TypeScript ts.Diagnostics
cannot be produced for files not in the ts.Program, and template type-
checking diagnostics are naturally produced for external .html template
files.

This design isn't optimal for several reasons:

1) Downstream tooling (such as the CLI) must support multiple formats of
diagnostics, adding to the maintenance burden.

2) ts.Diagnostics have gotten a lot better in recent releases, with support
for suggested changes, highlighting of the code in question, etc. None of
these changes have been of any benefit for api.Diagnostics, which have
continued to be reported in a very primitive fashion.

3) A future plugin model will not support anything but ts.Diagnostics, so
generating api.Diagnostics is a blocker for ngtsc-as-a-plugin.

4) The split complicates both the typings and the testing of ngtsc.

To fix this issue, this commit changes template type-checking to produce
ts.Diagnostics instead. Instead of reporting a special kind of diagnostic
for external template files, errors in a template are always reported in
a ts.Diagnostic that highlights the portion of the template which contains
the error. When this template text is distinct from the source .ts file
(for example, when the template is parsed from an external resource file),
additional contextual information links the error back to the originating
component.

A template error can thus be reported in 3 separate ways, depending on how
the template was configured:

1) For inline template strings which can be directly mapped to offsets in
the TS code, ts.Diagnostics point to real ranges in the source.

This is the case if an inline template is used with a string literal or a
"no-substitution" string. For example:

```typescript
@component({..., template: `
<p>Bar: {{baz}}</p>
`})
export class TestCmp {
  bar: string;
}
```

The above template contains an error (no 'baz' property of `TestCmp`). The
error produced by TS will look like:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts:2:11 - error TS2339: Property 'baz' does not exist on type 'TestCmp'. Did you mean 'bar'?
```

2) For template strings which cannot be directly mapped to offsets in the
TS code, a logical offset into the template string will be included in
the error message. For example:

```typescript
const SOME_TEMPLATE = '<p>Bar: {{baz}}</p>';

@component({..., template: SOME_TEMPLATE})
export class TestCmp {
  bar: string;
}
```

Because the template is a reference to another variable and is not an
inline string constant, the compiler will not be able to use "absolute"
positions when parsing the template. As a result, errors will report logical
offsets into the template string:

```
<p>Bar: {{baz}}</p>
          ~~~

test.ts (TestCmp template):2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:3:28
    @component({..., template: TEMPLATE})
                               ~~~~~~~~

    Error occurs in the template of component TestCmp.
```

This error message uses logical offsets into the template string, and also
gives a reference to the `TEMPLATE` expression from which the template was
parsed. This helps in locating the component which contains the error.

3) For external templates (templateUrl), the error message is delivered
within the HTML template file (testcmp.html) instead, and additional
information contextualizes the error on the templateUrl expression from
which the template file was determined:

```
<p>Bar: {{baz}}</p>
          ~~~

testcmp.html:2:15 - error TS2339: Property 'baz' does not exist on type 'TestCmp'.

  test.ts:10:31
    @component({..., templateUrl: './testcmp.html'})
                                  ~~~~~~~~~~~~~~~~

    Error occurs in the template of component TestCmp.
```

PR Close angular#31952
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.