feat(compiler-cli): Support template binding to protected component members

[zelliott]

PR Checklist

Please check if your PR fulfills the following requirements:

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

(unclear if any docs need to be updated)

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?

Currently, you can not create a template binding to a protected component member. For example:

@Component({
  selector: 'my-component',
  template: '{{message}}',  // Does not compile, as `message` is protected and not visible to the template.
})
export class MyComponent {
  protected message = 'Hello world';
}

Issue Number: N/A

What is the new behavior?

Now, the code above compiles.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[zelliott]

@JoostK JFYI - I think the test failures on CircleCI are real failures that we don't want. I think the original diagnostic messages are correct. Looking into it, but LMK if you have any thoughts. It looks like we're hitting this scenario: http://typescriptlang.org/docs/handbook/2/classes.html#cross-hierarchy-protected-access.

[zelliott]

I think the scenario is essentially this:

// some-cmp.ng.html
// <other-cmp [foo]="bar"></other-cmp>

class SomeCmp {
    bar = 5;
}

class OtherCmp {
    protected foo = 4;
}

function tcb(this: SomeCmp): void {
  var ctx = this;
  var _t1: OtherCmp = null!;
  _t1.foo = ctx.bar;
}

where the expression _t1.foo has the error:

Property 'foo' is protected and only accessible through an instance of class 'SomeCmp'. This is an instance of class 'OtherCmp'.

Here's a TS Playground: https://www.typescriptlang.org/play?#code/PTAEGcHsFsFMFoDG0AOA6AdgczQCwC7QA2AUCKADyT66wBOSqoA2gGaSQC6AvAEQBGAQzq8AfBWDVaDZClEkSiIoPDhQAZRiwAwkwDeJUEdBC6obqACsAbhIBfBUpVqA8jXq6UoA8dAo61LCI+LAAJqDskOagACy2DiSsAK4YwQCWkBig+Ij8ABQ0aeAAXBpangCUpQBukGnhPqDVwqDBAB7RheC2Rs1mAPr4AIylbtKe0RhJREQAhD2gg0NokdHtaKbxQA

Previously, before this PR, the error read:

Property 'foo' is protected and only accessible within class 'OtherCmp' and its subclasses.

Which is I think much clearer. Are these diagnostic messages user-facing? If so, this new error message might be a problem. Otherwise, maybe it's fine to just update the test assertion to expect the new error message.

[JoostK]

Those errors are indeed user-facing; we only translate the TS diagnostics to their original template location but we use the messages directly.

Interestingly, this behaviour was changed in microsoft/TypeScript#47991 for TS 4.7 and if you switch the playground over to Nightly you'll find that the error is as it used to be. We've only switched the repo over to TS 4.7 in the last couple days and it is super strange that the test job succeeds, but only test_win fails. I suspect that test_win is somehow still pulling in TS 4.6 as otherwise I have no idea how it can report different diagnostics compared to the Linux test job.

[zelliott]

Nice find! What's the best path forward then? It sounds like we need to determine why test_win doesn't seem to be on TS 4.7. Who's the best person to look into this? I'm not sure exactly where to start, might be more efficient for someone more familiar with Angular's testing infra.

[JoostK]

I just SSH'd into the Windows box to see what's going on, and the root cause is that the Windows CI job does not use the rebased checkout, unlike the Linux test jobs (the rebase is done in the setup job). You'll have to fetch latest main from angular/angular and rebase on top of it, then force push to also run the Windows tests using TypeScript 4.7. I'll mention this to infra folks.

[zelliott]

Great, looks like that resolved it.

[zelliott]

@JoostK, regarding this:

I'm sending a quick addendum to my previous suggestion as I realized we also emit nested function expressions into a type-check function, which would have a separate this pointer. This means that the proposed change won't work, but this can be worked around by generating a single const ctx = this; as first statement into the type-check function such that references can continue to use ctx as they do today.

I haven't been able to get this addendum to work. Here's a playground that demonstrates. Am I missing something?

Note that we can still use string index notation to work around this (and also bind to private members), but I know you have concerns around the stability of that TS behavior.

EDIT: Maybe we can do something like this: Another playground. Not sure if this breaks other stuff.

[JoostK]

The idea of the const ctx = this; alias was to be able to use ctx within a function expression instead of this directly, as the function expression would have its own calling context. Your playgrounds show that using ctx from within such function expression voids the privileged access to protected members, so we need some other way.

I see two possibilities, one of which actually seems to work:

1. Change the function expression into an arrow function. Playground.
2. Add an explicit this: TestComponent parameter to function expressions. Playground. This doesn't actually work because addEventListener doesn't accept a listener with bound this type, so this approach is not feasible it seems.

We can get rid of the const ctx = this; alias in both cases, as all usages can then be updated to use this directly.

[zelliott]

Yep, makes sense. Changed to an arrow function, removed the const ctx = this, and updated tests accordingly.

[zelliott]

• g3sync: cl/445520050 (the g3sync command pulled in some changes from other commits, not sure if this is expected, but I reverted them before running TGP).
• Testing this PR with some real-world g3 code: cl/445417620 (LGTM).

[jelbourn]

Should there be similar tests for @Directive as well?

[alxhub]

I don't believe there's any template type-checking associated with directives.

[zelliott]

@alxhub This thread made me think of host attribute bindings and whether it was possible today to create a host attribute binding to a protected/private member. From my testing on StackBlitz, it looks like it's already possible (which surprised me).

[JoostK]

We don't type-check host bindings at all at the moment (not since Ivy, at least; VE had limited type-checking of host bindings)

[jelbourn]

Well, that sounds like something for the backlog

[JoostK]

Great work! It would be great if you could also update the README.md file to reflect the changes.

[zelliott]

I realized I think I'm "dismissing" these reviews with my force pushes (rebasing + force pushing to avoid a ton of noisy commits). Please LMK if this isn't the right workflow for future reference!

[zelliott]

• g3sync: cl/445520050 (the g3sync command pulled in some changes from other commits, not sure if this is expected, but I reverted them before running TGP).
• Testing this PR with some real-world g3 code: cl/445417620 (LGTM).

g3sync PR looks good except for some already failing targets.

[jelbourn]

@zelliott we just turned on a new branch protection feature that dismisses all approvals when new commits are pushed as a security safeguard. For the time being, this prevents the "lgtm with nits" workflow until we add some additional tooling.

[jelbourn]

lgtm

[atscott]

presubmit

[dylhunn]

This PR was merged into the repository by commit 752ddbc.

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