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.

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

Allow all private declarations to be emitted in declaration output #23351

Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Apr 11, 2018

Followup to #22798 the allows any top-level declaration to be made visible if it is required to properly emit another emitted declaration.

Fixes #23127

Makes #15058 redundant.

As a side effect, because we now consider these declarations "visible", we now sometimes use their names in the type baselines.

@weswigham weswigham force-pushed the allow-other-private-declarations branch from 7924e42 to 4bfefba Compare April 11, 2018 22:46
@bluelovers
Copy link
Contributor

looks like this is something i need it lol

@weswigham weswigham force-pushed the allow-other-private-declarations branch from f42e713 to aa71540 Compare May 3, 2018 22:04
@weswigham
Copy link
Member Author

@sheetalkamat Would you like to give this a review? The GH UI won't let me add people T.T

@@ -107,3 +107,4 @@ declare const FilteredThing_base: {
export declare class FilteredThing extends FilteredThing_base {
match(path: string): boolean;
}
export {};
Copy link
Member

Choose a reason for hiding this comment

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

where is this coming from ?

Copy link
Member Author

Choose a reason for hiding this comment

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

declare const FilteredThing_base: {
     new (...args: any[]): {	     new (...args: any[]): {
         match(path: string): boolean;	         match(path: string): boolean;
         thing: number;	         thing: number;
     };	     };
 } & typeof Unmixed;	 } & typeof Unmixed;

doesn't exist at runtime (we just emit it so we have a thing for the extends clause) - this scopes the file so it's actually private.

}
declare class D extends C {
}
export {};
Copy link
Member

Choose a reason for hiding this comment

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

unwanted export {} ?

Copy link
Member Author

Choose a reason for hiding this comment

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

declare class D extends C {
}

isn't exported (but it used in C). To keep it that way, we need to emit export {} to fix the scoping of the file.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

In mostly all of these scenarios the error you are fixing is public -> private so you are just adding private types to file, the scoping of file shouldnt affect by any of these additions.

Copy link
Member Author

@weswigham weswigham May 7, 2018

Choose a reason for hiding this comment

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

No, it is. Our .d.ts scope rules are odd - any export statement makes it a module, but (separately from that) in a .d.ts unless there's an export assignment or export declaration, every top-level declaration is seen at exported, even without an explicit export modifier. AFAIK this was a decision made to reduce the noise in writing declaration files a long time ago.

@bluelovers
Copy link
Contributor

does can fix this code too? and @ts-ignore not work here

function fn(inputRegExp: RegExp, input: string){}

namespace fn
{
	export const S = Symbol('fn');
}

// @ts-ignore
const fn2 = fn;

export = fn2;

Error:(13, 7) TS4025: Exported variable 'fn2' has or is using private name 'fn'.

@@ -3679,7 +3684,14 @@ namespace ts {
return typeParameterNodes;
}

function symbolToTypeNode(symbol: Symbol, context: NodeBuilderContext, meaning: SymbolFlags): TypeQueryNode | TypeReferenceNode | ImportTypeNode {
function getAccessTypeSplitNode(top: IndexedAccessTypeNode): IndexedAccessTypeNode {
Copy link
Member

Choose a reason for hiding this comment

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

What does getAccessTypeSplitNode mean? The name is quite opaque, so a doc comment would be helpful.

@@ -1171,7 +1156,7 @@ namespace ts {
}

function ensureAccessor(node: AccessorDeclaration): PropertyDeclaration | undefined {
const accessors = getAllAccessorDeclarations((node.parent as ClassDeclaration).members, node);
const accessors = resolver.getAllAccessorDeclarations(node);
Copy link
Member

Choose a reason for hiding this comment

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

Why was this needed? If AccessorDeclaration is now synthetic, you could just call getParseTreeNode (which you are still doing in the version in the checker) and use the existing getAllAccessorDeclarations in utilities.ts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Computed property names have differing names and need to merge - the only way to do so is via their symbol.

@weswigham
Copy link
Member Author

@rbuckton anything else?

@weswigham weswigham merged commit 20f9493 into microsoft:master May 10, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
@weswigham weswigham deleted the allow-other-private-declarations branch August 1, 2018 00:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants