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 private/protected properties in keyof operator WHEN it is used within the owning class #13543

Closed
marty-wang opened this Issue Jan 17, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@marty-wang
Copy link
Member

marty-wang commented Jan 17, 2017

TypeScript Version: 2.1.1 / nightly (2.2.0-dev.201xxxxx)

Code

// A *self-contained* demonstration of the problem follows...

const foo = <T, P extends keyof T>(obj: T, prop: P) => {}

class A {
    public pub() {
        this._pri; // this is OK
        foo(this, "_pri") // why is this not?
    }

    private _pri() {
    }
}

Expected behavior:

foo(this, "_pri") is expected to work.

The argument is that since this can access the private methods within its class, the keyof operator should also be able to expose private properties to it (this) within the class. Basically the property visibility is the same.

Actual behavior:
foo(this, "_pri") does not work within the class

@HerringtonDarkholme

This comment has been minimized.

Copy link
Contributor

HerringtonDarkholme commented Jan 18, 2017

May I ask for a real world usage for this feature?
Since private/protected members usually does not need reflection. You can manually list all private/protected members to assign/modify in its own class.

Also, this change probably conflicts with future proposal.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jan 18, 2017

I'm not sure this is coherent. Types really need to be the same regardless of where they're "seen" from. Let's say you had code like this:

class X {
  private m;
  foo(): keyof X {
    return 'm'; // Must be OK?
  }
}
let x = new X();
// ... error? Cannot convert 'keyof X' to 'keyof X' ? Or what?
let m: keyof X = x.foo();
// true, but we would flag this as impossible?
if (m === 'm') {  }
@marty-wang

This comment has been minimized.

Copy link
Member Author

marty-wang commented Jan 18, 2017

@RyanCavanaugh

In your example, the compiler can complain when private 'm' are returned from the public 'foo' method.

In my view, the result of keyof operator is basically the union of the properties of the given type. Since the visibility of the properties are inherently scoped, the union of the properties should be scoped accordingly. It should be consistent that, within any scope if obj.someProp works, the obj should access the someProp via keyof operator as well.

@HerringtonDarkholme

The real world usage I have is the following.

In the React world, it is common practice that you create a self bound method to the instance within the constructor

class Foo {
    constructor() {
        this._pri = this._pri.bind(this);
    }
    private _pri() {
    }
}

It easily becomes tedious when there are many methods that are needed bound. So I was planning to have function like this

const bind = <T, P extends keyof T>(thisArg: T, ...props: P[]) => {
    props.forEach(prop => {
        Object.defineProperty(thisArg, prop, {
            configurable: true,
            enumerable: false,
            writable: true,
            value: (<any>thisArg[prop]).bind(thisArg)
        })
    })
}

Use it like

class Foo {
    constructor() {
        bind(this, "_pri");
    }
    private _pri() {
    }
}

I do know there are decorators that try to do the job. but all of them have issues when the bound methods are inherited. I won't go into details here. You can try it yourself. Honestly, I don't think the decorator can solve the problem and still keep inheritance to work as it does.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jan 18, 2017

In your example, the compiler can complain when private 'm' are returned from the public 'foo' method.

You have to consider indirection

class X {
  private m;
  foo(): keyof X {
    let a: keyof X = 'm'; // Proposal is to allow this, after all
    return a; // keyof X must be OK to return here
  }
}
@marty-wang

This comment has been minimized.

Copy link
Member Author

marty-wang commented Jan 18, 2017

@RyanCavanaugh

I made the suggestion based on the fact that the indirection is currently not allowed. :)

Even with indirection, the compiler could still statically determine if 'a' is 'm' or not, unless you plan to allow the following, which I dont see any merits whatsoever.

class X {
  private bar;
  foo(): keyof X {
    let a: keyof X = 'b' + 'ar'; // This would be ridiculous
    return a;
  }
}
@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jan 18, 2017

Even with indirection, the compiler could still statically determine if 'a' is 'm' or not

At some point this logic does have to stop. Like, here, we're not going to internally track that c's "actual" return type is maybe bar or maybe foo because a() is actually bar and b() is actually foo. Analysis stops at return type annotations.

class X {
  private bar;
  a(): keyof X { return 'bar' }
  b(): keyof X { return 'foo' }
  c(): keyof X { return Math.random() > 0.5 ? a() : b() }
  foo(): keyof X {
    let a: keyof X = c();
    return a;
  }
}
@marty-wang

This comment has been minimized.

Copy link
Member Author

marty-wang commented Jan 19, 2017

It seems that most of you disagreement stems from the technical challenge, but not from my arguments for the consistency of the property visibility.

In my view, the result of keyof operator is basically the union of the properties of the given type. Since the visibility of the properties are inherently scoped, the union of the properties should be scoped accordingly. It should be consistent that, within any scope if obj.someProp works, the obj should access the someProp via keyof operator as well.

If you don't share the same view, could you please provide the counterarguments.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Jan 19, 2017

What I'm saying is that there are type positions in a class declaration which are ambiguously private or public, and that the transition zones between "clearly private" and "clearly private" are extremely ill-defined. Without a clearly-specified rule about where you can use private names as a keyof member and where you can't, this simply boils down to "make keyof return all private names as well" which is clearly not OK.

Using flow control analysis to wave away the first example I presented is dodging the problem, because ambient classes exist.

For example:

declare class Base {
  public q;
  protected w;
  protected f(): keyof Base;
}

class X extends Base {
  private p;
  private g(): keyof X { return f(); }
  public h(): keyof X { return g(); }
}
let a: keyof X = (new X()).g();

Is this an error? We don't know if f is returning a public, protected, or even potentially private member of Base. We don't know g returns a protected member, or maybe even a private member. Is it allowed for h to return g()? They have the same return type, so it should be OK, but we know that keyof X sometimes has nonpublic members in it, so maybe keyof X isn't assignable to keyof X ?

@HerringtonDarkholme

This comment has been minimized.

Copy link
Contributor

HerringtonDarkholme commented Jan 19, 2017

IMHO, Ryan is talking about ambiguous usage under your proposal, not technical implementation problem.

Consider this more concrete example:

class PasswordVault {
  private password
  public username
  private goodKey(): keyof PasswordVault { return 'username '}
  private badKey(): keyof PasswordVault { return 'password '} 
  public getValue() {
    return this[this.badKey()] // should compiler approve this leakage? 
  }
}
@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented May 22, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this May 22, 2017

@Microsoft Microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.