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

Declaration-emit class expressions as type literals #15932

Merged
merged 5 commits into from
May 22, 2017

Conversation

sandersn
Copy link
Member

Fixes #15066, although the resulting d.ts file is a bit ugly. After discussion, @mhegazy and I decided that adding a new "named class expression" type would not be enough to solve the problem on its own, making any complete solution extremely complex. Instead, this change just writes out class expressions as object types, resulting in types like this:

// @Filename: a.ts
class B { b; }
function f() {
    return class extends B { static d; }
}
// @Filename: a.d.ts
declare class B { b: any; }
declare function f(): {
  new(): { b: any };
  prototype: { b: any };
  d: any;
}

Notes:

  1. Circular references bottom out as any.
  2. This happens for all type writing, not just declaration emit, but this is probably a slightly improvement overall.

sandersn added 2 commits May 18, 2017 09:10
This works pretty well. Note that circular references bottom out as
`any`. Right now this happens for all type writing, not just for
declaration emit, but this is probably an improvement on average.
new (): {
tags(): void;
};
prototype: {
Copy link
Contributor

Choose a reason for hiding this comment

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

kinda weird that we keep this property. can we filter prototype out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -3,4 +3,4 @@
////var [|{| "isWriteAccess": true, "isDefinition": true |}Base|] = class { };
////class C extends [|Base|] { }

verify.singleReferenceGroup("var Base: typeof Base");
verify.singleReferenceGroup("var Base: {\n new (): {};\n prototype: {};\n}");
Copy link
Member

Choose a reason for hiding this comment

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

If there is a way to keep this as typeof Base it would be better I think. The completionlist/quickinfoes would get unreadable with this change otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}

export class Test extends WithTags(FooItem) {}

Copy link
Member

Choose a reason for hiding this comment

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

can you please add example like:

class Base {
}
export function foo() {
return class extends Base {
}
}

This should report error that Base is private and used here.

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 tried adding an ad-hoc check, but I couldn't get it to distinguish between this case, where the class expression extends a class declaration, and the mixin case, where the class expression extends a parameter. There are two problems with the latter case:

  1. The mixed-in class could easily be a non-exported class declaration.
  2. Class declarations that call the mix-in are no longer enclosed in the mix-in function, making it difficult to track back to the original class expression's context in the displayType machinery.

This export will not cause errors in any case, because the class expression is exported as a type literal, so Base isn't actually referenced — just its contents.

tl;dr — I don't think it's worthwhile to issue an error here.

// @declaration: true
export var simpleExample = class {
static getTags() { }
tags() { }
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add private member to this? Are we suppose to see private member in the type literal? If yes we should not be writing type of that private property/method because then it would likely start showing errors (with inaccessible symbol)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test, and removed privates from declaration emit.

I am still working on the error for class expressions that extend from a
non-exported base.
@@ -3459,6 +3479,11 @@ namespace ts {
buildIndexSignatureDisplay(resolved.stringIndexInfo, writer, IndexKind.String, enclosingDeclaration, globalFlags, symbolStack);
buildIndexSignatureDisplay(resolved.numberIndexInfo, writer, IndexKind.Number, enclosingDeclaration, globalFlags, symbolStack);
for (const p of resolved.properties) {
if (globalFlags & TypeFormatFlags.WriteClassExpressionAsTypeLiteral &&
(p.name === "prototype" ||
Copy link
Member

Choose a reason for hiding this comment

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

how about instead checking SymbolFlags.Prototype

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

@sandersn
Copy link
Member Author

All right, I think this change is ready to go in now. @mhegazy mind looking at the latest changes to emit errors for private properties that are part of exported class expressions?

@sandersn sandersn merged commit bc914c0 into master May 22, 2017
@sandersn sandersn deleted the decl-emit-class-expr-as-type-literal branch May 22, 2017 22:46
@ghost ghost mentioned this pull request Aug 14, 2017
@jtlapp
Copy link

jtlapp commented Jan 29, 2018

Is it possible to apply this solution to mixins that use generics? I have the following mixins working, as long as I don't try to use them from within another module:

// broker.ts

export class BasicBroker<E> {
    doSomethingBasic(): E {
        //...
    }
}

export function SpecialBrokerMixin<E>() {
    return function <B extends Constructor<{}>>(Base: B) {
        // allows mixins to use decorators and be declared abstract
        class WithSpecialBroker extends Base {
            doSomethingSpecial(): E {
                //...
            }
        }
        return WithSpecialBroker;
    }
}

export function BasicSpecialBroker<E>() {
    // Can't pass a generic to a function. so make concrete first.
    class ConcreteBaseBroker extends BasicBroker<E> { }
    return SpecialBrokerMixin<E>()(ConcreteBaseBroker);
}

Here's what happens if I try to import them:

// app.ts

class AppEntity {
    //...
}

// Error TS4020: 'extends' clause of exported class 'AppBroker' has or is using private name 'ConcreteBaseBroker'.
// Error TS4020: 'extends' clause of exported class 'AppBroker' has or is using private name 'WithSpecialBroker'.

export class AppBroker extends BasicSpecialBroker<AppEntity>() {
    //...
}

I don't see how to define either ConcreteBaseBroker or WithSpecialBroker in ambient typings.

The following doesn't work, giving me the same errors, even in dev 2.8.0:

// broker.d.ts

export interface Constructor<C> {
    new (...args: any[]): C;
    prototype: C;
}

export interface BasicBroker<E> {
    doSomethingBasic(): E;
}

export interface BasicSpecialBroker<E> extends BasicBroker<E> {
    doSomethingSpecial(): E;
}

declare function BasicSpecialBrokerMixin<E>(): () => Constructor<BasicSpecialBroker<E>>;

@jtlapp
Copy link

jtlapp commented Jan 29, 2018

I found the problem. The compiler --declaration option appears to break things. Everything works if I turn off that option. Should that be a bug report?

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
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.

5 participants