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

A combination of mine and crisbeto@'s recent work on diagnostics in the compiler #34460

Conversation

@alxhub
Copy link
Contributor

alxhub commented Dec 18, 2019

This PR merges together several independent PRs: #34457, #34333, #34061

These were all interdependent and so this PR resolves the conflicts between them. Each constituent PR has already been reviewed & approved.

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Dec 18, 2019

@alxhub alxhub closed this Dec 18, 2019
@alxhub alxhub reopened this Dec 18, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/modules-and-providers-and-inheritance-oh-my branch from 2068ed6 to 21b2e46 Dec 18, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Dec 18, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Dec 18, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@alxhub alxhub force-pushed the alxhub:ngtsc/modules-and-providers-and-inheritance-oh-my branch from 21b2e46 to 9df9095 Dec 18, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Dec 18, 2019
@alxhub alxhub force-pushed the alxhub:ngtsc/modules-and-providers-and-inheritance-oh-my branch 2 times, most recently from 9ccd15d to 226771b Dec 18, 2019
@alxhub alxhub marked this pull request as ready for review Dec 18, 2019
@alxhub alxhub requested review from angular/fw-compiler as code owners Dec 18, 2019
@alxhub alxhub removed the cla: no label Dec 18, 2019
@alxhub alxhub added the cla: yes label Dec 18, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@JoostK
JoostK approved these changes Dec 18, 2019
Copy link
Member

JoostK left a comment

I have only reviewed the first 3 commits by Alex, assuming that the other ones have already been approved.

export class Module {}
`);

for (const diag of env.driveDiagnostics()) {

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 18, 2019

Member

nit: I would personally assign the results of env.driveDiagnostics() to a variable, to help debugging if needed.

@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Dec 18, 2019

crisbeto added 2 commits Dec 11, 2019
Adds a compilation error if the consumer tries to pass in an undecorated class into the `providers` of an `NgModule`, or the `providers`/`viewProviders` arrays of a `Directive`/`Component`.
Angular View Engine uses global knowledge to compile the following code:

```typescript
export class Base {
  constructor(private vcr: ViewContainerRef) {}
}

@directive({...})
export class Dir extends Base {
  // constructor inherited from base
}
```

Here, `Dir` extends `Base` and inherits its constructor. To create a `Dir`
the arguments to this inherited constructor must be obtained via dependency
injection. View Engine is able to generate a correct factory for `Dir` to do
this because via metadata it knows the arguments of `Base`'s constructor,
even if `Base` is declared in a different library.

In Ivy, DI is entirely a runtime concept. Currently `Dir` is compiled with
an ngDirectiveDef field that delegates its factory to `getInheritedFactory`.
This looks for some kind of factory function on `Base`, which comes up
empty. This case looks identical to an inheritance chain with no
constructors, which works today in Ivy.

Both of these cases will now become an error in this commit. If a decorated
class inherits from an undecorated base class, a diagnostic is produced
informing the user of the need to either explicitly declare a constructor or
to decorate the base class.
@alxhub alxhub force-pushed the alxhub:ngtsc/modules-and-providers-and-inheritance-oh-my branch from 2e75d05 to 450b49e Dec 18, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Dec 18, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Dec 18, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 18, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Dec 18, 2019

caretaker note to me: Alex is an owner of fw-ngcc, so Joost's LGTM is sufficient.

kara added a commit that referenced this pull request Dec 18, 2019
#34460)

Previously each NgModule trait checked its own scope for valid declarations
during 'resolve'. This worked, but caused the LocalModuleScopeRegistry to
declare that NgModule scopes were valid even if they contained invalid
declarations.

This commit moves the generation of diagnostic errors to the
LocalModuleScopeRegistry where it belongs. Now the registry can consider an
NgModule's scope to be invalid if it contains invalid declarations.

PR Close #34460
kara added a commit that referenced this pull request Dec 18, 2019
…34460)

Previously, ngtsc would perform scope analysis (which directives/pipes are
available inside a component's template) and template type-checking of that
template as separate steps. If a component's scope was somehow invalid (e.g.
its NgModule imported something which wasn't another NgModule), the
component was treated as not having a scope. This meant that during template
type-checking, errors would be produced for any invalid expressions/usage of
other components that should have been in the scope.

This commit changes ngtsc to skip template type-checking of a component if
its scope is erroneous (as opposed to not present in the first place). Thus,
users aren't overwhelmed with diagnostic errors for the template and are
only informed of the root cause of the problem: an invalid NgModule scope.

Fixes #33849

PR Close #34460
kara added a commit that referenced this pull request Dec 18, 2019
The function `makeTemplateDiagnostic` was accepting an error code of type
`number`, making it easy to accidentally pass an `ErrorCode` directly and
not convert it to an Angular diagnostic code first.

This commit refactors `makeTemplateDiagnostic` to accept `ErrorCode` up
front, and convert it internally. This is less error-prone.

PR Close #34460
kara added a commit that referenced this pull request Dec 18, 2019
…#34460)

Adds a compilation error if the consumer tries to pass in an undecorated class into the `providers` of an `NgModule`, or the `providers`/`viewProviders` arrays of a `Directive`/`Component`.

PR Close #34460
kara added a commit that referenced this pull request Dec 18, 2019
…se (#34460)

Angular View Engine uses global knowledge to compile the following code:

```typescript
export class Base {
  constructor(private vcr: ViewContainerRef) {}
}

