feat(compiler-cli): detect missing control flow directive imports

[AndrewKushnir]

This commit adds an extended diagnostics check that verifies that all control flow directives (such as ngIf, ngFor) have matching directives. Currently there is no diagnistics produced for such cases, which makes it harder for developers to detect and find a problem.

PR Type

What kind of change does this PR introduce?

• Bugfix

Does this PR introduce a breaking change?

• Yes
• No

[AndrewKushnir]

Exploratory presubmit (TGP).

[Harpush]

Just wondering - but isn't this PR solving a specific case that should be solved by #37322 ? Once that one is fixed - ngIf without common module will be unknown structual directive like any other and error.

[pkozlowski-opensource]

@Harpush you are right, it is a specific fix for a more general case described in #37322. It turns out, though, that having precise error message in a general case is not feasible as we don't have enough info about user's intention. This PR is a pragmatic compromise.

[Harpush]

@pkozlowski-opensource Shouldn't the error message (not from extended diagnostic) come from the compiler not finding the directive applied to the ng-emplate? I mean in both ngIf and any other structual directive the error is the same in case the directive isn't found (no error at all currently - Thus the linked issue). I agree that ngIf can be further extended to say the actual module that is missing.

[pkozlowski-opensource]

@Harpush right, suggestion about a NgModule to import is one aspect of it. When it comes to the "directive didn't match", the situation is more nuanced. #37322 talks about the specific situation: <div *something>. In compiler this will de-sugar to <ng-template something>.

Now the question we should ask: what is the meaning of something? It is reasonable to have 2 different takes on it:

1. we expect that something is an attribute on which at least one directive should match - if there are no matching directives we should error - this is what you are suggesting;
2. we assume that something is just a random attribute on which a directive can match - but is not required to. Such an attribute could be used for content projection, for example.

As we see the (2) usage in the wild we don't have much choice but, at this point, assume the (2) meaning. This is why we can't error on seeing <div *something> without a matching directive... I know that this is not ideal and might be confusing - but this is something we need to take into account when evolving a framework that has lots of existing code depending on it.

[Harpush]

@pkozlowski-opensource I understand the problem... Isn't there a way to "remember" it was once *something? If it was possible there would have been a certain distinction between random <ng-template something> and *something. That would solve the structual directives related problems when applied with *. Maybe even de-sugar to <ng-template [something]=""> or something similar.

[pkozlowski-opensource]

@Harpush as I've mentioned we see usage of <div *someAttrForConentProjectionMatching> where people are trying to express <ng-template someAttrForConentProjectionMatching> without any intention of having a directive matching on someAttrForConentProjectionMatching. So this is not a question of remembering or any other technical solution - it is about the meaning / intention.

We could change the meaning to expect *directive to have a directive but this would be a breaking change.

[Harpush]

@pkozlowski-opensource I believe the current way some people use * is wrong... But it's indeed a breaking change and I am not sure it's a step you wish to take. A possible solution can be a compiler flag. We already have strict related compiler flags so why not add something like strictAttributeDirectives? Or a more deicated strictStructualDirectives?

[pkozlowski-opensource]

I believe the current way some people use * is wrong...

Not sure if there is right / wrong here. I think that we just never made it intentional (in teaching / supported syntax / errors) so people can see it both ways.

At this point we are in the v14 RC period so a breaking change, even behind a compiler flag is not an option. And we would really need to dive deeper into desirable behaviour. Error message improvement is a temporary measure that we can do before the v14 final.

[AndrewKushnir]

Presubmit + TGP.

[jessicajaniuk]

reviewed-for: public-api

[alxhub]

Reviewed-for: public-api

[jessicajaniuk]

This PR was merged into the repository by commit 131d029.

[angular-automatic-lock-bot]

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