Skip to content

Don't skip markLinkedReferences on ambient properties #59325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 17, 2024
Merged

Don't skip markLinkedReferences on ambient properties #59325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0fd3b42
Add test
jakebailey Jul 17, 2024
fdf4432
Don't skip markLinkedReferences on ambients
jakebailey Jul 17, 2024
e6407a8
PR feedback
jakebailey Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29679,8 +29679,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (!canCollectSymbolAliasAccessabilityData) {
return;
}
if (location.flags & NodeFlags.Ambient) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wouldn't want to remove this outright - perf tests won't show it, but this is pretty important to keep this fast from my testing. I'd just except ambient property signatures from the early skip, since they're like... not really ambient? It looks like when we have something like

class A {
  @decorator(class B {})
  declare prop: string;
}

the prop is ambient marked, but the decorator is not, so we should just be able to except the prop, like @rbuckton says.

if (location.flags & NodeFlags.Ambient && !isPropertySignature(location) && !isPropertyDeclaration(location)) {

seems like it aughta work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done; that seemed to be enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare fields aren't quite as ambient as other declarations. They were added as a compatiblity mechanism due to native class fields using Define semantics, with legacy decorators being one of the main motivations.

return; // References within types and declaration files are never going to contribute to retaining a JS import
if (location.flags & NodeFlags.Ambient && !isPropertySignature(location) && !isPropertyDeclaration(location)) {
// References within types and declaration files are never going to contribute to retaining a JS import,
// except for properties (which can be decorated).
return;
}
switch (hint) {
case ReferenceHint.Identifier:
Expand Down
36 changes: 36 additions & 0 deletions tests/baselines/reference/decoratorMetadataElidedImportOnDeclare(module=commonjs).js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//// [tests/cases/compiler/decoratorMetadataElidedImportOnDeclare.ts] ////

//// [observable.d.ts]
export declare class Observable<T> {}

//// [index.ts]
import { Observable } from './observable';

function whatever(a: any, b: any) {}

class Test {
@whatever
declare prop: Observable<string>;
}


//// [index.js]
"use strict";
var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var __metadata = (this && this.__metadata) || function (k, v) {
if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
};
Object.defineProperty(exports, "__esModule", { value: true });
const observable_1 = require("./observable");
function whatever(a, b) { }
class Test {
}
__decorate([
whatever,
__metadata("design:type", observable_1.Observable)
], Test.prototype, "prop", void 0);
27 changes: 27 additions & 0 deletions tests/baselines/reference/decoratorMetadataElidedImportOnDeclare(module=commonjs).symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//// [tests/cases/compiler/decoratorMetadataElidedImportOnDeclare.ts] ////

=== observable.d.ts ===
export declare class Observable<T> {}
>Observable : Symbol(Observable, Decl(observable.d.ts, 0, 0))
>T : Symbol(T, Decl(observable.d.ts, 0, 32))

=== index.ts ===
import { Observable } from './observable';
>Observable : Symbol(Observable, Decl(index.ts, 0, 8))

function whatever(a: any, b: any) {}
>whatever : Symbol(whatever, Decl(index.ts, 0, 42))
>a : Symbol(a, Decl(index.ts, 2, 18))
>b : Symbol(b, Decl(index.ts, 2, 25))

class Test {
>Test : Symbol(Test, Decl(index.ts, 2, 36))

@whatever
>whatever : Symbol(whatever, Decl(index.ts, 0, 42))

declare prop: Observable<string>;
>prop : Symbol(Test.prop, Decl(index.ts, 4, 12))
>Observable : Symbol(Observable, Decl(index.ts, 0, 8))
}

31 changes: 31 additions & 0 deletions tests/baselines/reference/decoratorMetadataElidedImportOnDeclare(module=commonjs).types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//// [tests/cases/compiler/decoratorMetadataElidedImportOnDeclare.ts] ////

=== observable.d.ts ===
export declare class Observable<T> {}
>Observable : Observable<T>
> : ^^^^^^^^^^^^^

=== index.ts ===
import { Observable } from './observable';
>Observable : typeof Observable
> : ^^^^^^^^^^^^^^^^^

function whatever(a: any, b: any) {}
>whatever : (a: any, b: any) => void
> : ^ ^^ ^^ ^^ ^^^^^^^^^
>a : any
>b : any

class Test {
>Test : Test
> : ^^^^

@whatever
>whatever : (a: any, b: any) => void
> : ^ ^^ ^^ ^^ ^^^^^^^^^

declare prop: Observable<string>;
>prop : Observable<string>
> : ^^^^^^^^^^^^^^^^^^
}

34 changes: 34 additions & 0 deletions tests/baselines/reference/decoratorMetadataElidedImportOnDeclare(module=esnext).js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//// [tests/cases/compiler/decoratorMetadataElidedImportOnDeclare.ts] ////

//// [observable.d.ts]
export declare class Observable<T> {}

//// [index.ts]
import { Observable } from './observable';

function whatever(a: any, b: any) {}

class Test {
@whatever
declare prop: Observable<string>;
}


//// [index.js]
var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var __metadata = (this && this.__metadata) || function (k, v) {
if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
};
import { Observable } from './observable';
function whatever(a, b) { }
class Test {
}
__decorate([
whatever,
__metadata("design:type", Observable)
], Test.prototype, "prop", void 0);
27 changes: 27 additions & 0 deletions tests/baselines/reference/decoratorMetadataElidedImportOnDeclare(module=esnext).symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//// [tests/cases/compiler/decoratorMetadataElidedImportOnDeclare.ts] ////

=== observable.d.ts ===
export declare class Observable<T> {}
>Observable : Symbol(Observable, Decl(observable.d.ts, 0, 0))
>T : Symbol(T, Decl(observable.d.ts, 0, 32))

=== index.ts ===
import { Observable } from './observable';
>Observable : Symbol(Observable, Decl(index.ts, 0, 8))

function whatever(a: any, b: any) {}
>whatever : Symbol(whatever, Decl(index.ts, 0, 42))
>a : Symbol(a, Decl(index.ts, 2, 18))
>b : Symbol(b, Decl(index.ts, 2, 25))

class Test {
>Test : Symbol(Test, Decl(index.ts, 2, 36))

@whatever
>whatever : Symbol(whatever, Decl(index.ts, 0, 42))

declare prop: Observable<string>;
>prop : Symbol(Test.prop, Decl(index.ts, 4, 12))
>Observable : Symbol(Observable, Decl(index.ts, 0, 8))
}

31 changes: 31 additions & 0 deletions tests/baselines/reference/decoratorMetadataElidedImportOnDeclare(module=esnext).types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//// [tests/cases/compiler/decoratorMetadataElidedImportOnDeclare.ts] ////

=== observable.d.ts ===
export declare class Observable<T> {}
>Observable : Observable<T>
> : ^^^^^^^^^^^^^

=== index.ts ===
import { Observable } from './observable';
>Observable : typeof Observable
> : ^^^^^^^^^^^^^^^^^

function whatever(a: any, b: any) {}
>whatever : (a: any, b: any) => void
> : ^ ^^ ^^ ^^ ^^^^^^^^^
>a : any
>b : any

class Test {
>Test : Test
> : ^^^^

@whatever
>whatever : (a: any, b: any) => void
> : ^ ^^ ^^ ^^ ^^^^^^^^^

declare prop: Observable<string>;
>prop : Observable<string>
> : ^^^^^^^^^^^^^^^^^^
}

18 changes: 18 additions & 0 deletions tests/cases/compiler/decoratorMetadataElidedImportOnDeclare.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// @target: es2020
// @module: commonjs, esnext
// @strict: true
// @experimentalDecorators: true
// @emitDecoratorMetadata: true

// @filename: observable.d.ts
export declare class Observable<T> {}

// @filename: index.ts
import { Observable } from './observable';

function whatever(a: any, b: any) {}

class Test {
@whatever
declare prop: Observable<string>;
}