feat(compiler-cli): exclude abstract classes from `strictInjectionPar… #44615

JoostK · 2022-01-04T18:04:24Z

…ameters` requirement

In AOT compilations, the strictInjectionParameters compiler option can
be enabled to report errors when an @Injectable annotated class has a
constructor with parameters that do not provide an injection token, e.g.
only a primitive type or interface.

Since Ivy it's become required that any class with Angular behavior
(e.g. the ngOnDestroy lifecycle hook) is decorated using an Angular
decorator, which meant that @Injectable() may need to have been added
to abstract base classes. Doing so would then report an error if
strictInjectionParameters is enabled, if the abstract class has an
incompatible constructor for DI purposes. This may be fine though, as
a subclass may call the constructor explicitly without relying on
Angular's DI mechanism.

Therefore, this commit excludes abstract classes from the
strictInjectionParameters check. This avoids an error from being
reported at compile time. If the constructor ends up being used by
Angular's DI system at runtime, then the factory function of the
abstract class will throw an error by means of the ɵɵinvalidFactory
instruction.

BREAKING CHANGE: Invalid constructors for DI may now report compilation errors

When a class inherits its constructor from a base class, the compiler may now
report an error when that constructor cannot be used for DI purposes. This may
either be because the base class is missing an Angular decorator such as
@Injectable() or @Directive(), or because the constructor contains parameters
which do not have an associated token (such as primitive types like string).
These situations used to behave unexpectedly at runtime, where the class may be
constructed without any of its constructor parameters, so this is now reported
as an error during compilation.

Any new errors that may be reported because of this change can be resolved either
by decorating the base class from which the constructor is inherited, or by adding
an explicit constructor to the class for which the error is reported.

Closes #37914

packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts

JoostK · 2022-03-23T22:24:15Z

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

+             expect(diags[0].code).toBe(ngErrorCode(ErrorCode.DIRECTIVE_INHERITS_UNDECORATED_CTOR));
+             expect(diags[0].messageText)
+                 .toEqual(
+                     `The directive AbstractMiddleDir inherits its constructor from ParentClass, but the latter does not have an Angular decorator of its own. Dependency injection will not be able to resolve the parameters of ParentClass's constructor. Either add a @Directive decorator to ParentClass, or add an explicit constructor to AbstractMiddleDir.`);


We used to report this report this error for abstract classes and I haven't changed this, but I'm wondering if we should exempt abstract directives from this error.

JoostK · 2022-03-23T22:28:27Z

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

+               @Injectable({ providedIn: 'root', useValue: null })
+               export class UseValueService extends ParentService {}
+
+               @Injectable({ providedIn: 'root', useClass: ConcreteServiceWithCtor })


Interestingly, we don't check useClass here like we check providers in the providers array! 😱

jessicajaniuk

reviewed-for: public-api

dylhunn · 2022-05-02T18:58:18Z

@JoostK It looks like this hasn't been updated in a while. Is this a PR we'd like to merge for 14?

JoostK · 2022-05-02T19:04:16Z

@JoostK It looks like this hasn't been updated in a while. Is this a PR we'd like to merge for 14?

It's ready to go from my side, just awaiting review. I'm afraid we're short on time to get this LGTM'd in time.

packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts

atscott · 2022-05-02T21:31:37Z

packages/compiler-cli/src/ngtsc/annotations/common/src/injectable_registry.ts

+    const ctorDeps = getConstructorDependencies(declaration, this.host, this.isCore);
+    return {
+      ctorDeps: unwrapConstructorDependencies(ctorDeps),
+    };
+  }


Should this call registerInjectable before returning?

Hmm yeah that might make sense to avoid potential repeated work 👍

AndrewKushnir

Reviewed-for: public-api

dylhunn

reviewed-for: public-api

dylhunn · 2022-09-13T06:27:38Z

TGP is running.

dylhunn · 2022-09-21T18:43:43Z

We've resolved all the TGP issues inside of google3, and have inflight CLs now.

