Add coverage for Private methods and accessors

Question

Add coverage for Private methods and accessors

leobalter opened this issue 7 years ago · 21 comments

Answer 1 · 2018-06-25T16:41:02.000Z

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.

Answer 2 · 2018-07-04T15:09:53.000Z

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

Answer 3 · 2018-07-09T13:08:53.000Z

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

Answer 4 · 2018-07-09T13:16:30.000Z

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

Answer 5 · 2018-09-04T18:12:03.000Z

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

Answer 6 · 2018-11-08T23:10:49.000Z

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.

Answer 7 · 2018-12-11T14:39:41.000Z

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

Answer 8 · 2018-12-11T15:13:10.000Z

Great progress, @jbhoosreddy

Answer 9 · 2018-12-11T19:44:01.000Z

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.

Answer 10 · 2018-12-13T10:33:36.000Z

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 .

Answer 11 · 2018-12-14T20:36:10.000Z

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.

Answer 12 · 2018-12-14T20:37:21.000Z

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?

Answer 13 · 2019-04-01T20:11:52.000Z

@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

Answer 14 · 2019-04-08T14:11:03.000Z

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.

Answer 15 · 2019-06-28T16:23:17.000Z

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

Answer 16 · 2019-08-12T13:55:15.000Z

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;

Answer 17 · 2019-08-16T16:20:36.000Z

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

Would like to look into this.

Answer 18 · 2019-08-16T16:23:48.000Z

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

Answer 19 · 2019-08-30T15:15:14.000Z

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

Answer 20 · 2019-09-13T08:50:02.000Z

Can this be closed due to complete test coverage?

Answer 21 · 2019-10-10T15:11:32.000Z

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.

From tests and/or the other list