@directive({...})
export class Dir extends Base {
  // constructor inherited from base
}
```

Here, `Dir` extends `Base` and inherits its constructor. To create a `Dir`
the arguments to this inherited constructor must be obtained via dependency
injection. View Engine is able to generate a correct factory for `Dir` to do
this because via metadata it knows the arguments of `Base`'s constructor,
even if `Base` is declared in a different library.

In Ivy, DI is entirely a runtime concept. Currently `Dir` is compiled with
an ngDirectiveDef field that delegates its factory to `getInheritedFactory`.
This looks for some kind of factory function on `Base`, which comes up
empty. This case looks identical to an inheritance chain with no
constructors, which works today in Ivy.

Both of these cases will now become an error in this commit. If a decorated
class inherits from an undecorated base class, a diagnostic is produced
informing the user of the need to either explicitly declare a constructor or
to decorate the base class.

PR Close #34460
@kara kara closed this in 047488c Dec 18, 2019
kara added a commit that referenced this pull request Dec 18, 2019
…34460)

Previously, ngtsc would perform scope analysis (which directives/pipes are
available inside a component's template) and template type-checking of that
template as separate steps. If a component's scope was somehow invalid (e.g.
its NgModule imported something which wasn't another NgModule), the
component was treated as not having a scope. This meant that during template
type-checking, errors would be produced for any invalid expressions/usage of
other components that should have been in the scope.

This commit changes ngtsc to skip template type-checking of a component if
its scope is erroneous (as opposed to not present in the first place). Thus,
users aren't overwhelmed with diagnostic errors for the template and are
only informed of the root cause of the problem: an invalid NgModule scope.

Fixes #33849

PR Close #34460
kara added a commit that referenced this pull request Dec 18, 2019
The function `makeTemplateDiagnostic` was accepting an error code of type
`number`, making it easy to accidentally pass an `ErrorCode` directly and
not convert it to an Angular diagnostic code first.

This commit refactors `makeTemplateDiagnostic` to accept `ErrorCode` up
front, and convert it internally. This is less error-prone.

PR Close #34460
kara added a commit that referenced this pull request Dec 18, 2019
…#34460)

Adds a compilation error if the consumer tries to pass in an undecorated class into the `providers` of an `NgModule`, or the `providers`/`viewProviders` arrays of a `Directive`/`Component`.

PR Close #34460
kara added a commit that referenced this pull request Dec 18, 2019
…se (#34460)

Angular View Engine uses global knowledge to compile the following code:

```typescript
export class Base {
  constructor(private vcr: ViewContainerRef) {}
}

@directive({...})
export class Dir extends Base {
  // constructor inherited from base
}
```

Here, `Dir` extends `Base` and inherits its constructor. To create a `Dir`
the arguments to this inherited constructor must be obtained via dependency
injection. View Engine is able to generate a correct factory for `Dir` to do
this because via metadata it knows the arguments of `Base`'s constructor,
even if `Base` is declared in a different library.

In Ivy, DI is entirely a runtime concept. Currently `Dir` is compiled with
an ngDirectiveDef field that delegates its factory to `getInheritedFactory`.
This looks for some kind of factory function on `Base`, which comes up
empty. This case looks identical to an inheritance chain with no
constructors, which works today in Ivy.

Both of these cases will now become an error in this commit. If a decorated
class inherits from an undecorated base class, a diagnostic is produced
informing the user of the need to either explicitly declare a constructor or
to decorate the base class.

PR Close #34460
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 18, 2020

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.