cargdev/angular
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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
This commit is contained in:
Alex Rickabaugh
2019-08-01 15:01:55 -07:00
committed by Andrew Kushnir
parent bfc26bcd8c
commit 0287b234ea
10 changed files with 407 additions and 163 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
packages/compiler-cli/test/ngtsc/env.ts
View File

@ -186,8 +186,9 @@ export class NgtscTestEnvironment {
/**
* Run the compiler to completion, and return any `ts.Diagnostic` errors that may have occurred.
*/
driveDiagnostics(): ReadonlyArray<ts.Diagnostic|api.Diagnostic> {
return mainDiagnosticsForTest(['-p', this.basePath]);
driveDiagnostics(): ReadonlyArray<ts.Diagnostic> {
// ngtsc only produces ts.Diagnostic messages.
return mainDiagnosticsForTest(['-p', this.basePath]) as ts.Diagnostic[];
}
driveRoutes(entryPoint?: string): LazyRoute[] {

119
packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
View File

@ -6,9 +6,9 @@
* found in the LICENSE file at https://angular.io/license
*/
import {Diagnostic} from '@angular/compiler-cli';
import * as ts from 'typescript';
import {absoluteFrom as _} from '../../src/ngtsc/file_system';
import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing';
import {loadStandardTestFiles} from '../helpers/src/mock_file_loading';
@ -171,8 +171,10 @@ export declare class CommonModule {
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].messageText).toContain('does_not_exist');
expect(formatSpan(diags[0])).toEqual('/test.ts: 6:51, 6:70');
expect(diags[0].messageText)
.toEqual(`Property 'does_not_exist' does not exist on type '{ name: string; }'.`);
expect(diags[0].start).toBe(199);
expect(diags[0].length).toBe(19);
});
it('should accept an NgFor iteration over an any-typed value', () => {
@ -272,8 +274,9 @@ export declare class CommonModule {
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].messageText).toContain('does_not_exist');
expect(formatSpan(diags[0])).toEqual('/test.ts: 6:51, 6:70');
expect(diags[0].messageText).toEqual(`Property 'does_not_exist' does not exist on type 'T'.`);
expect(diags[0].start).toBe(206);
expect(diags[0].length).toBe(19);
});
it('should property type-check a microsyntax variable with the same name as the expression',
@ -333,52 +336,88 @@ export declare class CommonModule {
const diags = env.driveDiagnostics();
expect(diags.length).toBe(2);
// Error from the binding to [fromBase].
expect(diags[0].messageText)
.toBe(`Type 'number' is not assignable to type 'string | undefined'.`);
expect(formatSpan(diags[0])).toEqual('/test.ts: 19:28, 19:42');
// Error from the binding to [fromChild].
expect(diags[0].start).toEqual(386);
expect(diags[0].length).toEqual(14);
expect(diags[1].messageText)
.toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`);
expect(formatSpan(diags[1])).toEqual('/test.ts: 19:43, 19:58');
expect(diags[1].start).toEqual(401);
expect(diags[1].length).toEqual(15);
});
it('should report diagnostics for external template files', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
describe('error locations', () => {
it('should be correct for direct templates', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
template: \`<p>
{{user.does_not_exist}}
</p>\`,
})
export class TestCmp {
user: {name: string}[];
}`);
@Component({
selector: 'test',
templateUrl: './template.html',
})
export class TestCmp {
user: {name: string}[];
}
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/test.ts'));
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
});
@NgModule({
declarations: [TestCmp],
})
export class Module {}
`);
env.write('template.html', `<div>
<span>{{user.does_not_exist}}</span>
</div>`);
it('should be correct for indirect templates', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
const TEMPLATE = \`<p>
{{user.does_not_exist}}
</p>\`;
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].messageText).toContain('does_not_exist');
expect(formatSpan(diags[0])).toEqual('/template.html: 1:14, 1:33');
@Component({
selector: 'test',
template: TEMPLATE,
})
export class TestCmp {
user: {name: string}[];
}`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/test.ts') + ' (TestCmp template)');
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0])).toBe('TEMPLATE');
});
it('should be correct for external templates', () => {
env.write('template.html', `<p>
{{user.does_not_exist}}
</p>`);
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
@Component({
selector: 'test',
templateUrl: './template.html',
})
export class TestCmp {
user: {name: string}[];
}`);
const diags = env.driveDiagnostics();
expect(diags.length).toBe(1);
expect(diags[0].file !.fileName).toBe(_('/template.html'));
expect(getSourceCodeForDiagnostic(diags[0])).toBe('user.does_not_exist');
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation ![0]))
.toBe(`'./template.html'`);
});
});
});
});
function formatSpan(diagnostic: ts.Diagnostic | Diagnostic): string {
if (diagnostic.source !== 'angular') {
return '<unexpected non-angular span>';
}
const span = (diagnostic as Diagnostic).span !;
const fileName = span.start.file.url.replace(/^C:\//, '/');
return `${fileName}: ${span.start.line}:${span.start.col}, ${span.end.line}:${span.end.col}`;
function getSourceCodeForDiagnostic(diag: ts.Diagnostic): string {
const text = diag.file !.text;
return text.substr(diag.start !, diag.length !);
}