Skip to content

Ensure parameters are in scope when converting parameter/return types to type nodes #49627

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.

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 4 commits into from
Feb 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
90 changes: 88 additions & 2 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7276,6 +7276,86 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

const expandedParams = getExpandedParameters(signature, /*skipUnionExpanding*/ true)[0];

// For regular function/method declarations, the enclosing declaration will already be signature.declaration,
// so this is a no-op, but for arrow functions and function expressions, the enclosing declaration will be
// the declaration that the arrow function / function expression is assigned to.
//
// If the parameters or return type include "typeof globalThis.paramName", using the wrong scope will lead
// us to believe that we can emit "typeof paramName" instead, even though that would refer to the parameter,
// not the global. Make sure we are in the right scope by changing the enclosingDeclaration to the function.
//
// We can't use the declaration directly; it may be in another file and so we may lose access to symbols
// accessible to the current enclosing declaration, or gain access to symbols not accessible to the current
// enclosing declaration. To keep this chain accurate, insert a fake scope into the chain which makes the
// function's parameters visible.
//
// If the declaration is in a JS file, then we don't need to do this at all, as there are no annotations besides
// JSDoc, which are always outside the function declaration, so are not in the parameter scope.
let cleanup: (() => void) | undefined;
if (
context.enclosingDeclaration
&& signature.declaration
&& signature.declaration !== context.enclosingDeclaration
&& !isInJSFile(signature.declaration)
&& some(expandedParams)
Copy link
Member

Choose a reason for hiding this comment

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

getExpandedParameters looks like it always returns an array of length 1, at least unless the rest type is somehow a 0-constituent union, but I think those are always impossible.

If the check is there just because the types could allow an empty array, maybe it should be an assert in the body of the if instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Check above, and you'll see:

const expandedParams = getExpandedParameters(signature, /*skipUnionExpanding*/ true)[0];

Copy link
Member Author

@jakebailey jakebailey Feb 18, 2023

Choose a reason for hiding this comment

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

(Later I might see if we can avoid constructing a bunch of random arrays we don't need...)

) {
// As a performance optimization, reuse the same fake scope within this chain.
// This is especially needed when we are working on an excessively deep type;
// if we don't do this, then we spend all of our time adding more and more
// scopes that need to be searched in isSymbolAccessible later. Since all we
// really want to do is to mark certain names as unavailable, we can just keep
// all of the names we're introducing in one large table and push/pop from it as
// needed; isSymbolAccessible will walk upward and find the closest "fake" scope,
// which will conveniently report on any and all faked scopes in the chain.
//
// It'd likely be better to store this somewhere else for isSymbolAccessible, but
// since that API _only_ uses the enclosing declaration (and its parents), this is
// seems like the best way to inject names into that search process.
//
// Note that we only check the most immediate enclosingDeclaration; the only place we
// could potentially add another fake scope into the chain is right here, so we don't
// traverse all ancestors.
const existingFakeScope = getNodeLinks(context.enclosingDeclaration).fakeScopeForSignatureDeclaration ? context.enclosingDeclaration : undefined;
Debug.assertOptionalNode(existingFakeScope, isBlock);

const locals = existingFakeScope?.locals ?? createSymbolTable();

let newLocals: __String[] | undefined;
for (const param of expandedParams) {
if (!locals.has(param.escapedName)) {
newLocals = append(newLocals, param.escapedName);
locals.set(param.escapedName, param);
}
}

if (newLocals) {
function removeNewLocals() {
forEach(newLocals, s => locals.delete(s));
}

if (existingFakeScope) {
cleanup = removeNewLocals;
}
else {
// Use a Block for this; the type of the node doesn't matter so long as it
// has locals, and this is cheaper/easier than using a function-ish Node.
const fakeScope = parseNodeFactory.createBlock(emptyArray);
getNodeLinks(fakeScope).fakeScopeForSignatureDeclaration = true;
fakeScope.locals = locals;

const saveEnclosingDeclaration = context.enclosingDeclaration;
setParent(fakeScope, saveEnclosingDeclaration);
context.enclosingDeclaration = fakeScope;

cleanup = () => {
context.enclosingDeclaration = saveEnclosingDeclaration;
removeNewLocals();
};
}
}
}

// If the expanded parameter list had a variadic in a non-trailing position, don't expand it
const parameters = (some(expandedParams, p => p !== expandedParams[expandedParams.length - 1] && !!(getCheckFlags(p) & CheckFlags.RestParameter)) ? signature.parameters : expandedParams).map(parameter => symbolToParameterDeclaration(parameter, context, kind === SyntaxKind.Constructor, options?.privateSymbolVisitor, options?.bundledImports));
const thisParameter = context.flags & NodeBuilderFlags.OmitThisParameter ? undefined : tryGetThisParameterDeclaration(signature, context);
Expand Down Expand Up @@ -7331,6 +7411,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
node.typeArguments = factory.createNodeArray(typeArguments);
}

cleanup?.();
return node;
}

