Skip to content

Serialize inferred type alias when type alias symbol is not accessible #10953

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 8 commits into from
Sep 16, 2016
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
39 changes: 27 additions & 12 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1758,15 +1758,23 @@ namespace ts {
return false;
}

function isSymbolAccessible(symbol: Symbol, enclosingDeclaration: Node, meaning: SymbolFlags): SymbolAccessibilityResult {
/**
* Check if the given symbol in given enclosing declaration is accessible and mark all associated alias to be visible if requested
*
* @param symbol a Symbol to check if accessible
* @param enclosingDeclaration a Node containing reference to the symbol
Copy link
Member

Choose a reason for hiding this comment

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

a Node containing reference to the symbol?

* @param meaning a SymbolFlags to check if such meaning of the symbol is accessible
* @param shouldComputeAliasToMakeVisible a boolean value to indicate whether to return aliases to be mark visible in case the symbol is accessible
*/
function isSymbolAccessible(symbol: Symbol, enclosingDeclaration: Node, meaning: SymbolFlags, shouldComputeAliasesToMakeVisible: boolean): SymbolAccessibilityResult {
if (symbol && enclosingDeclaration && !(symbol.flags & SymbolFlags.TypeParameter)) {
const initialSymbol = symbol;
let meaningToLook = meaning;
while (symbol) {
// Symbol is accessible if it by itself is accessible
const accessibleSymbolChain = getAccessibleSymbolChain(symbol, enclosingDeclaration, meaningToLook, /*useOnlyExternalAliasing*/ false);
if (accessibleSymbolChain) {
const hasAccessibleDeclarations = hasVisibleDeclarations(accessibleSymbolChain[0]);
const hasAccessibleDeclarations = hasVisibleDeclarations(accessibleSymbolChain[0], shouldComputeAliasesToMakeVisible);
if (!hasAccessibleDeclarations) {
return <SymbolAccessibilityResult>{
accessibility: SymbolAccessibility.NotAccessible,
Expand Down Expand Up @@ -1830,7 +1838,7 @@ namespace ts {
return isAmbientModule(declaration) || (declaration.kind === SyntaxKind.SourceFile && isExternalOrCommonJsModule(<SourceFile>declaration));
}

function hasVisibleDeclarations(symbol: Symbol): SymbolVisibilityResult {
function hasVisibleDeclarations(symbol: Symbol, shouldComputeAliasToMakeVisible: boolean): SymbolVisibilityResult {
Copy link
Member

Choose a reason for hiding this comment

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

MarkVisible or MakeVisible?

Copy link
Contributor Author

@yuit yuit Sep 16, 2016

Choose a reason for hiding this comment

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

make :)

let aliasesToMakeVisible: AnyImportSyntax[];
if (forEach(symbol.declarations, declaration => !getIsDeclarationVisible(declaration))) {
return undefined;
Expand All @@ -1846,14 +1854,19 @@ namespace ts {
if (anyImportSyntax &&
!(getModifierFlags(anyImportSyntax) & ModifierFlags.Export) && // import clause without export
isDeclarationVisible(<Declaration>anyImportSyntax.parent)) {
getNodeLinks(declaration).isVisible = true;
if (aliasesToMakeVisible) {
if (!contains(aliasesToMakeVisible, anyImportSyntax)) {
aliasesToMakeVisible.push(anyImportSyntax);
// In function "buildTypeDisplay" where we decide whether to write type-alias or serialize types,
Copy link
Member

Choose a reason for hiding this comment

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

This seems not completely correct. Because You wouldn't be making the alias visible yet returning that the declaration is visible. At the consumer side say result of all this is that the symbol is visible. (not the false case that you have marked) then you definitely going to get error in the .d.ts file because you should have made the alias visible for the correct emit of the declaration file but it isn't going to be emitted.

Copy link
Member

Choose a reason for hiding this comment

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

I think I get it why it works because here you just check if this is visible but the tracking of the symbol does happen again when actually writing the symbol. Probably we need a comment here explaining that.

// we want to just check if type- alias is accessible or not but we don't care about emitting those alias at that time
// since we will do the emitting later in trackSymbol.
if (shouldComputeAliasToMakeVisible) {
getNodeLinks(declaration).isVisible = true;
if (aliasesToMakeVisible) {
if (!contains(aliasesToMakeVisible, anyImportSyntax)) {
aliasesToMakeVisible.push(anyImportSyntax);
}
}
else {
aliasesToMakeVisible = [anyImportSyntax];
}
}
else {
aliasesToMakeVisible = [anyImportSyntax];
}
return true;
}
Expand Down Expand Up @@ -1888,7 +1901,7 @@ namespace ts {
const symbol = resolveName(enclosingDeclaration, (<Identifier>firstIdentifier).text, meaning, /*nodeNotFoundErrorMessage*/ undefined, /*nameArg*/ undefined);

// Verify if the symbol is accessible
return (symbol && hasVisibleDeclarations(symbol)) || <SymbolVisibilityResult>{
return (symbol && hasVisibleDeclarations(symbol, /*shouldComputeAliasToMakeVisible*/ true)) || <SymbolVisibilityResult>{
accessibility: SymbolAccessibility.NotAccessible,
errorSymbolName: getTextOfNode(firstIdentifier),
errorNode: firstIdentifier
Expand Down Expand Up @@ -2163,7 +2176,9 @@ namespace ts {
// The specified symbol flags need to be reinterpreted as type flags
buildSymbolDisplay(type.symbol, writer, enclosingDeclaration, SymbolFlags.Type, SymbolFormatFlags.None, nextFlags);
}
else if (!(flags & TypeFormatFlags.InTypeAlias) && type.flags & (TypeFlags.Anonymous | TypeFlags.UnionOrIntersection) && type.aliasSymbol) {
else if (!(flags & TypeFormatFlags.InTypeAlias) && type.flags & (TypeFlags.Anonymous | TypeFlags.UnionOrIntersection) && type.aliasSymbol &&
isSymbolAccessible(type.aliasSymbol, enclosingDeclaration, SymbolFlags.Type, /*shouldComputeAliasesToMakeVisible*/ false).accessibility === SymbolAccessibility.Accessible) {
// Only write out inferred type with its corresponding type-alias if type-alias is visible
const typeArguments = type.aliasTypeArguments;
writeSymbolTypeReference(type.aliasSymbol, typeArguments, 0, typeArguments ? typeArguments.length : 0, nextFlags);
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/declarationEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ namespace ts {
}

function trackSymbol(symbol: Symbol, enclosingDeclaration?: Node, meaning?: SymbolFlags) {
handleSymbolAccessibilityError(resolver.isSymbolAccessible(symbol, enclosingDeclaration, meaning));
handleSymbolAccessibilityError(resolver.isSymbolAccessible(symbol, enclosingDeclaration, meaning, /*shouldComputeAliasesToMakeVisible*/ true));
recordTypeReferenceDirectivesIfNecessary(resolver.getTypeReferenceDirectivesForSymbol(symbol, meaning));
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2144,7 +2144,7 @@ namespace ts {
writeReturnTypeOfSignatureDeclaration(signatureDeclaration: SignatureDeclaration, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: SymbolWriter): void;
writeTypeOfExpression(expr: Expression, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: SymbolWriter): void;
writeBaseConstructorTypeOfClass(node: ClassLikeDeclaration, enclosingDeclaration: Node, flags: TypeFormatFlags, writer: SymbolWriter): void;
isSymbolAccessible(symbol: Symbol, enclosingDeclaration: Node, meaning: SymbolFlags): SymbolAccessibilityResult;
isSymbolAccessible(symbol: Symbol, enclosingDeclaration: Node, meaning: SymbolFlags, shouldComputeAliasToMarkVisible: boolean): SymbolAccessibilityResult;
isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult;
// Returns the constant value this property access resolves to, or 'undefined' for a non-constant
getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): number;
Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/controlFlowBinaryOrExpression.types
Original file line number Diff line number Diff line change
Expand Up @@ -62,21 +62,21 @@ declare function isHTMLCollection(sourceObj: any): sourceObj is HTMLCollection;
>HTMLCollection : HTMLCollection

type EventTargetLike = {a: string} | HTMLCollection | NodeList;
>EventTargetLike : EventTargetLike
>EventTargetLike : NodeList | HTMLCollection | { a: string; }
>a : string
>HTMLCollection : HTMLCollection
>NodeList : NodeList

var sourceObj: EventTargetLike = <any>undefined;
>sourceObj : EventTargetLike
>EventTargetLike : EventTargetLike
>sourceObj : NodeList | HTMLCollection | { a: string; }
>EventTargetLike : NodeList | HTMLCollection | { a: string; }
><any>undefined : any
>undefined : undefined

if (isNodeList(sourceObj)) {
>isNodeList(sourceObj) : boolean
>isNodeList : (sourceObj: any) => sourceObj is NodeList
>sourceObj : EventTargetLike
>sourceObj : NodeList | HTMLCollection | { a: string; }

sourceObj.length;
>sourceObj.length : number
Expand All @@ -87,7 +87,7 @@ if (isNodeList(sourceObj)) {
if (isHTMLCollection(sourceObj)) {
>isHTMLCollection(sourceObj) : boolean
>isHTMLCollection : (sourceObj: any) => sourceObj is HTMLCollection
>sourceObj : EventTargetLike
>sourceObj : NodeList | HTMLCollection | { a: string; }

sourceObj.length;
>sourceObj.length : number
Expand All @@ -99,7 +99,7 @@ if (isNodeList(sourceObj) || isHTMLCollection(sourceObj)) {
>isNodeList(sourceObj) || isHTMLCollection(sourceObj) : boolean
>isNodeList(sourceObj) : boolean
>isNodeList : (sourceObj: any) => sourceObj is NodeList
>sourceObj : EventTargetLike
>sourceObj : NodeList | HTMLCollection | { a: string; }
>isHTMLCollection(sourceObj) : boolean
>isHTMLCollection : (sourceObj: any) => sourceObj is HTMLCollection
>sourceObj : HTMLCollection | { a: string; }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//// [declarationEmitArrayTypesFromGenericArrayUsage.ts]
interface A extends Array<string> { }


//// [declarationEmitArrayTypesFromGenericArrayUsage.js]


//// [declarationEmitArrayTypesFromGenericArrayUsage.d.ts]
interface A extends Array<string> {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
=== tests/cases/compiler/declarationEmitArrayTypesFromGenericArrayUsage.ts ===
interface A extends Array<string> { }
>A : Symbol(A, Decl(declarationEmitArrayTypesFromGenericArrayUsage.ts, 0, 0))
>Array : Symbol(Array, Decl(lib.d.ts, --, --), Decl(lib.d.ts, --, --))

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
=== tests/cases/compiler/declarationEmitArrayTypesFromGenericArrayUsage.ts ===
interface A extends Array<string> { }
>A : A
>Array : T[]

Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
//// [declarationEmit_bindingPatterns.ts]
//// [declarationEmitBindingPatterns.ts]

const k = ({x: z = 'y'}) => { }

var a;
function f({} = a, [] = a, { p: {} = a} = a) {
}

//// [declarationEmit_bindingPatterns.js]
//// [declarationEmitBindingPatterns.js]
var k = function (_a) {
var _b = _a.x, z = _b === void 0 ? 'y' : _b;
};
Expand All @@ -18,7 +18,7 @@ function f(_a, _b, _c) {
}


//// [declarationEmit_bindingPatterns.d.ts]
//// [declarationEmitBindingPatterns.d.ts]
declare const k: ({x: z}: {
x?: string;
}) => void;
Expand Down
17 changes: 17 additions & 0 deletions tests/baselines/reference/declarationEmitBindingPatterns.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
=== tests/cases/compiler/declarationEmitBindingPatterns.ts ===

const k = ({x: z = 'y'}) => { }
>k : Symbol(k, Decl(declarationEmitBindingPatterns.ts, 1, 5))
>x : Symbol(x)
>z : Symbol(z, Decl(declarationEmitBindingPatterns.ts, 1, 12))

var a;
>a : Symbol(a, Decl(declarationEmitBindingPatterns.ts, 3, 3))

function f({} = a, [] = a, { p: {} = a} = a) {
>f : Symbol(f, Decl(declarationEmitBindingPatterns.ts, 3, 6))
>a : Symbol(a, Decl(declarationEmitBindingPatterns.ts, 3, 3))
>a : Symbol(a, Decl(declarationEmitBindingPatterns.ts, 3, 3))
>a : Symbol(a, Decl(declarationEmitBindingPatterns.ts, 3, 3))
>a : Symbol(a, Decl(declarationEmitBindingPatterns.ts, 3, 3))
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
=== tests/cases/compiler/declarationEmit_bindingPatterns.ts ===
=== tests/cases/compiler/declarationEmitBindingPatterns.ts ===

const k = ({x: z = 'y'}) => { }
>k : ({x: z}: { x?: string; }) => void
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//// [declarationEmit_classMemberNameConflict.ts]
//// [declarationEmitClassMemberNameConflict.ts]

export class C1 {
C1() { } // has to be the same as the class name
Expand Down Expand Up @@ -36,7 +36,7 @@ export class C4 {
}
}

//// [declarationEmit_classMemberNameConflict.js]
//// [declarationEmitClassMemberNameConflict.js]
"use strict";
var C1 = (function () {
function C1() {
Expand Down Expand Up @@ -93,7 +93,7 @@ var C4 = (function () {
exports.C4 = C4;


//// [declarationEmit_classMemberNameConflict.d.ts]
//// [declarationEmitClassMemberNameConflict.d.ts]
export declare class C1 {
C1(): void;
bar(): (t: typeof C1) => void;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
=== tests/cases/compiler/declarationEmitClassMemberNameConflict.ts ===

export class C1 {
>C1 : Symbol(C1, Decl(declarationEmitClassMemberNameConflict.ts, 0, 0))

C1() { } // has to be the same as the class name
>C1 : Symbol(C1.C1, Decl(declarationEmitClassMemberNameConflict.ts, 1, 17))

bar() {
>bar : Symbol(C1.bar, Decl(declarationEmitClassMemberNameConflict.ts, 2, 12))

return function (t: typeof C1) {
>t : Symbol(t, Decl(declarationEmitClassMemberNameConflict.ts, 5, 25))
>C1 : Symbol(C1, Decl(declarationEmitClassMemberNameConflict.ts, 0, 0))

};
}
}

export class C2 {
>C2 : Symbol(C2, Decl(declarationEmitClassMemberNameConflict.ts, 8, 1))

C2: any // has to be the same as the class name
>C2 : Symbol(C2.C2, Decl(declarationEmitClassMemberNameConflict.ts, 10, 17))

bar() {
>bar : Symbol(C2.bar, Decl(declarationEmitClassMemberNameConflict.ts, 11, 11))

return function (t: typeof C2) {
>t : Symbol(t, Decl(declarationEmitClassMemberNameConflict.ts, 14, 25))
>C2 : Symbol(C2, Decl(declarationEmitClassMemberNameConflict.ts, 8, 1))

};
}
}

export class C3 {
>C3 : Symbol(C3, Decl(declarationEmitClassMemberNameConflict.ts, 17, 1))

get C3() { return 0; } // has to be the same as the class name
>C3 : Symbol(C3.C3, Decl(declarationEmitClassMemberNameConflict.ts, 19, 17))

bar() {
>bar : Symbol(C3.bar, Decl(declarationEmitClassMemberNameConflict.ts, 20, 26))

return function (t: typeof C3) {
>t : Symbol(t, Decl(declarationEmitClassMemberNameConflict.ts, 23, 25))
>C3 : Symbol(C3, Decl(declarationEmitClassMemberNameConflict.ts, 17, 1))

};
}
}

export class C4 {
>C4 : Symbol(C4, Decl(declarationEmitClassMemberNameConflict.ts, 26, 1))

set C4(v) { } // has to be the same as the class name
>C4 : Symbol(C4.C4, Decl(declarationEmitClassMemberNameConflict.ts, 28, 17))
>v : Symbol(v, Decl(declarationEmitClassMemberNameConflict.ts, 29, 11))

bar() {
>bar : Symbol(C4.bar, Decl(declarationEmitClassMemberNameConflict.ts, 29, 17))

return function (t: typeof C4) {
>t : Symbol(t, Decl(declarationEmitClassMemberNameConflict.ts, 32, 25))
>C4 : Symbol(C4, Decl(declarationEmitClassMemberNameConflict.ts, 26, 1))

};
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
=== tests/cases/compiler/declarationEmit_classMemberNameConflict.ts ===
=== tests/cases/compiler/declarationEmitClassMemberNameConflict.ts ===

export class C1 {
>C1 : C1
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//// [declarationEmit_classMemberNameConflict2.ts]
//// [declarationEmitClassMemberNameConflict2.ts]

const Bar = 'bar';

Expand All @@ -21,7 +21,7 @@ class Foo {
Hello2 = Hello1;
}

//// [declarationEmit_classMemberNameConflict2.js]
//// [declarationEmitClassMemberNameConflict2.js]
var Bar = 'bar';
var Hello;
(function (Hello) {
Expand All @@ -44,7 +44,7 @@ var Foo = (function () {
}());


//// [declarationEmit_classMemberNameConflict2.d.ts]
//// [declarationEmitClassMemberNameConflict2.d.ts]
declare const Bar: "bar";
declare enum Hello {
World = 0,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
=== tests/cases/compiler/declarationEmitClassMemberNameConflict2.ts ===

const Bar = 'bar';
>Bar : Symbol(Bar, Decl(declarationEmitClassMemberNameConflict2.ts, 1, 5))

enum Hello {
>Hello : Symbol(Hello, Decl(declarationEmitClassMemberNameConflict2.ts, 1, 18))

World
>World : Symbol(Hello.World, Decl(declarationEmitClassMemberNameConflict2.ts, 3, 12))
}

enum Hello1 {
>Hello1 : Symbol(Hello1, Decl(declarationEmitClassMemberNameConflict2.ts, 5, 1))

World1
>World1 : Symbol(Hello1.World1, Decl(declarationEmitClassMemberNameConflict2.ts, 7, 13))
}

class Foo {
>Foo : Symbol(Foo, Decl(declarationEmitClassMemberNameConflict2.ts, 9, 1))

// Same names + string => OK
Bar = Bar;
>Bar : Symbol(Foo.Bar, Decl(declarationEmitClassMemberNameConflict2.ts, 11, 11))
>Bar : Symbol(Bar, Decl(declarationEmitClassMemberNameConflict2.ts, 1, 5))

// Same names + enum => OK
Hello = Hello;
>Hello : Symbol(Foo.Hello, Decl(declarationEmitClassMemberNameConflict2.ts, 13, 14))
>Hello : Symbol(Hello, Decl(declarationEmitClassMemberNameConflict2.ts, 1, 18))

// Different names + enum => OK
Hello2 = Hello1;
>Hello2 : Symbol(Foo.Hello2, Decl(declarationEmitClassMemberNameConflict2.ts, 16, 18))
>Hello1 : Symbol(Hello1, Decl(declarationEmitClassMemberNameConflict2.ts, 5, 1))
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
=== tests/cases/compiler/declarationEmit_classMemberNameConflict2.ts ===
=== tests/cases/compiler/declarationEmitClassMemberNameConflict2.ts ===

const Bar = 'bar';
>Bar : "bar"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//// [tests/cases/compiler/declarationEmit_exportAssignment.ts] ////
//// [tests/cases/compiler/declarationEmitExportAssignment.ts] ////

//// [utils.ts]

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//// [tests/cases/compiler/declarationEmit_exportDeclaration.ts] ////
//// [tests/cases/compiler/declarationEmitExportDeclaration.ts] ////

//// [utils.ts]

Expand Down
Loading