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 functions and ambient classes to merge #32584

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jul 26, 2019

While working on #32372, I've come upon the need for a generally applicable way to serialize a JS-style callable class, potentially with static members which may or may not be types. Today, a class and a function cannot merge (though you can define a static interface with both call and construct signatures, merging a namespace with a const of the type of that static interface is difficult).

This allows one to declare a callable constructor which is also a type-containing namespace in declaration files. For example:

declare function Foo(x: number): Foo.Inst;
declare class Foo {
    constructor(x: string);
}
declare namespace Foo {
    export type Inst = number;
}

const a = new Foo("");
const b = Foo(12);

many builtins, like RegExp or String, actually neatly fit this pattern. 🤷‍♂
The class must be ambient, since an es6 class implementation can never be callable. The function may be implemented. If so, the ambient class definition behaves similarly to an interface that augments a class - it provides additional structure which the implementation is claimed to be conformant to, however the checker does not directly verify it (it augments the externally visible type information the function's signature provides, similarly to an overload).

This has a side bonus in that when the types for a global js file and the original file are merged into the same compilation, the ambient class's member types contextually type the function's prototype assignments (which is shown in a change in one of the tests), which is niiiiiiice.

Addenda/future work: A class merged with a function does not imply a specific this type within the function body - it is still any (and subject to noImplicitAny errors). You could think that the class declaration implies the function's this, but that's not quite right - since the function is still callable, it's this is still quite ambiguous. If you wanted to use the class as the this type in the function body, adding an explicit this: Foo | undefined in the function signature is always an option.

Fixes #2959
Fixes #183

@weswigham
Copy link
Member Author

@sandersn @RyanCavanaugh would you two care to review this for 3.6? The code change is pretty small.

@sandersn sandersn self-assigned this Aug 9, 2019
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Some extraneous error messages to think about, although they might be prohibitively expensive to fix.
Also I'm not sure we should ship this with 3.6, since it (1) doesn't do much without dts-from-js (2) might see changes based on that PR -- that is, it might not be stable (3) isn't a bugfix. I think you said that you'd like to have 3.6 able to understand 3.7-produced d.ts. Let's discuss this in person with @RyanCavanaugh to decide whether to ship this or not.

src/compiler/checker.ts Show resolved Hide resolved
@@ -31650,7 +31665,7 @@ namespace ts {
if (!symbol || !(symbol.flags & SymbolFlags.Function)) {
return false;
}
return !!forEachEntry(getExportsOfSymbol(symbol), p => p.flags & SymbolFlags.Value && isPropertyAccessExpression(p.valueDeclaration));
return !!forEachEntry(getExportsOfSymbol(symbol), p => p.flags & SymbolFlags.Value && p.valueDeclaration && isPropertyAccessExpression(p.valueDeclaration));
Copy link
Member

Choose a reason for hiding this comment

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

so an ambient class sets SymbolFlags.Value too? I assume that's why we have to check p.valueDeclaration now, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ambient classes do, in fact, count as values, since they do, in fact, resolve in an expression. A declaration being ambient just means it's missing a body - doesn't change its meaning in any other way, really.

Copy link
Member Author

Choose a reason for hiding this comment

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

(The value without a value declaration thing shouldn't happen anymore - it's what caused me to do the "Assignment no longer implies Type or Value" in the other PR for @enum I merged last week. The reason I had to add this was because I had a namespace with no value declaration due to an assignment-caused namespace in a test, IIRC.)

Copy link
Member

Choose a reason for hiding this comment

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

So it sounds like you can revert if it you want, or keep it for future safety. Either way is fine with me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather keep it - I always feel assuming valueDeclaration exists is sketchy anyway, and there's minimal overhead to checking it here.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Figure out with @sandersn if a class/function implies that it's block-scoped, since that affects globalThis

@DanielRosenwasser
Copy link
Member

Sounds like var-scoped

@sandersn
Copy link
Member

sandersn commented Aug 9, 2019

No it is block-scoped. Class/namespace merges behave this way today; the merged symbol disappears from globalThis completions in value space because at least one declaration is block-scoped.

@weswigham
Copy link
Member Author

Our code for globalThis members is thus:

                        members.forEach(p => {
                            if (!(p.flags & SymbolFlags.BlockScoped)) {
                                varsOnly.set(p.escapedName, p);
                            }
                        });

where

        BlockScoped = BlockScopedVariable | Class | Enum,

so, just like a class merged with a namespace, it will not appear on globalThis.

And that answer is debatable given our emit - eliding anything with a single block scoped meaning is correct when target >= es6, while including anything with any non-block-scoped meaning would be more correct when target < es6. 🤷‍♂ Our current behavior is in the effort of forward compatibility, I suppose. In any case, nothing new here~

@weswigham
Copy link
Member Author

doesn't do much without dts-from-js

It does plentry - enables new syntax to function without error.

might see changes based on that PR

Nah, there's other changes in that PR to other things, but this part is pretty trivial, which is why I split it out.

isn't a bugfix

That depends - can I interpret #183 as a bug? I, for one, would love to mark a 3-digit issue a closed ❤️ Or better yet, the (closed with no action) #2959 (which was closed "in favor of callable constructors" which aren't a thing and got moved to the inactive proposal bench).

@weswigham weswigham merged commit f2719f9 into microsoft:master Aug 9, 2019
@weswigham weswigham deleted the allow-ambient-classes-and-functions-to-merge branch August 9, 2019 23:10
@gavar
Copy link

gavar commented Nov 1, 2019

how to achieve same functionality in .ts files?

@weswigham
Copy link
Member Author

weswigham commented Nov 1, 2019

Only the class must be ambient, so this works:

declare class Foo {}
function Foo(this: Foo | undefined | void) {
    if (!(this instanceof Foo)) return new Foo();
    // initialize `this` here
    return this;
}

const x = Foo();
const alsox = new Foo();

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Merge ambient function and class declarations with the same name. Suggestion: callable class
4 participants