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

Add coverage for Private methods and accessors #1343

Closed
34 of 47 tasks
leobalter opened this issue Nov 10, 2017 · 21 comments
Closed
34 of 47 tasks

Add coverage for Private methods and accessors #1343

leobalter opened this issue Nov 10, 2017 · 21 comments
Labels

Comments

@leobalter
Copy link
Member

leobalter commented Nov 10, 2017

Already on Stage 3

Ref: https://tc39.github.io/proposal-private-methods/

From tests and/or the other list

  • Methods that can be named "#prototype"
  • Valid identifier names
  • No space permitted between # and name
@littledan
Copy link
Member

I'm working on a test plan with @mkubilayk @Neuroboy23 @jbhoosreddy @tim-mc @cameron-mcateer @molisani @rricard from Bloomberg ; we plan to collaborate on the private methods/accessors tests.

@littledan
Copy link
Member

littledan commented Jul 4, 2018

A test plan--if you have other ideas, please post them further down on the thread:

  • Early errors
    • Grammar (@jbhoosreddy)
      • No space permitted between # and name (either in definition or usage)
      • Space permitted between receiver and private method access
      • "shorthand" not permitted -- a receiver is required
      • Private methods not permitted in object literals
      • Super private access (super.#x()) is not permitted
      • Private methods may not be deleted
    • Referencing a missing private name, including
      • Cases where the name is defined in a more inner context, as allowed in Java
      • In a subclass (this is private, not protected)
      • Indirect eval doesn't see any private names at all
      • Private names are not visible inside of the extends clause
    • Duplicate private names (@mkubilayk)
      • Multiple private methods inside of a class
      • A private method and a getter/setter
      • Multiple get or multiple set
      • A private method and a private field conflicting
      • Static and instance declarations with the same name conflicts
      • Shadowing in a nested class is OK
  • Semantics
    • Private method constructs
      • Private ordinary methods
      • Private accessors
      • Private generators, async functions, async generators
      • static variants of all three of the above
    • Usage
      • Calling a private method
      • Reading a private method separately from calling it
        • All instances of the class share the same function identity for the private method
      • Private methods can do super property access of public methods/properties, and this access uses the appropriate home object
      • The .name property of the private method is "#foo"
      • The .toString() of the private method is the exact string of its declaration (depends on the Function.prototype.toString Stage 3 feature flag as well) -- include comments and various declaration forms in the test
      • It's OK at parse-time to reference a static private method from an instance method, or vice versa, and you can make use of this at runtime with creative application of Function.prototype.call.
    • Complex contexts where private names are visible (use an assignment as the left operand of a comma expression as an easy mechanism to "leak" a function to access the private name)
      • Direct eval sees the same set of private names as the outer scope
      • Inside of computed property names
      • Inside of a nested function, block, class, arrow function
    • Error on missing method
      • The type check is done on reading the method, not on calling it--potentially, the method could be called with a different receiver
      • Execution order -- private methods are added before any field initializer is run, even if they appear textually later
      • Class factories -- If you have a function which returns a class expression, then each time this function is run, it will create different internal private names, and the private methods from instances of one will not exist on the other
        • Ditto, if the same script is evaluated in two different realms, the clasess can't access each other's private methods
      • Subclass receiver for static private, e.g., (class extends (class { static f() { this.#g(); } static #g() { } } { }).f() is a TypeError.
      • Private methods are installed "when super returns" and no earlier, e.g., the following is a TypeError when accessing #h (not g): new (class extends (class { constructor() { this.g() } }) { g() { this.#h() } #h() { })(). A similar test is possible with field initializers instead of code called in the constructor.
      • If you don't actually reach the super call, and instead return something else from the constructor, the private methods aren't available
    • Object model
      • Private methods are non-writable
      • If a getter but not setter is provided, set throws an exception (and vice versa)
      • Private methods don't show up in
        • [] access
        • Object.defineProperty
        • Object.keys
        • Proxy traps
      • Private methods can be installed through the "super return trick", including on Proxy objects
      • You can have a property obj["#foo"] and this doesn't at all conflict with a private method obj.#foo or affect its interpretation
  • Usage in various contexts -- Some test262 tests are based on the templating system for this purpose. You could do this for all tests, or you could provide a few targeted tests to look at these combinations; I don't have a strong preference (my intuition is usually for targeted combinations, but templating has found some interesting bugs in the past).
    • Types of declarations
      • export default class
      • export class X
      • class declaration at the top level of a script/module
      • class declaration in a block
      • class declaration inside a function at the top level
    • class expression in various contexts
      • Just in parentheses, at the top level or in some nested context
      • As a default in destructuring assignment (e.g., ({x = class C { /*...*/ }}) = /* ... */)
      • As a default in destructuring bind (e.g., (x = class C { /* */ }) => {/* ... */})
      • In an extends clause
    • Interactions with eval
      • Any of the above declarations in a direct or indirect eval
      • A direct eval can access any outer private names
      • No private names leak from direct eval

@jbhoosreddy
Copy link
Contributor

Thanks @littledan for the test plan. I'll start of with Early Errors > Grammar to familiarize myself with working on test262.

@mkubilayk
Copy link
Contributor

I'll also give it a try this week. Taking Early errors > Duplicate private names.

jbhoosreddy added a commit to jbhoosreddy/test262 that referenced this issue Aug 30, 2018
jbhoosreddy added a commit to jbhoosreddy/test262 that referenced this issue Aug 30, 2018
jbhoosreddy added a commit to jbhoosreddy/test262 that referenced this issue Aug 30, 2018
leobalter added a commit that referenced this issue Sep 4, 2018
test: Add private generator method tests (#1343)
@jbhoosreddy
Copy link
Contributor

private method async generators, generators, async function, tests landed

jbhoosreddy added a commit to jbhoosreddy/test262 that referenced this issue Sep 5, 2018
leobalter pushed a commit to leobalter/test262 that referenced this issue Sep 10, 2018
jbhoosreddy added a commit to jbhoosreddy/test262 that referenced this issue Sep 11, 2018
rwaldron added a commit to rwaldron/test262 that referenced this issue Sep 13, 2018
…sreddy/test262 into jbhoosreddy-private-methods-delete-error

* 'private-methods-delete-error' of git://github.com/jbhoosreddy/test262:
  revisions
  fixes
  test: Early error private methods delete (tc39#1343)
leobalter added a commit that referenced this issue Sep 13, 2018
…lete-error

test: Early error private methods delete (#1343)
@leobalter leobalter self-assigned this Nov 5, 2018
@leobalter
Copy link
Member Author

I updated the top comment with a Checklist I'm still working on - it's being pretty tricky to verify everything covered.

That list follows parts identified directly in the specs texts (also considering the private fields spec text).

I haven't had the time to cross match with items from the other list.

@jbhoosreddy
Copy link
Contributor

jbhoosreddy commented Dec 11, 2018

  • Early errors
    • Grammar (@jbhoosreddy)
    • No space permitted between # and name (either in definition or usage)
    • Space permitted between receiver and private method access
    • "shorthand" not permitted -- a receiver is required
    • Private methods not permitted in object literals
    • Super private access (super.#x()) is not permitted
    • Private methods may not be deleted
  • Referencing a missing private name, including
    • Cases where the name is defined in a more inner context, as allowed in Java
    • In a subclass (this is private, not protected)
    • Indirect eval doesn't see any private names at all
    • Private names are not visible inside of the extends clause
  • Duplicate private names (@mkubilayk)
    • Multiple private methods inside of a class
    • A private method and a getter/setter
    • Multiple get or multiple set
    • A private method and a private field conflicting
    • Static and instance declarations with the same name conflicts
    • Shadowing in a nested class is OK
  • Semantics
    • Private method constructs
    • Private ordinary methods
    • Private accessors
    • Private generators, async functions, async generators
    • static variants of all three of the above
  • Usage
    • Calling a private method
    • Reading a private method separately from calling it
    • All instances of the class share the same function identity for the private method
    • Private methods can do super property access of public methods/properties, and this access uses the appropriate home object Link
    • The .name property of the private method is "#foo"
    • The .toString() of the private method is the exact string of its declaration (depends on the
    • Function.prototype.toString Stage 3 feature flag as well) -- include comments and various declaration forms in the test
    • It's OK at parse-time to reference a static private method from an instance method, or vice versa, and you can make use of this at runtime with creative application of Function.prototype.call.
  • Complex contexts where private names are visible (use an assignment as the left operand of a comma expression as an easy mechanism to "leak" a function to access the private name)
    • Direct eval sees the same set of private names as the outer scope
    • Inside of computed property names/fields
    • Inside of a
      • nested function block
      • class Link
      • arrow function
  • Error on missing method
    • The type check is done on reading the method, not on calling it--potentially, the method could be called with a different receiver
    • Execution order -- private methods are added before any field initializer is run, even if they appear textually later
    • Class factories -- If you have a function which returns a class expression, then each time this function is run, it will create different internal private names, and the private methods from instances of one will not exist on the other
    • Ditto, if the same script is evaluated in two different realms, the clasess can't access each other's private methods
    • Subclass receiver for static private, e.g., (class extends (class { static f() { this.#g(); } static #g() { } } { }).f() is a TypeError.
    • Private methods are installed "when super returns" and no earlier, e.g., the following is a TypeError when accessing #h (not g): new (class extends (class { constructor() { this.g() } }) { g() { this.#h() } #h() { })(). A similar test is possible with field initializers instead of code called in the constructor.
    • If you don't actually reach the super call, and instead return something else from the constructor, the private methods aren't available
  • Object model
    • Private methods are non-writable
    • If a getter but not setter is provided, set throws an exception (and vice versa)
    • Private methods don't show up in
      • [] access
      • Object.defineProperty
      • Object.keys
      • Proxy traps
    • Private methods can be installed through the "super return trick"
      • including on Proxy objects
    • You can have a property obj["#foo"] and this doesn't at all conflict with a private method obj.#foo or affect its interpretation
  • Usage in various contexts -- Some test262 tests are based on the templating system for this purpose. You could do this for all tests, or you could provide a few targeted tests to look at these combinations; I don't have a strong preference (my intuition is usually for targeted combinations, but templating has found some interesting bugs in the past).
    • Types of declarations
      • export default class
      • export class X
      • class declaration at the top level of a script/module
      • class declaration in a block
      • class declaration inside a function at the top level
    • class expression in various contexts
      • Just in parentheses, at the top level or in some nested context
      • As a default in destructuring assignment (e.g., ({x = class C { /.../ }}) = /* ... */)
      • As a default in destructuring bind (e.g., (x = class C { /* / }) => {/ ... */})
      • In an extends clause
    • Interactions with eval
      • Any of the above declarations in a direct or indirect eval
      • A direct eval can access any outer private names
      • No private names leak from direct eval

@littledan
Copy link
Member

Great progress, @jbhoosreddy

@leobalter
Copy link
Member Author

I'm trying a good effort to connect these dots to effort text. It would be nice if this could be done in any checklist as well. Otherwise it's complex enough to understand how coverage affects the proposed specs text.

Examples:

Private methods are non-writable

This can connected to the abstractions for PrivateFieldSet, etc. There is a list above with this information.

If a getter but not setter is provided, set throws an exception (and vice versa)

set is undefined, it's also in the PrivateFieldSet

You can have a property obj["#foo"] and this doesn't at all conflict with a private method obj.#foo or affect its interpretation

nice one, but we can link this to the fact "#foo" is a valid property name, and not a reference to a private field.

Class factories -- If you have a function which returns a class expression, then each time this function is run, it will create different internal private names, and the private methods from instances of one will not exist on the other

I believe we already have this covered. It's also in the fact private names are stored differently. We should explain why, not just tell.

@littledan
Copy link
Member

I'm trying a good effort to connect these dots to effort text.

(by "effort text", you meant "spec text", right?)

Those associations LGTM. Are there any lines in the test plan that are not clear to you, where the spec text describes their behavior? I'd be happy to clarify.

We should explain why, not just tell.

Where do you suggest that we write this documentation?

FYI I just landed a large editorial change to private methods in tc39/proposal-private-methods#52 .

@leobalter
Copy link
Member Author

Are there any lines in the test plan that are not clear to you, where the spec text describes their behavior? I'd be happy to clarify.

It's impossible we organize our work the same way. I'm trying to share my way to map things to spec text. Without it, everything is really hard for me to connect to spec text without any reference.

The clarification I need - as mentioned in the other message - would be finding a way to map these to the spec. If that's impossible, I'll need extra time to connect everything because I would need to map everything you are providing in two different forms. I'm fine doing either way, but of course I prefer it faster.

@leobalter
Copy link
Member Author

Where do you suggest that we write this documentation?

just a quick mapping to the spec (no specific way to map) would do the trick. The spec is the documentation, right?

@jbhoosreddy
Copy link
Contributor

jbhoosreddy commented Apr 1, 2019

@leobalter, @mkubilayk and I were trying to get back at the test plan. Have a few questions around:

  • ModuleBody : ModuleItemList It is a Syntax Error if AllPrivateNamesValid of ModuleItemList with an empty List as an argument is false.
    • We were unable to decipher what this means? Could you give us an example..

We think these tests are done:

Runtime Semantics: ClassElementEvaluation
Method Descriptor (can't be observed directly)
ClassElementName in MethodDefinition
Accessor get/set methods https://github.com/tc39/test262/tree/master/src/class-elements (grep: rs-private*)

cc @littledan

@littledan
Copy link
Member

ModuleBody : ModuleItemList It is a Syntax Error if AllPrivateNamesValid of ModuleItemList with an empty List as an argument is false.

I believe this is simply getting at, if you have a module with an undefined private name in it (e.g., the module body is simply #x), then it's a SyntaxError.

@caiolima
Copy link
Contributor

#2188 is covering "Private methods don't show up in..."

@caiolima
Copy link
Contributor

caiolima commented Aug 12, 2019

I'm adding here also current plan to support static public/private fields and static private methods.

  • Declaration:
    • Early error using constructor/prototype.
    • Simple declaration.
    • Declaration with initializer (It seems to be partially covered).
    • Initializer with direct eval.
    • Initializer using ‘this’.
    • Initializer with super/arguments.
    • Redeclaration of public fields.
    • Redeclaration of private fields/methods.
    • Private field version for tests above;
    • Valid names declarations (Done?);
    • Computed properties.
  • Usage
    • Try to access private fields and methods from:
      • Inner class.
      • Inner functions/arrow functions.
    • Try to access private fields/methods from subclass.
    • The .”name” property of the static private method.
    • PrivateNames are visible through direct eval.
    • Static private methods can be called with different receiver.
    • Verify that multiple evaluations of class can’t access their private static method.
      • Direct eval;
      • Indirect Eval;
      • Differente scope (inner function);
      • Different realms.
    • Complex usage of inner class:
      • Access inside inner class;
      • Shadow of private fields/methods/accessors inside inner class;
      • Shadow of private fields/methods/accessors from outer class;
      • Static accessors for tests above;

@qmma70
Copy link
Contributor

qmma70 commented Aug 16, 2019

ClassBody : ClassElementList - Missing duplicates checks for generators, async functions, and async generators

Would like to look into this.

@jbhoosreddy
Copy link
Contributor

jbhoosreddy commented Aug 16, 2019

Should we test? cc @leobalter

Runtime Semantics: CreateDynamicFunction > Usage within scope where AllPrivateNamesValid of class elements is true

https://github.com/tc39/test262/pull/2289/files#diff-42f5a24ed6ec02a54fc62434aa5defa6R41 with function constructor

@caiolima
Copy link
Contributor

I'm removing Method Descriptor (can't be observed directly) item, since it is not part of current spec anymore.

@littledan
Copy link
Member

Can this be closed due to complete test coverage?

@leobalter
Copy link
Member Author

I believe we have a reasonable coverage at this point and everything else should be considered edgy cases we are yet to identify as missing coverage. We should close these for now and work with follow ups on demand, if any.

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

No branches or pull requests

6 participants