bug: Go to definition doesn't work on the second component in a single file

[eneajaho]

Which @angular/* package(s) are the source of the bug?

language-service

Is this a regression?

Yes

Description

Let's say we have two components in the same file:

@Component({
  selector: 'app-main-layout',
  template: `
    <app-navbar />
     <router-outlet />
    <app-footer />
  `,
  standalone: true,
  imports: [NavbarComponent, FooterComponent, RouterOutlet],
})
export class MainLayoutComponent {}

@Component({
  selector: 'app-home-layout',
  template: `
    <app-navbar [isHome]="true" />
    <router-outlet />
  `,
  standalone: true,
  imports: [NavbarComponent, RouterOutlet],
})
export class HomeLayoutComponent {}

1. On the first component clicking Go to definition for app-navbar works correctly (it sends you to the NavbarComponent)
2. On the second component clicking Go to definition for app-navbar doesn't work (it says no definition found) (same thing for router-outlet)

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in (run ng version)

Angular CLI: 17.2.3
Node: 18.17.0
Package Manager: npm 9.6.7
OS: darwin arm64

Angular: 17.2.4
... animations, common, compiler, compiler-cli, core, forms
... platform-browser, platform-browser-dynamic, router

Package                          Version
----------------------------------------------------------
@angular-devkit/architect        0.1702.3
@angular-devkit/build-angular    17.2.3
@angular-devkit/core             17.2.3
@angular-devkit/schematics       17.2.3
@angular/cdk                     17.2.0
@angular/cli                     17.2.3
@angular/fire                    17.0.1
@angular/material                17.2.0
@angular/material-experimental   17.2.0
@schematics/angular              17.2.3
rxjs                             7.8.1
typescript                       5.2.2
zone.js                          0.14.4

Anything else?

No response

[atscott]

This appears to have been broken by angular/vscode-ng-language-service@bd6282e. @ivanwonder If you have and time, would you be able to take a look at this? I clearly don't understand the tokenizer and can't look into it more anytime soon.

[ivanwonder]

OK, I'll take a look.

[atscott]

@ivanwonder Thank you! We don't necessarily need the whole implementation that TypeScript has for this because we don't care about tokenizing all the parts correctly. All we really need to know is where the start and end is of the string. But maybe the easiest way to do that is just tokenizing the whole thing correctly by copying the TS code.

[ivanwonder]

Yes, the tokenizer is complex, and I also can't fully understand it, and make sure this will work for us.

1. const a =`abc`

The token is ts.SyntaxKind.NoSubstitutionTemplateLiteral which includes the CloseBraceToken. We don't need to call the reScanTemplateToken.

2. const a = `ab${1}c`

We need to call the reScanTemplateToken when encountering a CloseBraceToken.

3. const a = `ab${b({})}c`

This includes an object {}, we need to record the OpenBraceToken, close them all, then call the reScanTemplateToken.

---

But maybe the easiest way to do that is just tokenizing the whole thing correctly by copying the TS code.

I think this is what the ts.createSourceFile does. Scan the code and create the AST tree.
https://github.com/microsoft/TypeScript/blob/806d7340472082e5ff6d844ecdb70a4b5165e8c5/src/compiler/parser.ts#L1592

---

I have an idea about the original problem, I guess which is the diagnostics can be quite expensive. In the typescript the type checker can be interrupted.

    // Cancellation that controls whether or not we can cancel in the middle of type checking.
    // In general cancelling is *not* safe for the type checker.  We might be in the middle of
    // computing something, and we will leave our internals in an inconsistent state.  Callers
    // who set the cancellation token should catch if a cancellation exception occurs, and
    // should throw away and create a new TypeChecker.
    //
    // Currently we only support setting the cancellation token when getting diagnostics.  This
    // is because diagnostics can be quite expensive, and we want to allow hosts to bail out if
    // they no longer need the information (for example, if the user started editing again).
    var cancellationToken: CancellationToken | undefined;

function runWithCancellationToken<T>(func: () => T): T {
        try {
            return func();
        }
        catch (e) {
            if (e instanceof OperationCanceledException) {
                // We were canceled while performing the operation.  Because our type checker
                // might be a bad state, we need to throw it away.
                typeChecker = undefined!;
            }

            throw e;
        }
    }

This copy is from here. https://github.com/microsoft/TypeScript/blob/806d7340472082e5ff6d844ecdb70a4b5165e8c5/src/compiler/checker.ts#L1435
https://github.com/microsoft/TypeScript/blob/806d7340472082e5ff6d844ecdb70a4b5165e8c5/src/compiler/program.ts#L2853

Maybe we can evaluate this option, and try to throw an error in the template type checker and catch the error in the NgCompiler when the request is canceled.

[atscott]

I think this is what the ts.createSourceFile does. Scan the code and create the AST tree.

Yea, this is probably the direction we should go at this point (creating a temporary source file). We can copy the same code used on the server side here:

angular/packages/language-service/src/language_service.ts

Lines 604 to 621 in 75a186e

	function isInAngularContext(program: ts.Program, fileName: string, position: number) {
	if (!isTypeScriptFile(fileName)) {
	return true;
	}
	const node = findTightestNodeAtPosition(program, fileName, position);
	if (node === undefined) {
	return false;
	}
	const assignment = getPropertyAssignmentFromValue(node, 'template') ??
	getPropertyAssignmentFromValue(node, 'templateUrl') ??
	// `node.parent` is used because the string is a child of an array element and we want to get
	// the property name
	getPropertyAssignmentFromValue(node.parent, 'styleUrls') ??
	getPropertyAssignmentFromValue(node, 'styleUrl');
	return assignment !== null && getClassDeclFromDecoratorProp(assignment) !== null;
	}

[ivanwonder]

Yea, this is probably the direction we should go at this point (creating a temporary source file). We can copy the same code used on the server side here:

I have opened an PR for this. You can take a look. I add more explanations in this PR.