Expand Down Expand Up @@ -7996,13 +8077,17 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return !(getObjectFlags(type) & ObjectFlags.Reference) || !isTypeReferenceNode(existing) || length(existing.typeArguments) >= getMinTypeArgumentCount((type as TypeReference).target.typeParameters);
}

function getEnclosingDeclarationIgnoringFakeScope(enclosingDeclaration: Node) {
return getNodeLinks(enclosingDeclaration).fakeScopeForSignatureDeclaration ? enclosingDeclaration.parent : enclosingDeclaration;
}

/**
* Unlike `typeToTypeNodeHelper`, this handles setting up the `AllowUniqueESSymbolType` flag
* so a `unique symbol` is returned when appropriate for the input symbol, rather than `typeof sym`
*/
function serializeTypeForDeclaration(context: NodeBuilderContext, type: Type, symbol: Symbol, enclosingDeclaration: Node | undefined, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) {
if (!isErrorType(type) && enclosingDeclaration) {
const declWithExistingAnnotation = getDeclarationWithTypeAnnotation(symbol, enclosingDeclaration);
const declWithExistingAnnotation = getDeclarationWithTypeAnnotation(symbol, getEnclosingDeclarationIgnoringFakeScope(enclosingDeclaration));
if (declWithExistingAnnotation && !isFunctionLikeDeclaration(declWithExistingAnnotation) && !isGetAccessorDeclaration(declWithExistingAnnotation)) {
// try to reuse the existing annotation
const existing = getEffectiveTypeAnnotationNode(declWithExistingAnnotation)!;
Expand Down Expand Up @@ -8038,7 +8123,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
function serializeReturnTypeForSignature(context: NodeBuilderContext, type: Type, signature: Signature, includePrivateSymbol?: (s: Symbol) => void, bundled?: boolean) {
if (!isErrorType(type) && context.enclosingDeclaration) {
const annotation = signature.declaration && getEffectiveReturnTypeNode(signature.declaration);
if (!!findAncestor(annotation, n => n === context.enclosingDeclaration) && annotation) {
const enclosingDeclarationIgnoringFakeScope = getEnclosingDeclarationIgnoringFakeScope(context.enclosingDeclaration);
if (!!findAncestor(annotation, n => n === enclosingDeclarationIgnoringFakeScope) && annotation) {
const annotated = getTypeFromTypeNode(annotation);
const thisInstantiated = annotated.flags & TypeFlags.TypeParameter && (annotated as TypeParameter).isThisType ? instantiateType(annotated, signature.mapper) : annotated;
if (thisInstantiated === type && existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount(annotation, type)) {
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5997,6 +5997,7 @@ export interface NodeLinks {
serializedTypes?: Map<string, SerializedTypeEntry>; // Collection of types serialized at this location
decoratorSignature?: Signature; // Signature for decorator as if invoked by the runtime.
parameterInitializerContainsUndefined?: boolean; // True if this is a parameter declaration whose type annotation contains "undefined".
fakeScopeForSignatureDeclaration?: boolean; // True if this is a fake scope injected into an enclosing declaration chain.
}

/** @internal */
Expand Down
170 changes: 170 additions & 0 deletions tests/baselines/reference/declarationEmitGlobalThisPreserved.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
//// [declarationEmitGlobalThisPreserved.ts]
// Adding this makes tooltips fail too.
// declare global {
// namespace isNaN {
// const prop: number;
// }
// }

// Broken inference cases.

export const a1 = (isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN => isNaN;
export const a2 = (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN): typeof globalThis.isNaN => bar ?? isNaN;
export const a3 = (isNaN: number, bar: typeof globalThis.isNaN): typeof globalThis.isNaN => bar;
export const a4 = (isNaN: number): typeof globalThis.isNaN => globalThis.isNaN;

export const aObj = {
a1: (isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN => isNaN,
a2: (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN): typeof globalThis.isNaN => bar ?? isNaN,
a3: (isNaN: number, bar: typeof globalThis.isNaN): typeof globalThis.isNaN => bar,
a4: (isNaN: number): typeof globalThis.isNaN => globalThis.isNaN,
}

export type a4Return = ReturnType<ReturnType<typeof a4>>;
export type a4oReturn = ReturnType<ReturnType<typeof aObj['a4']>>;

export const b1 = (isNaN: typeof globalThis.isNaN) => isNaN;
export const b2 = (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) => bar ?? isNaN;
export const b3 = (isNaN: number, bar: typeof globalThis.isNaN) => bar;
export const b4 = (isNaN: number) => globalThis.isNaN;

export const bObj = {
b1: (isNaN: typeof globalThis.isNaN) => isNaN,
b2: (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) => bar ?? isNaN,
b3: (isNaN: number, bar: typeof globalThis.isNaN) => bar,
b4: (isNaN: number) => globalThis.isNaN,
}

export type b4Return = ReturnType<ReturnType<typeof b4>>;
export type b4oReturn = ReturnType<ReturnType<typeof bObj['b4']>>;

export function c1(isNaN: typeof globalThis.isNaN) { return isNaN }
export function c2(isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) { return bar ?? isNaN }
export function c3(isNaN: number, bar: typeof globalThis.isNaN) { return bar }
export function c4(isNaN: number) { return globalThis.isNaN; }

export const cObj = {
c1(isNaN: typeof globalThis.isNaN) { return isNaN },
c2(isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) { return bar ?? isNaN },
c3(isNaN: number, bar: typeof globalThis.isNaN) { return bar },
c4(isNaN: number) { return globalThis.isNaN; },
}

export type c4Return = ReturnType<ReturnType<typeof c4>>;
export type c4oReturn = ReturnType<ReturnType<typeof cObj['c4']>>;

export function d1() {
const fn = (isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN => isNaN;
return function() { return fn };
}

export function d2() {
const fn = (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN): typeof globalThis.isNaN => bar ?? isNaN;
return function() { return fn };
}

export function d3() {
const fn = (isNaN: number, bar: typeof globalThis.isNaN): typeof globalThis.isNaN => bar;
return function() { return fn };
}

export function d4() {
const fn = (isNaN: number): typeof globalThis.isNaN => globalThis.isNaN;
return function() { return fn };
}

export type d4Return = ReturnType<ReturnType<ReturnType<ReturnType<typeof d4>>>>;

export class A {
method1(isNaN: typeof globalThis.isNaN) { return isNaN }
method2(isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) { return bar ?? isNaN }
method3(isNaN: number, bar: typeof globalThis.isNaN) { return bar }
method4(isNaN: number) { return globalThis.isNaN; }
}

export function fromParameter(isNaN: number, bar: typeof globalThis.isNaN) {
return function() { return { bar } };
}

// Non-inference cases.

export const explicitlyTypedVariable: (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN = (isNaN) => isNaN;

export function explicitlyTypedFunction(isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN {
return isNaN;
};

export type AsObjectProperty = {
isNaN: typeof globalThis.isNaN;
}

export class AsClassProperty {
isNaN?: typeof globalThis.isNaN;
}

export type AsFunctionType = (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;





//// [declarationEmitGlobalThisPreserved.d.ts]
export declare const a1: (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare const a2: (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare const a3: (isNaN: number, bar: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare const a4: (isNaN: number) => typeof globalThis.isNaN;
export declare const aObj: {
a1: (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;
a2: (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) => typeof globalThis.isNaN;
a3: (isNaN: number, bar: typeof globalThis.isNaN) => typeof globalThis.isNaN;
a4: (isNaN: number) => typeof globalThis.isNaN;
};
export type a4Return = ReturnType<ReturnType<typeof a4>>;
export type a4oReturn = ReturnType<ReturnType<typeof aObj['a4']>>;
export declare const b1: (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare const b2: (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare const b3: (isNaN: number, bar: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare const b4: (isNaN: number) => typeof globalThis.isNaN;
export declare const bObj: {
b1: (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;
b2: (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) => typeof globalThis.isNaN;
b3: (isNaN: number, bar: typeof globalThis.isNaN) => typeof globalThis.isNaN;
b4: (isNaN: number) => typeof globalThis.isNaN;
};
export type b4Return = ReturnType<ReturnType<typeof b4>>;
export type b4oReturn = ReturnType<ReturnType<typeof bObj['b4']>>;
export declare function c1(isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN;
export declare function c2(isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN): typeof globalThis.isNaN;
export declare function c3(isNaN: number, bar: typeof globalThis.isNaN): typeof globalThis.isNaN;
export declare function c4(isNaN: number): typeof globalThis.isNaN;
export declare const cObj: {
c1(isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN;
c2(isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN): typeof globalThis.isNaN;
c3(isNaN: number, bar: typeof globalThis.isNaN): typeof globalThis.isNaN;
c4(isNaN: number): typeof globalThis.isNaN;
};
export type c4Return = ReturnType<ReturnType<typeof c4>>;
export type c4oReturn = ReturnType<ReturnType<typeof cObj['c4']>>;
export declare function d1(): () => (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare function d2(): () => (isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare function d3(): () => (isNaN: number, bar: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare function d4(): () => (isNaN: number) => typeof globalThis.isNaN;
export type d4Return = ReturnType<ReturnType<ReturnType<ReturnType<typeof d4>>>>;
export declare class A {
method1(isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN;
method2(isNaN: typeof globalThis.isNaN, bar?: typeof globalThis.isNaN): typeof globalThis.isNaN;
method3(isNaN: number, bar: typeof globalThis.isNaN): typeof globalThis.isNaN;
method4(isNaN: number): typeof globalThis.isNaN;
}
export declare function fromParameter(isNaN: number, bar: typeof globalThis.isNaN): () => {
bar: typeof globalThis.isNaN;
};
export declare const explicitlyTypedVariable: (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;
export declare function explicitlyTypedFunction(isNaN: typeof globalThis.isNaN): typeof globalThis.isNaN;
export type AsObjectProperty = {
isNaN: typeof globalThis.isNaN;
};
export declare class AsClassProperty {
isNaN?: typeof globalThis.isNaN;
}
export type AsFunctionType = (isNaN: typeof globalThis.isNaN) => typeof globalThis.isNaN;
Loading