…meters` requirement In AOT compilations, the `strictInjectionParameters` compiler option can be enabled to report errors when an `@Injectable` annotated class has a constructor with parameters that do not provide an injection token, e.g. only a primitive type or interface. Since Ivy it's become required that any class with Angular behavior (e.g. the `ngOnDestroy` lifecycle hook) is decorated using an Angular decorator, which meant that `@Injectable()` may need to have been added to abstract base classes. Doing so would then report an error if `strictInjectionParameters` is enabled, if the abstract class has an incompatible constructor for DI purposes. This may be fine though, as a subclass may call the constructor explicitly without relying on Angular's DI mechanism. Therefore, this commit excludes abstract classes from the `strictInjectionParameters` check. This avoids an error from being reported at compile time. If the constructor ends up being used by Angular's DI system at runtime, then the factory function of the abstract class will throw an error by means of the `ɵɵinvalidFactory` instruction. In addition to the runtime error, this commit also analyzes the inheritance chain of an injectable without a constructor to verify that their inherited constructor is valid. BREAKING CHANGE: Invalid constructors for DI may now report compilation errors When a class inherits its constructor from a base class, the compiler may now report an error when that constructor cannot be used for DI purposes. This may either be because the base class is missing an Angular decorator such as `@Injectable()` or `@Directive()`, or because the constructor contains parameters which do not have an associated token (such as primitive types like `string`). These situations used to behave unexpectedly at runtime, where the class may be constructed without any of its constructor parameters, so this is now reported as an error during compilation. Any new errors that may be reported because of this change can be resolved either by decorating the base class from which the constructor is inherited, or by adding an explicit constructor to the class for which the error is reported. Closes angular#37914

dylhunn · 2022-10-10T16:41:57Z

This should now be green based on the previous TGP. Running a TAP Train to double-check before submitting.

dylhunn · 2022-10-10T21:34:38Z

TAP Train was green, we're good to go!

dylhunn · 2022-10-10T21:35:26Z

merge-assistance: this already got all the needed approvals and TAP Train was green.

jessicajaniuk

reviewed-for: public-api

dylhunn

reviewed-for: public-api

jessicajaniuk · 2022-10-10T21:46:28Z

This PR was merged into the repository by commit bc54687.

angular-automatic-lock-bot · 2022-11-10T16:06:30Z

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.}

JoostK added feature Issue that requests a new feature area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Jan 4, 2022

ngbot bot modified the milestone: Backlog Jan 4, 2022

JoostK force-pushed the ngtsc/di/strict-params-abstract branch 2 times, most recently from 72cc61c to 7f30792 Compare March 23, 2022 22:13

JoostK commented Mar 23, 2022

View reviewed changes

JoostK force-pushed the ngtsc/di/strict-params-abstract branch 3 times, most recently from efbc3f0 to b1db475 Compare April 25, 2022 17:43

JoostK marked this pull request as ready for review April 25, 2022 19:20

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 25, 2022

pullapprove bot requested review from alxhub, AndrewKushnir, atscott and jessicajaniuk April 25, 2022 19:20

jessicajaniuk previously approved these changes Apr 26, 2022

View reviewed changes

pullapprove bot requested a review from dylhunn April 26, 2022 09:53

AndrewKushnir modified the milestones: Backlog, v14-candidates Apr 27, 2022

JoostK force-pushed the ngtsc/di/strict-params-abstract branch from b1db475 to 7be53d8 Compare April 27, 2022 18:10

dylhunn dismissed jessicajaniuk’s stale review via 7be53d8 May 2, 2022 18:42

atscott approved these changes May 2, 2022

View reviewed changes

AndrewKushnir previously approved these changes May 2, 2022

View reviewed changes

pullapprove bot requested a review from atscott May 2, 2022 22:14

dylhunn previously approved these changes May 2, 2022

View reviewed changes

dylhunn assigned JoostK Sep 12, 2022

JoostK force-pushed the ngtsc/di/strict-params-abstract branch 3 times, most recently from 48d4344 to 4714782 Compare September 12, 2022 19:07

alxhub assigned dylhunn Sep 29, 2022

alxhub modified the milestones: v15 candidates, v15 feature freeze blockers Sep 29, 2022

JoostK force-pushed the ngtsc/di/strict-params-abstract branch from 4714782 to 699f9b5 Compare October 7, 2022 16:38

angular-robot bot added the flag: breaking change label Oct 7, 2022

JoostK added target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Oct 7, 2022

dylhunn removed the request for review from alxhub October 10, 2022 21:35

dylhunn added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed state: blocked labels Oct 10, 2022

ngbot bot added the action: merge The PR is ready for merge by the caretaker label Oct 10, 2022

jessicajaniuk approved these changes Oct 10, 2022

View reviewed changes

dylhunn self-requested a review October 10, 2022 21:43

dylhunn approved these changes Oct 10, 2022

View reviewed changes

jessicajaniuk removed action: global presubmit The PR is in need of a google3 global presubmit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 10, 2022

jessicajaniuk closed this in bc54687 Oct 10, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler-cli): exclude abstract classes from `strictInjectionPar… #44615

feat(compiler-cli): exclude abstract classes from `strictInjectionPar… #44615

JoostK commented Jan 4, 2022 •

edited

JoostK Mar 23, 2022

JoostK Mar 23, 2022

jessicajaniuk left a comment

dylhunn commented May 2, 2022

JoostK commented May 2, 2022

atscott May 2, 2022

JoostK May 3, 2022

AndrewKushnir left a comment

dylhunn left a comment

dylhunn commented Sep 13, 2022 •

edited

dylhunn commented Sep 21, 2022

dylhunn commented Oct 10, 2022

dylhunn commented Oct 10, 2022

dylhunn commented Oct 10, 2022 •

edited

jessicajaniuk left a comment

dylhunn left a comment

jessicajaniuk commented Oct 10, 2022

angular-automatic-lock-bot bot commented Nov 10, 2022

feat(compiler-cli): exclude abstract classes from `strictInjectionPar… #44615

feat(compiler-cli): exclude abstract classes from `strictInjectionPar… #44615

Conversation

JoostK commented Jan 4, 2022 • edited

JoostK Mar 23, 2022

Choose a reason for hiding this comment

JoostK Mar 23, 2022

Choose a reason for hiding this comment

jessicajaniuk left a comment

Choose a reason for hiding this comment

dylhunn commented May 2, 2022

JoostK commented May 2, 2022

atscott May 2, 2022

Choose a reason for hiding this comment

JoostK May 3, 2022

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

dylhunn commented Sep 13, 2022 • edited

dylhunn commented Sep 21, 2022

dylhunn commented Oct 10, 2022

dylhunn commented Oct 10, 2022

dylhunn commented Oct 10, 2022 • edited

jessicajaniuk left a comment

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

jessicajaniuk commented Oct 10, 2022

angular-automatic-lock-bot bot commented Nov 10, 2022

JoostK commented Jan 4, 2022 •

edited

dylhunn commented Sep 13, 2022 •

edited

dylhunn commented Oct 10, 2022 •

edited