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

Private properties should still emit type #20979

Closed
AnyhowStep opened this issue Jan 3, 2018 · 4 comments
Closed

Private properties should still emit type #20979

AnyhowStep opened this issue Jan 3, 2018 · 4 comments
Labels
Won't Fix The severity and priority of this issue do not warrant the time or complexity needed to fix it

Comments

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Jan 3, 2018

TypeScript Version: 2.7.0-dev.20171229

Code

class Gen <T> {
    private x : undefined|T;
}

Expected behavior:

declare class Gen<T> {
    private x: undefined | T;
}

Actual behavior:

declare class Gen<T> {
    private x;
}

The problem comes when trying to use such a class from another package.

import {Gen} from "some-package";

let a = new Gen<number>(9001);
let b = new Gen<string>("Hello, world!");

a = b; //OK
b = a; //OK

The assignment is okay because x is any in the generated declaration, making both Gen<number> and Gen<string> assignable to each other, when they shouldn't.


A more concrete example is something like this,

export class Builder <CanBuild extends boolean> {
    private _dummyCanBuild? : CanBuild;
    private amount? : number;
    private constructor (amount? : number) {
        this.amount = amount;
    }
    public static Create () {
        return new Builder<false>();
    }
    public setImportantValue (amount : number) : Builder<true> {
        //Set some important value
        return new Builder<true>(amount);
    }
    public build (this : Builder<true>) : number {
        //Return some stuff
        if (this.amount == null) {
            throw new Error("You should not have been able to call build()");
        }
        return this.amount;
    }
}

It works fine in the package that declares it. But packages that use it will see private _dummyCanBuild : any, making Builder<true> and Builder<false> assignable to each other. This would allow the user of the package to call build() without having called setImportantValue().


@DanielRosenwasser
Copy link
Member

That's pretty gnarly, and admittedly not something I'm sure we discussed back in #1532 and earlier.

@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jan 3, 2018
@AnyhowStep
Copy link
Contributor Author

The temporary fix I use is to just have public readonly _dummyXXX? : GenericType|undefined for classes that need the type-safety, and are being used in other packages.

But this problem had plagued me, on and off, for a pretty long time, before I finally discovered the cause. I never thought to look at the type declaration file before!

I would have this assignable-when-it-should-not-be problem for some projects and not on others, and couldn't see a pattern to reproducibility. So, I never brought it up.

@RyanCavanaugh RyanCavanaugh added Won't Fix The severity and priority of this issue do not warrant the time or complexity needed to fix it and removed In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jan 30, 2018
@RyanCavanaugh
Copy link
Member

We can't really change the emit behavior as it stands because private properties are currently allowed to refer to types which are visible in the source file but not in the .d.ts file, so it'd be a big breaking change to start emitting the type. Due to some architectural issues it's not feasible to only emit the type when a) it's fully "nameable" in the current .d.ts scope and b) contains a reference the generic type parameter.

Without an extant property, we can't properly measure the variance behavior of the generic parameter, so just assuming that Foo<T> has "some" T isn't sufficient either.

If T is only in private positions then the only observable unsoundndess of a wrongly-typed class instance would be passing a Gen<T> to a Gen<U>'s public method and doing cross-instance private access.

This bug is about as old as TypeScript itself and this is the first we've heard of it, so asking the rare cases to add the workaround property (which can be protected BTW) seems good enough. Thanks for reporting it, though.

@AnyhowStep
Copy link
Contributor Author

protected sounds way better.

@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
Won't Fix The severity and priority of this issue do not warrant the time or complexity needed to fix it
Projects
None yet
Development

No branches or pull requests

3 participants