feat(compiler-cli): add extended diagnostics to check for component v… by robertIsaac · Pull Request #48658 · angular/angular

robertIsaac · 2023-01-07T13:40:34Z

…ariable shadowing template reference

for example

@COMPONENT({
  template: '<div #var1></div>'
})
export class FooComponent {
  var1: string;
}

fixes #45227

right now this goes without error, but it can lead to mistakenly inside the template to use the template reference as if it was the component variable

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: 45227

What is the new behavior?

it will show warning when there is shadowing between component variable and template reference

Does this PR introduce a breaking change?

Yes
No

I guess it's not, because it's a warning not an error, but I'm leaving it for the Angular team to decide

to fix the warning, simply rename the component variable or the template reference

Other information

this will show the warning even if the component variable is private, which in my opinion is correct
but in case the Angular team disagree, I will make it only show the warning in case of protect or public variables only

robertIsaac · 2023-01-09T15:09:05Z

@jessicajaniuk @AndrewKushnir
why google-internal-tests is failing? I can't see the details of this

JoostK

There's a few more cases that such check would need to take into account:

Inherited properties will not cause the message to be reported.
Template scopes can shadow ancestor template variables:

<div *ngFor="let local of []">
  <button #local></button>
</div>

Here, #local shadows the let-local declaration of the ng-template element that is created by the structural ngFor directive.

AndrewKushnir

@robertIsaac thanks for the PR 👍

I've left some comments, including additional information about build failures from the internal tests run.

AndrewKushnir · 2023-01-09T19:02:13Z

...li/src/ngtsc/typecheck/extended/checks/component_variable_shadow_template_reference/index.ts

The deep link here is the reason for the internal test failures. Could you please update it to avoid the deep link?

Suggested change

import {Reference} from '@angular/compiler/src/render3/r3_ast';

import {TmplAstReference as Reference} from '@angular/compiler';

I see there is more imports from src/render3/r3_ast, should I fix them as well?

AndrewKushnir · 2023-01-09T19:04:19Z

...li/src/ngtsc/typecheck/extended/checks/component_variable_shadow_template_reference/index.ts

This line is also causing some issues, could you please update it as well?

Suggested change

import ts, {Identifier} from 'typescript';

import ts from 'typescript';

and use ts.Identifier in the code below.

AndrewKushnir · 2023-01-09T19:10:40Z

...li/src/ngtsc/typecheck/extended/checks/component_variable_shadow_template_reference/index.ts

There are 3 other build errors, presumably related to the lack of type-narrowing for the node (potentially related to the deep linking issue above):

packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadow_template_reference/index.ts:32:36 - error TS2339: Property 'name' does not exist on type 'Node | AST'. Property 'name' does not exist on type 'Node'. 32 const templateReference = node.name; ~~~~ packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadow_template_reference/index.ts:41:9 - error TS2345: Argument of type 'ParseSourceSpan | AbsoluteSourceSpan' is not assignable to parameter of type 'ParseSourceSpan'. Type 'AbsoluteSourceSpan' is missing the following properties from type 'ParseSourceSpan': fullStart, details 41 node.sourceSpan, ~~~~~~~~~~~~~~~

and an error related to the implicit any type in the following code:

packages/compiler-cli/src/ngtsc/typecheck/extended/checks/component_variable_shadow_template_reference/index.ts:34:31 - error TS7006: Parameter 'member' implicitly has an 'any' type. 34 component.members.map(member => (member.name as Identifier)?.escapedText.toString()); ~~~~~~

do I need to explicitly provide type to member or what? because in my IDE it shows correctly member: ts.ClassElement

AndrewKushnir · 2023-01-09T19:26:03Z

packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

Suggested change

