The diagnostic is named control flow, but might be fine.

Yeah that was one of the things I was unsure about. Would it be better as a separate diagnostic? Would renaming this diagnostic be considered a breaking change?

@manbearwiz Since this extended diagnostic is specifically about control flow, this seems out of scope to me. I think it would be better to have a separate optional template diagnostic for this since it doesn't apply to everyone. What do you think?

That works for me. I'll update it to be a separate diagnostic. Any other suggestions while I'm in there? This will definitely touch a lot more files

@manbearwiz It looks like you have legitimate test failures. Can you take a look?

Should be good to go

Running the diagnostic against google's codebase surfaced some interesting issues.

I could narrow it down as following :

@Component({ template: '' })
class UnrelatedComponent {} // unexported

@Directive({ selector: '[foo]' })
export class Foo {}

@Component({
  selector: 'app-root',
  imports: [Foo],
  template: `<div *foo></div>`,
})
export class AppComponent {}

If there is an unexported component, even if that component is unrelated, the diagnostic raises a false positive :

A structural directive `foo` was used in the template without a corresponding import in the component. 
Make sure that the directive is included in the `@Component.imports` array of this component

I pushed a (currently) broken test as a fixup, feel free to try to fix it.

I pushed a (currently) broken test as a fixup, feel free to try to fix it.

We've been discussing this a bit and we do understand why this is happening, which isn't too easy to address.

We've been discussing this a bit and we do understand why this is happening, which isn't too easy to address.

I pushed a commit before I read this. I didn't look that thoroughly, but it seems like it ends up doing a reference comparison on nodes from 2 different ASTs? I definitely don't know enough about the ng compiler to know the best solution. My commit just changes the reference check to a name comparison. That's enough to make all the tests pass, but I would defer to you whether there are additional fields to compare or if different nodes should be passed into that fn entirely.

Yeah that would pass the test but is unreliable; two different classes with equal name would be incorrectly matched. For this particular use-case that might be an acceptable trade-off for simplicity, but in the general sense it is incorrect and I think we need a better check.

The problem occurs because Angular's template type-checking approach operates in a separate TypeScript ts.Program from the ts.Program that is used for the primary compilation. The second ts.Program is mostly identical except for containing Angular-specific type-checking code (TCBs, for type-check blocks). These TCBs are typically emitted into separate .ngtypecheck.ts files that are adjacent to the original source files, but in the case of non-exported classes the compiler wouldn't be able to reference the backing type. Hence, it uses a different strategy where the TCBs are emitted into the original source file (but only within the second ts.Program that is specific to type-checking) and this means that the AST has to be recreated, creating new object identities for all AST nodes. Then, type-checking symbols use the AST from the type-checkingts.Program, but directive metadata that has been gathered is obtained from the original ts.Program, resulting in the reference equality to miss.

Thanks for the explanation! That makes a lot of sense and matches what I was seeing when I was debugging.

When the TCBs are emitted into the source files are they guaranteed to be at the end? In that case, position and file path might work to give us more confidence in a match?

Do we know if this affects any of the other diagnostics? I'm guessing the missing control flow check has this same issue.

edit: I checked and yeah, tests in missing_control_flow_directive_spec.ts do break when you add an unexported class.

The type-check file may be adjusted in various places:

as well as additional import statements at the top.

My suggestion would be to search for declarations of the same name in both files, then cross-reference based on their index in this list. TCB-generated code shouldn't have conflicting names so the indices should be stable. We'd only need to do this if we know that the type-check declaration is within a source file that was updated in the type-checking ts.Program:

so the perf impact of this (realizing this approach is quite expensive) may not be too much of a concern?

I think that makes sense. I want to see if I can make a test case that makes the simple name comparison fail.

Since this is a bug that affects existing diagnostics, do we want to handle this in this feature PR or should it be a seperate bug fix PR that would go in first?

Having it in this PR could help us check if this actually fixes any false-positive we detected in Google's monorepo.

I think it would be fine as a separate commit.

I found a final breakage within the Google codebase.

@Directive({ selector: '[foo]' })
export class FooDir {}

export function foo() {
  @Component({
    selector: 'error',
    imports: [FooDir],
    template: '<div *foo></div>',
  })
  class MyCmp {}
}

A function that wraps a component and which has a structural directive will report the import error despite having the correct import.

This pattern was found in 2 unit tests, so it's really a rare case.

G3 has been cleaned up. We'll go through a re-review after the recent changes.

I see that you just added the action: merge label, but the following checks are still failing:
     status "google-internal-tests" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Passing TGP, the broken target got fixed in between.

This PR was merged into the repository by commit c889382.

The changes were merged into the following branches: main

@manbearwiz Thanks for your effort into this PR! 💯

Of course! Glad it got in. Thanks @JeanMeche for taking it the rest of the way!

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