Skip to content
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

Jump to bottom

Lookup retained type nodes in the node builder using correct, specific SymbolFlags meaning #57887

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Lookup retained type nodes in the node builder using correct, specific SymbolFlags meaning #57887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 13 additions & 5 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6311,7 +6311,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}

function isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult {
function getMeaningOfEntityNameReference(entityName: EntityNameOrEntityNameExpression): SymbolFlags {
// get symbol of the first identifier of the entityName
let meaning: SymbolFlags;
if (
Expand All @@ -6324,7 +6324,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
else if (
entityName.kind === SyntaxKind.QualifiedName || entityName.kind === SyntaxKind.PropertyAccessExpression ||
entityName.parent.kind === SyntaxKind.ImportEqualsDeclaration
entityName.parent.kind === SyntaxKind.ImportEqualsDeclaration ||
(entityName.parent.kind === SyntaxKind.QualifiedName && (entityName.parent as QualifiedName).left === entityName) ||
(entityName.parent.kind === SyntaxKind.PropertyAccessExpression && (entityName.parent as PropertyAccessExpression).expression === entityName) ||
(entityName.parent.kind === SyntaxKind.ElementAccessExpression && (entityName.parent as ElementAccessExpression).expression === entityName)
Comment on lines +6328 to +6330
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is even a good thought, but could entityName.parent.left/entityName.parent.expression be yet another one of these nodes has a left/expression that satisfies these rules, e.g. does this need to be recursively unwrapped to check for the innermost thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are so if you're the A in A.B, you get a Namespace meaning. Since we always get the leftmost identifier for the lookup, we only ever actually lookup the A. Even in an A.B.C, we only lookup the A. Elsewhere, we use getQualifiedLeftMeaning on the "input" meaning for the whole dotted name to map type/value meaning to a namespace/value meaning, but when we're using this function, we're usually in a state where we don't even have an "input" meaning yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

That this codepath is somewhat bespoke is... unfortunate. It basically has to distill "what meaning is done by a resolveName call where elsewhere in the checker for this node" into a single function, so declaration emit can redo the lookup with alias collection on.

Copy link
Member Author

@weswigham weswigham Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For example, getResolvedSymbol, as used by checkIdentifier (for expression identifiers), uses SymbolFlags.Value | SymbolFlags.ExportValue, while getTypeFromTypeReference uses SymbolFlags.Type* (and SymbolFlags.Namespace for anything left of the last dot), while checkPropertyAccessExpressionOrQualifiedName just expects the LHS namespace to already be resolved and looks up members on it directly, so doesn't concern itself with meanings.

*Unless you're in JSDoc and it gets very complicated with Value meanings mixing in a fallback lookup.

) {
// Left identifier from type reference or TypeAlias
// Entity name of the import declaration
Expand All @@ -6334,7 +6337,11 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// Type Reference or TypeAlias entity = Identifier
meaning = SymbolFlags.Type;
}
return meaning;
}

function isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult {
const meaning = getMeaningOfEntityNameReference(entityName);
const firstIdentifier = getFirstIdentifier(entityName);
const symbol = resolveName(enclosingDeclaration, firstIdentifier.escapedText, meaning, /*nameNotFoundMessage*/ undefined, /*nameArg*/ undefined, /*isUse*/ false);
if (symbol && symbol.flags & SymbolFlags.TypeParameter && meaning & SymbolFlags.Type) {
Expand Down Expand Up @@ -8579,15 +8586,16 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
introducesError = true;
return { introducesError, node };
}
const sym = resolveEntityName(leftmost, SymbolFlags.All, /*ignoreErrors*/ true, /*dontResolveAlias*/ true);
const meaning = getMeaningOfEntityNameReference(node);
const sym = resolveEntityName(leftmost, meaning, /*ignoreErrors*/ true, /*dontResolveAlias*/ true);
if (sym) {
if (isSymbolAccessible(sym, context.enclosingDeclaration, SymbolFlags.All, /*shouldComputeAliasesToMakeVisible*/ false).accessibility !== SymbolAccessibility.Accessible) {
if (isSymbolAccessible(sym, context.enclosingDeclaration, meaning, /*shouldComputeAliasesToMakeVisible*/ false).accessibility !== SymbolAccessibility.Accessible) {
if (!isDeclarationName(node)) {
introducesError = true;
}
}
else {
context.tracker.trackSymbol(sym, context.enclosingDeclaration, SymbolFlags.All);
context.tracker.trackSymbol(sym, context.enclosingDeclaration, meaning);
Copy link
Member Author

Choose a reason for hiding this comment

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

Just as an FYI to anyone who wants to know why this is the fix for the issue, this call here passes the symbol off to the declaration emitter, which then does a isSymbolAccessible with shouldComputeAliasesToMakeVisible set to true (unlike the speculative one above, where it's false), and uses that alias list to determine what to preserve in the output. When the meaning is inaccurate, you get the wrong symbol from the resolveEntityName lookup, and collect the wrong set of aliases.

includePrivateSymbol?.(sym);
}
if (isIdentifier(node)) {
Expand Down
98 changes: 49 additions & 49 deletions tests/baselines/reference/declarationEmitGlobalThisPreserved.types
View file
Open in desktop

Large diffs are not rendered by default.

23 changes: 23 additions & 0 deletions tests/baselines/reference/declarationEmitRetainedAnnotationRetainsImportInOutput.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//// [tests/cases/compiler/declarationEmitRetainedAnnotationRetainsImportInOutput.ts] ////

//// [index.d.ts]
export type Whatever<T> = {x: T};
export declare function something<T>(cb: () => Whatever<T>): Whatever<T>;

//// [index.ts]
import * as E from 'whatever';

export const run = <E>(i: () => E.Whatever<E>): E.Whatever<E> => E.something(i);

//// [index.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.run = void 0;
var E = require("whatever");
var run = function (i) { return E.something(i); };
exports.run = run;


//// [index.d.ts]
import * as E from 'whatever';
export declare const run: <E>(i: () => E.Whatever<E>) => E.Whatever<E>;
37 changes: 37 additions & 0 deletions tests/baselines/reference/declarationEmitRetainedAnnotationRetainsImportInOutput.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
//// [tests/cases/compiler/declarationEmitRetainedAnnotationRetainsImportInOutput.ts] ////

=== node_modules/whatever/index.d.ts ===
export type Whatever<T> = {x: T};
>Whatever : Symbol(Whatever, Decl(index.d.ts, 0, 0))
>T : Symbol(T, Decl(index.d.ts, 0, 21))
>x : Symbol(x, Decl(index.d.ts, 0, 27))
>T : Symbol(T, Decl(index.d.ts, 0, 21))

export declare function something<T>(cb: () => Whatever<T>): Whatever<T>;
>something : Symbol(something, Decl(index.d.ts, 0, 33))
>T : Symbol(T, Decl(index.d.ts, 1, 34))
>cb : Symbol(cb, Decl(index.d.ts, 1, 37))
>Whatever : Symbol(Whatever, Decl(index.d.ts, 0, 0))
>T : Symbol(T, Decl(index.d.ts, 1, 34))
>Whatever : Symbol(Whatever, Decl(index.d.ts, 0, 0))
>T : Symbol(T, Decl(index.d.ts, 1, 34))

=== index.ts ===
import * as E from 'whatever';
>E : Symbol(E, Decl(index.ts, 0, 6))

export const run = <E>(i: () => E.Whatever<E>): E.Whatever<E> => E.something(i);
>run : Symbol(run, Decl(index.ts, 2, 12))
>E : Symbol(E, Decl(index.ts, 2, 20))
>i : Symbol(i, Decl(index.ts, 2, 23))
>E : Symbol(E, Decl(index.ts, 0, 6))
>Whatever : Symbol(E.Whatever, Decl(index.d.ts, 0, 0))
>E : Symbol(E, Decl(index.ts, 2, 20))
>E : Symbol(E, Decl(index.ts, 0, 6))
>Whatever : Symbol(E.Whatever, Decl(index.d.ts, 0, 0))
>E : Symbol(E, Decl(index.ts, 2, 20))
>E.something : Symbol(E.something, Decl(index.d.ts, 0, 33))
>E : Symbol(E, Decl(index.ts, 0, 6))
>something : Symbol(E.something, Decl(index.d.ts, 0, 33))
>i : Symbol(i, Decl(index.ts, 2, 23))

27 changes: 27 additions & 0 deletions tests/baselines/reference/declarationEmitRetainedAnnotationRetainsImportInOutput.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//// [tests/cases/compiler/declarationEmitRetainedAnnotationRetainsImportInOutput.ts] ////

=== node_modules/whatever/index.d.ts ===
export type Whatever<T> = {x: T};
>Whatever : Whatever<T>
>x : T

export declare function something<T>(cb: () => Whatever<T>): Whatever<T>;
>something : <T>(cb: () => Whatever<T>) => Whatever<T>
>cb : () => Whatever<T>

=== index.ts ===
import * as E from 'whatever';
>E : typeof E

export const run = <E>(i: () => E.Whatever<E>): E.Whatever<E> => E.something(i);
>run : <E>(i: () => E.Whatever<E>) => E.Whatever<E>
><E>(i: () => E.Whatever<E>): E.Whatever<E> => E.something(i) : <E>(i: () => E.Whatever<E>) => E.Whatever<E>
>i : () => E.Whatever<E>
>E : any
>E : any
>E.something(i) : E.Whatever<E>
>E.something : <T>(cb: () => E.Whatever<T>) => E.Whatever<T>
>E : typeof E
>something : <T>(cb: () => E.Whatever<T>) => E.Whatever<T>
>i : () => E.Whatever<E>

2 changes: 1 addition & 1 deletion tests/baselines/reference/intraExpressionInferences.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ example({
// Repro from #45255

declare const branch:
>branch : <T, U extends T>(_: { test: T; if: (t: T) => t is U; then: (u: U) => void; }) => void
>branch : <T, U extends T>(_: { test: T; if: (t: T) => t is U; then: (u: U) => void;}) => void

<T, U extends T>(_: { test: T, if: (t: T) => t is U, then: (u: U) => void }) => void
>_ : { test: T; if: (t: T) => t is U; then: (u: U) => void; }
Expand Down
11 changes: 8 additions & 3 deletions tests/baselines/reference/jsDeclarationsImportAliasExposedWithinNamespace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,15 @@ export namespace myTypes {
prop2: string;
};
type typeC = myTypes.typeB | Function;
let myTypes: {
[x: string]: any;
};
}
/**
* @namespace myTypes
* @global
* @type {Object<string,*>}
*/
export const myTypes: {
[x: string]: any;
};
//// [file2.d.ts]
export namespace testFnTypes {
type input = boolean | myTypes.typeC;
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/reverseMappedUnionInference.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ interface Extractor<T, Result> {
}

declare function createExtractor<T, Result>(params: {
>createExtractor : <T, Result>(params: { matcher: (node: unknown) => node is T; extract: (node: T) => Result; }) => Extractor<T, Result>
>createExtractor : <T, Result>(params: { matcher: (node: unknown) => node is T; extract: (node: T) => Result;}) => Extractor<T, Result>
>params : { matcher: (node: unknown) => node is T; extract: (node: T) => Result; }

matcher: (node: unknown) => node is T;
Expand Down
10 changes: 10 additions & 0 deletions tests/cases/compiler/declarationEmitRetainedAnnotationRetainsImportInOutput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// @strict: true
// @declaration: true
// @filename: node_modules/whatever/index.d.ts
export type Whatever<T> = {x: T};
export declare function something<T>(cb: () => Whatever<T>): Whatever<T>;

// @filename: index.ts
import * as E from 'whatever';

export const run = <E>(i: () => E.Whatever<E>): E.Whatever<E> => E.something(i);