* @COMPONENT({

* @Component({

AndrewKushnir · 2023-01-09T19:27:09Z

packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

Suggested change

* A variable in the component shadow the same name as a template reference.

* A template reference shadows a component class property or method.

AndrewKushnir · 2023-01-09T19:27:40Z

goldens/public-api/compiler-cli/extended_template_diagnostic_name.md

Suggested change

COMPONENT_VARIABLE_SHADOW_TEMPLATE_REFERENCE = "ComponentVariableShadowTemplateReference",

COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE = "ComponentVariableShadowsTemplateReference",

I renamed everything to be shadows

robertIsaac · 2023-01-09T21:26:04Z

Inherited properties will not cause the message to be reported.

so members of the component return only direct members? is there anyway to get all the members of a class?

Template scopes can shadow ancestor template variables:
<div *ngFor="let local of []">
  <button #local></button>
</div>
Here, #local shadows the let-local declaration of the ng-template element that is created by the structural ngFor directive.

oh I missed this when I was solving the solution, will see what I can do about it tomorrow

robertIsaac · 2023-01-09T21:47:10Z

@JoostK I found making componentVariables to equal (component.parent as unknown as {identifiers: Map<string, string>}).identifiers.keys(); solves the first problem
but there is two problems

the type of parent is node which shouldn't have identifiers
it will also forbid using the class names of the component and its superclasses
any suggestions here?

JoostK · 2023-01-09T21:47:54Z

so members of the component return only direct members? is there anyway to get all the members of a class?

Indeed, members is a property of the parse tree so it corresponds with syntactical items of the class. You'll need a ts.TypeChecker to obtain all properties for a given declaration (I'm on mobile atm and don't know how to get to that, not which methods to then use, by heart)

robertIsaac · 2023-01-10T13:11:39Z

You'll need a ts.TypeChecker to obtain all properties for a given declaration (I'm on mobile atm and don't know how to get to that, not which methods to then use, by heart)

@JoostK I don't get what you mean here, TypeChecker is interface
anyway I found another way to do it, can you take a look now

robertIsaac · 2023-01-10T14:08:14Z

@JoostK I made test cases to check how actually angular will work with edge cases
https://stackblitz.com/edit/angular-ivy-jnn75s

and it seems things are actually more complicated than what I originally thought
the template reference has scope for structural directives tree, but if there is none then it's global for the template

so the acceptance criteria from my opinion is:

any template reference shouldn't shadow any component variable (that's done in this PR)
any two template references can shadow each other as long as they have different scopes - inside different structural directive elements (that's the current behavior and we need to maintain it)
global references shouldn't shallow each others (not done)
scoped references shouldn't shallow global ones (not done)
inside same scope, no two reference should shallow each other (not done)

do you think there is more edge cases I should consider?

robertIsaac · 2023-01-13T16:11:12Z

@AndrewKushnir @JoostK any update about this PR?

JeanMeche · 2023-07-03T12:53:42Z

Can you rebase your branch to fix the conflicts ? (Error 8108 is now taken).

robertIsaac · 2023-07-03T15:12:04Z

Can you rebase your branch to fix the conflicts ? (Error 8108 is now taken).

done

do you plan to accept it?

jessicajaniuk · 2023-07-10T17:04:38Z

@robertIsaac looks like you have failing status checks. You'll need to resolve those first. One of them is showing that you need to update goldens now that you've added this check.

JeanMeche · 2023-07-18T21:57:33Z

(You'll need to run yarn bazel run //packages/compiler-cli:error_code_api.accept for the tests to pass).

robertIsaac · 2023-07-19T19:25:31Z

(You'll need to run yarn bazel run //packages/compiler-cli:error_code_api.accept for the tests to pass).

done

…ariable shadowing template reference for example ``` @component({ template: '<div #var1></div>' }) export class FooComponent { var1: string; } ``` right now this goes without error, but it can lead to mistakenly inside the template to use the template reference as if it was the component variable Fixes angular#45227

JoostK · 2024-09-25T15:17:01Z

We discussed this just now and decided to close this for now, as the PR in its current state is not ready to land:

it uses internal fields of TS AST nodes, which we'd rather avoid as it's brittle w.r.t TS updates, as well as being incomplete w.r.t. inherited fields.
There's no shadowing checks for nested templates (which may shadow variable declarations of enclosing templates)

robertIsaac · 2024-09-25T19:23:45Z

hi @JoostK thanks for your feedback
but I'm interested to know what should be the proper way to do it
in a previous comment it was mentioned I should use ts.TypeChecker but I never understood how exactly to use it

JoostK · 2024-09-25T20:18:38Z

ts.TypeChecker provides a method getPropertiesOfType to go from a ts.Type to its properties as ts.Symbol. Those symbols also have names, and that result also includes properties from base classes, which wouldn't be part of the class' AST structure. An extended diagnostic has access to the type checker in ctx.typeChecker (with ctx being TemplateContext that is passed as first parameter of TemplateCheck.run).

You'd also have to go from the ts.ClassDeclaration to its ts.Type using ts.TypeChecker.getTypeAtLocation(), arriving at something like the following:

const properties = ctx.typeChecker.getPropertiesOfType(ctx.typeChecker.getTypeAtLocation(component));

angular-automatic-lock-bot · 2024-10-26T00:11:08Z

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

pullapprove bot requested a review from AndrewKushnir January 7, 2023 13:40

angular-robot bot added the detected: feature PR contains a feature commit label Jan 7, 2023

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch from e43432c to 10b7a88 Compare January 7, 2023 14:34

AndrewKushnir requested a review from JoostK January 9, 2023 18:52

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer target: minor This PR is targeted for the next minor release area: compiler Issues related to `ngc`, Angular's template compiler compiler: extended diagnostics labels Jan 9, 2023

ngbot bot modified the milestone: Backlog Jan 9, 2023

JoostK requested changes Jan 9, 2023

View reviewed changes

AndrewKushnir reviewed Jan 9, 2023

View reviewed changes

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch 3 times, most recently from 53b6526 to c1e974a Compare January 9, 2023 21:19

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch from 7d2df67 to 4a79c47 Compare January 10, 2023 13:34

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch from 4a79c47 to c4fa18c Compare January 10, 2023 14:40

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch 2 times, most recently from fc18891 to 99ab9f4 Compare July 3, 2023 15:10

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch from 99ab9f4 to ab8c6c2 Compare July 19, 2023 19:25

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch from ab8c6c2 to ae70936 Compare July 20, 2023 10:17

robertIsaac added 2 commits September 28, 2023 20:51

robertIsaac force-pushed the feat/compiler-cli/extended-diagnostics/component-variable-shadow-template-reference/45227 branch from ae70936 to fd2b2ec Compare September 28, 2023 17:51

JoostK closed this Sep 25, 2024

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 26, 2024

	import {Reference} from '@angular/compiler/src/render3/r3_ast';
	import {TmplAstReference as Reference} from '@angular/compiler';

	import ts, {Identifier} from 'typescript';
	import ts from 'typescript';

	* A variable in the component shadow the same name as a template reference.
	* A template reference shadows a component class property or method.

	COMPONENT_VARIABLE_SHADOW_TEMPLATE_REFERENCE = "ComponentVariableShadowTemplateReference",
	COMPONENT_VARIABLE_SHADOWS_TEMPLATE_REFERENCE = "ComponentVariableShadowsTemplateReference",

Comments

Conversation

robertIsaac commented Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

robertIsaac commented Jan 9, 2023

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertIsaac commented Jan 9, 2023

Uh oh!

robertIsaac commented Jan 9, 2023

Uh oh!

JoostK commented Jan 9, 2023

Uh oh!

robertIsaac commented Jan 10, 2023

Uh oh!

robertIsaac commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertIsaac commented Jan 13, 2023

Uh oh!

JeanMeche commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertIsaac commented Jul 3, 2023

Uh oh!

jessicajaniuk commented Jul 10, 2023

Uh oh!

JeanMeche commented Jul 18, 2023

Uh oh!

robertIsaac commented Jul 19, 2023

Uh oh!

JoostK commented Sep 25, 2024

Uh oh!

robertIsaac commented Sep 25, 2024

Uh oh!

JoostK commented Sep 25, 2024

Uh oh!

angular-automatic-lock-bot bot commented Oct 26, 2024

Uh oh!

Reviewers

Assignees

Labels

robertIsaac commented Jan 7, 2023 •

edited

Loading

robertIsaac commented Jan 10, 2023 •

edited

Loading

JeanMeche commented Jul 3, 2023 •

edited

Loading