Skip to content

Clone signature return type nodes if they are present and yield the same type as the signature return type #23296

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

Closed
wants to merge 3 commits into from
Closed

Clone signature return type nodes if they are present and yield the same type as the signature return type #23296

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
8894c8d
Clone signature return type nodes if they are present and yield the s…
weswigham Apr 10, 2018
714e4c0
Fix lint
weswigham Apr 10, 2018
3b12735
Update tests
weswigham Apr 11, 2018
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
51 changes: 48 additions & 3 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2797,7 +2797,7 @@ namespace ts {
}
}

function isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult {
function resolveNameFromEntityName(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we already have a resolveEntityName function?

Copy link
Member Author

Choose a reason for hiding this comment

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

This skips a lot of the work resolveEntityName does to verify namespaceyness and also generates an appropriate meaning from the entityName's parent. So they're a bit different, unfortunately.

Copy link
Member

Choose a reason for hiding this comment

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

We either should have only one function with options to control behavior (preferred), or should clearly document the difference between the two.

// get symbol of the first identifier of the entityName
let meaning: SymbolFlags;
if (entityName.parent.kind === SyntaxKind.TypeQuery ||
Expand All @@ -2819,9 +2819,14 @@ namespace ts {

const firstIdentifier = getFirstIdentifier(entityName);
const symbol = resolveName(enclosingDeclaration, firstIdentifier.escapedText, meaning, /*nodeNotFoundErrorMessage*/ undefined, /*nameArg*/ undefined, /*isUse*/ false);
return symbol;
}

function isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node, shouldComputeAliasToMakeVisible = true): SymbolVisibilityResult {
const symbol = resolveNameFromEntityName(entityName, enclosingDeclaration);
const firstIdentifier = getFirstIdentifier(entityName);
// Verify if the symbol is accessible
return (symbol && hasVisibleDeclarations(symbol, /*shouldComputeAliasToMakeVisible*/ true)) || {
return (symbol && hasVisibleDeclarations(symbol, shouldComputeAliasToMakeVisible)) || {
accessibility: SymbolAccessibility.NotAccessible,
errorSymbolName: getTextOfNode(firstIdentifier),
errorNode: firstIdentifier
Expand Down Expand Up @@ -3473,7 +3478,14 @@ namespace ts {
}
else {
const returnType = getReturnTypeOfSignature(signature);
returnTypeNode = returnType && typeToTypeNodeHelper(returnType, context);
const originalReturnTypeNode = signature.declaration && getEffectiveReturnTypeNode(signature.declaration);
// If there's a return type node on the signature and it will resolve the same way in the new context, we should reuse it, rather than build a new one
if (originalReturnTypeNode && returnType && !(originalReturnTypeNode.flags & NodeFlags.JSDoc) && getTypeFromTypeNode(originalReturnTypeNode) === returnType && !forEachChild(originalReturnTypeNode, checkEmbdeddedReferencesInaccesible)) {
returnTypeNode = getDeepSynthesizedCloneAndPaintVisibility(originalReturnTypeNode);
}
else {
returnTypeNode = returnType && typeToTypeNodeHelper(returnType, context);
}
}
if (context.flags & NodeBuilderFlags.SuppressAnyReturnType) {
if (returnTypeNode && returnTypeNode.kind === SyntaxKind.AnyKeyword) {
Expand All @@ -3484,6 +3496,39 @@ namespace ts {
returnTypeNode = createKeywordTypeNode(SyntaxKind.AnyKeyword);
}
return createSignatureDeclaration(kind, typeParameters, parameters, returnTypeNode, typeArguments);

function getDeepSynthesizedCloneAndPaintVisibility<T extends Node>(node: T | undefined): T | undefined {
if (isEntityName(node) || isEntityNameExpression(node)) {
// Paint visible aliases required for copied nodes (so required imports are not elided)
isEntityNameVisible(node, context.enclosingDeclaration);
Copy link
Member

Choose a reason for hiding this comment

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

Calling isEntityNameVisible without using the result is odd. I understand what this is doing, but perhaps its better to split isEntityNameVisible into two functions: one that tracks visibility and one that uses the result.

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'd just be hasVisibleDeclarations(resolveNameFromEntityName(entityName, enclosingDeclaration), /*shouldComputeAliasToMakeVisible*/ true), I suppose?

}
if (isIdentifier(node)) {
const clone = getSynthesizedClone(node);
const typeAt = getTypeOfNode(node);
if (typeAt && typeAt.symbol) {
clone.symbol = typeAt.symbol; // Used by quickinfo for richer metadata, would be set by `symbolToName` if not reusing input nodes
}
return clone;
}
return visitEachChild(node, getDeepSynthesizedCloneAndPaintVisibility, nullTransformationContext);
}

function checkEmbdeddedReferencesInaccesible<T extends Node>(node: T): boolean {
if (isEntityName(node) || isEntityNameExpression(node)) {
// Check that the name resolves to the same thing in both locations
const originalTarget = resolveNameFromEntityName(node, node);
const potentialTarget = resolveNameFromEntityName(node, context.enclosingDeclaration);
if (originalTarget !== potentialTarget) {
return true;
}
// And that the symbol it resolves to is accessible
const result = isEntityNameVisible(node, context.enclosingDeclaration, /*shouldComputeAliasesToMakeVisible*/ false);
if (result.accessibility !== SymbolAccessibility.Accessible) {
return true;
}
}
return forEachChild(node, checkEmbdeddedReferencesInaccesible);
}
}

function typeParameterToDeclaration(type: TypeParameter, context: NodeBuilderContext, constraint = getConstraintFromTypeParameter(type)): TypeParameterDeclaration {
Expand Down
4 changes: 1 addition & 3 deletions src/harness/fourslash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2240,9 +2240,7 @@ Actual: ${stringify(fullActual)}`);

public verifyCurrentFileContent(text: string) {
const actual = this.getFileContent(this.activeFile.fileName);
if (actual !== text) {
throw new Error(`verifyCurrentFileContent failed:\n${showTextDiff(text, actual)}`);
}
assert.equal(text, actual);
}

public verifyTextAtCaretIs(text: string) {
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/assignmentCompatWithCallSignatures3.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
Type 'T' is not assignable to type '{ foo: string; bar: string; }'.
Type 'Base' is not assignable to type '{ foo: string; bar: string; }'.
Property 'bar' is missing in type 'Base'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithCallSignatures3.ts(80,1): error TS2322: Type '(x: Base[], y: Derived2[]) => Derived[]' is not assignable to type '<T extends Base[]>(x: Base[], y: T) => Derived[]'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithCallSignatures3.ts(80,1): error TS2322: Type '(x: Base[], y: Derived2[]) => Array<Derived>' is not assignable to type '<T extends Base[]>(x: Base[], y: T) => Array<Derived>'.
Types of parameters 'y' and 'y' are incompatible.
Type 'T' is not assignable to type 'Derived2[]'.
Type 'Base[]' is not assignable to type 'Derived2[]'.
Type 'Base' is not assignable to type 'Derived2'.
Property 'baz' is missing in type 'Base'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithCallSignatures3.ts(83,1): error TS2322: Type '(x: Base[], y: Derived[]) => Derived[]' is not assignable to type '<T extends Derived[]>(x: Base[], y: T) => T'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithCallSignatures3.ts(83,1): error TS2322: Type '(x: Base[], y: Derived[]) => Array<Derived>' is not assignable to type '<T extends Derived[]>(x: Base[], y: T) => T'.
Type 'Derived[]' is not assignable to type 'T'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithCallSignatures3.ts(86,1): error TS2322: Type '(x: { a: string; b: number; }) => Object' is not assignable to type '<T>(x: { a: T; b: T; }) => T'.
Types of parameters 'x' and 'x' are incompatible.
Expand Down Expand Up @@ -184,7 +184,7 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
a12 = b12; // ok
b12 = a12; // ok
~~~
!!! error TS2322: Type '(x: Base[], y: Derived2[]) => Derived[]' is not assignable to type '<T extends Base[]>(x: Base[], y: T) => Derived[]'.
!!! error TS2322: Type '(x: Base[], y: Derived2[]) => Array<Derived>' is not assignable to type '<T extends Base[]>(x: Base[], y: T) => Array<Derived>'.
!!! error TS2322: Types of parameters 'y' and 'y' are incompatible.
!!! error TS2322: Type 'T' is not assignable to type 'Derived2[]'.
!!! error TS2322: Type 'Base[]' is not assignable to type 'Derived2[]'.
Expand All @@ -194,7 +194,7 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
a13 = b13; // ok
b13 = a13; // ok
~~~
!!! error TS2322: Type '(x: Base[], y: Derived[]) => Derived[]' is not assignable to type '<T extends Derived[]>(x: Base[], y: T) => T'.
!!! error TS2322: Type '(x: Base[], y: Derived[]) => Array<Derived>' is not assignable to type '<T extends Derived[]>(x: Base[], y: T) => T'.
!!! error TS2322: Type 'Derived[]' is not assignable to type 'T'.
var b14: <T>(x: { a: T; b: T }) => T;
a14 = b14; // ok
Expand Down
24 changes: 12 additions & 12 deletions tests/baselines/reference/assignmentCompatWithCallSignatures3.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ var a11: (x: { foo: string }, y: { foo: string; bar: string }) => Base;
>Base : Base

var a12: (x: Array<Base>, y: Array<Derived2>) => Array<Derived>;
>a12 : (x: Base[], y: Derived2[]) => Derived[]
>a12 : (x: Base[], y: Derived2[]) => Array<Derived>
>x : Base[]
>Array : T[]
>Base : Base
Expand All @@ -115,7 +115,7 @@ var a12: (x: Array<Base>, y: Array<Derived2>) => Array<Derived>;
>Derived : Derived

var a13: (x: Array<Base>, y: Array<Derived>) => Array<Derived>;
>a13 : (x: Base[], y: Derived[]) => Derived[]
>a13 : (x: Base[], y: Derived[]) => Array<Derived>
>x : Base[]
>Array : T[]
>Base : Base
Expand Down Expand Up @@ -427,7 +427,7 @@ b11 = a11; // ok
>a11 : (x: { foo: string; }, y: { foo: string; bar: string; }) => Base

var b12: <T extends Array<Base>>(x: Array<Base>, y: T) => Array<Derived>;
>b12 : <T extends Base[]>(x: Base[], y: T) => Derived[]
>b12 : <T extends Base[]>(x: Base[], y: T) => Array<Derived>
>T : T
>Array : T[]
>Base : Base
Expand All @@ -440,14 +440,14 @@ var b12: <T extends Array<Base>>(x: Array<Base>, y: T) => Array<Derived>;
>Derived : Derived

a12 = b12; // ok
>a12 = b12 : <T extends Base[]>(x: Base[], y: T) => Derived[]
>a12 : (x: Base[], y: Derived2[]) => Derived[]
>b12 : <T extends Base[]>(x: Base[], y: T) => Derived[]
>a12 = b12 : <T extends Base[]>(x: Base[], y: T) => Array<Derived>
>a12 : (x: Base[], y: Derived2[]) => Array<Derived>
>b12 : <T extends Base[]>(x: Base[], y: T) => Array<Derived>

b12 = a12; // ok
>b12 = a12 : (x: Base[], y: Derived2[]) => Derived[]
>b12 : <T extends Base[]>(x: Base[], y: T) => Derived[]
>a12 : (x: Base[], y: Derived2[]) => Derived[]
>b12 = a12 : (x: Base[], y: Derived2[]) => Array<Derived>
>b12 : <T extends Base[]>(x: Base[], y: T) => Array<Derived>
>a12 : (x: Base[], y: Derived2[]) => Array<Derived>

var b13: <T extends Array<Derived>>(x: Array<Base>, y: T) => T;
>b13 : <T extends Derived[]>(x: Base[], y: T) => T
Expand All @@ -463,13 +463,13 @@ var b13: <T extends Array<Derived>>(x: Array<Base>, y: T) => T;

a13 = b13; // ok
>a13 = b13 : <T extends Derived[]>(x: Base[], y: T) => T
>a13 : (x: Base[], y: Derived[]) => Derived[]
>a13 : (x: Base[], y: Derived[]) => Array<Derived>
>b13 : <T extends Derived[]>(x: Base[], y: T) => T

b13 = a13; // ok
>b13 = a13 : (x: Base[], y: Derived[]) => Derived[]
>b13 = a13 : (x: Base[], y: Derived[]) => Array<Derived>
>b13 : <T extends Derived[]>(x: Base[], y: T) => T
>a13 : (x: Base[], y: Derived[]) => Derived[]
>a13 : (x: Base[], y: Derived[]) => Array<Derived>

var b14: <T>(x: { a: T; b: T }) => T;
>b14 : <T>(x: { a: T; b: T; }) => T
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/assignmentCompatWithConstructSignatures3.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
Type 'T' is not assignable to type '{ foo: string; bar: string; }'.
Type 'Base' is not assignable to type '{ foo: string; bar: string; }'.
Property 'bar' is missing in type 'Base'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithConstructSignatures3.ts(80,1): error TS2322: Type 'new (x: Base[], y: Derived2[]) => Derived[]' is not assignable to type 'new <T extends Base[]>(x: Base[], y: T) => Derived[]'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithConstructSignatures3.ts(80,1): error TS2322: Type 'new (x: Base[], y: Derived2[]) => Array<Derived>' is not assignable to type 'new <T extends Base[]>(x: Base[], y: T) => Array<Derived>'.
Types of parameters 'y' and 'y' are incompatible.
Type 'T' is not assignable to type 'Derived2[]'.
Type 'Base[]' is not assignable to type 'Derived2[]'.
Type 'Base' is not assignable to type 'Derived2'.
Property 'baz' is missing in type 'Base'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithConstructSignatures3.ts(83,1): error TS2322: Type 'new (x: Base[], y: Derived[]) => Derived[]' is not assignable to type 'new <T extends Derived[]>(x: Base[], y: T) => T'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithConstructSignatures3.ts(83,1): error TS2322: Type 'new (x: Base[], y: Derived[]) => Array<Derived>' is not assignable to type 'new <T extends Derived[]>(x: Base[], y: T) => T'.
Type 'Derived[]' is not assignable to type 'T'.
tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignmentCompatWithConstructSignatures3.ts(86,1): error TS2322: Type 'new (x: { a: string; b: number; }) => Object' is not assignable to type 'new <T>(x: { a: T; b: T; }) => T'.
Types of parameters 'x' and 'x' are incompatible.
Expand Down Expand Up @@ -184,7 +184,7 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
a12 = b12; // ok
b12 = a12; // ok
~~~
!!! error TS2322: Type 'new (x: Base[], y: Derived2[]) => Derived[]' is not assignable to type 'new <T extends Base[]>(x: Base[], y: T) => Derived[]'.
!!! error TS2322: Type 'new (x: Base[], y: Derived2[]) => Array<Derived>' is not assignable to type 'new <T extends Base[]>(x: Base[], y: T) => Array<Derived>'.
!!! error TS2322: Types of parameters 'y' and 'y' are incompatible.
!!! error TS2322: Type 'T' is not assignable to type 'Derived2[]'.
!!! error TS2322: Type 'Base[]' is not assignable to type 'Derived2[]'.
Expand All @@ -194,7 +194,7 @@ tests/cases/conformance/types/typeRelationships/assignmentCompatibility/assignme
a13 = b13; // ok
b13 = a13; // ok
~~~
!!! error TS2322: Type 'new (x: Base[], y: Derived[]) => Derived[]' is not assignable to type 'new <T extends Derived[]>(x: Base[], y: T) => T'.
!!! error TS2322: Type 'new (x: Base[], y: Derived[]) => Array<Derived>' is not assignable to type 'new <T extends Derived[]>(x: Base[], y: T) => T'.
!!! error TS2322: Type 'Derived[]' is not assignable to type 'T'.
var b14: new <T>(x: { a: T; b: T }) => T;
a14 = b14; // ok
Expand Down
24 changes: 12 additions & 12 deletions tests/baselines/reference/assignmentCompatWithConstructSignatures3.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ var a11: new (x: { foo: string }, y: { foo: string; bar: string }) => Base;
>Base : Base

var a12: new (x: Array<Base>, y: Array<Derived2>) => Array<Derived>;
>a12 : new (x: Base[], y: Derived2[]) => Derived[]
>a12 : new (x: Base[], y: Derived2[]) => Array<Derived>
>x : Base[]
>Array : T[]
>Base : Base
Expand All @@ -115,7 +115,7 @@ var a12: new (x: Array<Base>, y: Array<Derived2>) => Array<Derived>;
>Derived : Derived

var a13: new (x: Array<Base>, y: Array<Derived>) => Array<Derived>;
>a13 : new (x: Base[], y: Derived[]) => Derived[]
>a13 : new (x: Base[], y: Derived[]) => Array<Derived>
>x : Base[]
>Array : T[]
>Base : Base
Expand Down Expand Up @@ -427,7 +427,7 @@ b11 = a11; // ok
>a11 : new (x: { foo: string; }, y: { foo: string; bar: string; }) => Base

var b12: new <T extends Array<Base>>(x: Array<Base>, y: T) => Array<Derived>;
>b12 : new <T extends Base[]>(x: Base[], y: T) => Derived[]
>b12 : new <T extends Base[]>(x: Base[], y: T) => Array<Derived>
>T : T
>Array : T[]
>Base : Base
Expand All @@ -440,14 +440,14 @@ var b12: new <T extends Array<Base>>(x: Array<Base>, y: T) => Array<Derived>;
>Derived : Derived

a12 = b12; // ok
>a12 = b12 : new <T extends Base[]>(x: Base[], y: T) => Derived[]
>a12 : new (x: Base[], y: Derived2[]) => Derived[]
>b12 : new <T extends Base[]>(x: Base[], y: T) => Derived[]
>a12 = b12 : new <T extends Base[]>(x: Base[], y: T) => Array<Derived>
>a12 : new (x: Base[], y: Derived2[]) => Array<Derived>
>b12 : new <T extends Base[]>(x: Base[], y: T) => Array<Derived>

b12 = a12; // ok
>b12 = a12 : new (x: Base[], y: Derived2[]) => Derived[]
>b12 : new <T extends Base[]>(x: Base[], y: T) => Derived[]
>a12 : new (x: Base[], y: Derived2[]) => Derived[]
>b12 = a12 : new (x: Base[], y: Derived2[]) => Array<Derived>
>b12 : new <T extends Base[]>(x: Base[], y: T) => Array<Derived>
>a12 : new (x: Base[], y: Derived2[]) => Array<Derived>

var b13: new <T extends Array<Derived>>(x: Array<Base>, y: T) => T;
>b13 : new <T extends Derived[]>(x: Base[], y: T) => T
Expand All @@ -463,13 +463,13 @@ var b13: new <T extends Array<Derived>>(x: Array<Base>, y: T) => T;

a13 = b13; // ok
>a13 = b13 : new <T extends Derived[]>(x: Base[], y: T) => T
>a13 : new (x: Base[], y: Derived[]) => Derived[]
>a13 : new (x: Base[], y: Derived[]) => Array<Derived>
>b13 : new <T extends Derived[]>(x: Base[], y: T) => T

b13 = a13; // ok
>b13 = a13 : new (x: Base[], y: Derived[]) => Derived[]
>b13 = a13 : new (x: Base[], y: Derived[]) => Array<Derived>
>b13 : new <T extends Derived[]>(x: Base[], y: T) => T
>a13 : new (x: Base[], y: Derived[]) => Derived[]
>a13 : new (x: Base[], y: Derived[]) => Array<Derived>

var b14: new <T>(x: { a: T; b: T }) => T;
>b14 : new <T>(x: { a: T; b: T; }) => T
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncAliasReturnType_es5.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ type PromiseAlias<T> = Promise<T>;
>T : T

async function f(): PromiseAlias<void> {
>f : () => Promise<void>
>f : () => PromiseAlias<void>
>PromiseAlias : Promise<T>
}
2 changes: 1 addition & 1 deletion tests/baselines/reference/asyncAliasReturnType_es6.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ type PromiseAlias<T> = Promise<T>;
>T : T

async function f(): PromiseAlias<void> {
>f : () => Promise<void>
>f : () => PromiseAlias<void>
>PromiseAlias : Promise<T>
}
Loading