Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

The proposal should be rejected! #148

Closed
aimingoo opened this issue Oct 16, 2018 · 15 comments
Closed

The proposal should be rejected! #148

aimingoo opened this issue Oct 16, 2018 · 15 comments

Comments

@aimingoo
Copy link

aimingoo commented Oct 16, 2018

I know that there are many similar issues and read them enough for all, but the observations, facts and analysis in this article are relatively independent, so I hope to have a new issue to publish this report.
Thanks for all reviewers.
If you want to read the Chinese version, here.

1. Concept: "Not Fields"!

The Object is defined as "object is collection of properties [^Note 1]" in ECMAScript. So if Field is not a property, it must not belong to the concept set of "Object's member (collection elements of object)"; if Filed is property, then public field must be equal to property, there is a conceptual conflict.

This is the root of all existing contradictions.

The "proposal-class-fields" proposal is fundamentally contradictory to the core concept of the ECMAScript object. It attempts to illustrate [^Note 2] : an object is a structure defined by a class member, and a class member includes a property name and a private name; The property name defines the property.

In this definition, the object is a "entity of a class member (fields)", and each field define by his name. That is to say, the Object is collection of names, is instance of field definitions by class.

The proposal's stealing of the concept is not only at the narrative level, it is exactly what it is doing. As follows [^Note 3] :

2.7 DefineField(receiver, fieldRecord)
...
  8. If fieldName is a Private Name,
    ... // add as private filed
  9. Else, // public field as property
    Assert: IsPropertyKey(fieldName) is true.
    Perform ? CreateDataPropertyOrThrow(receiver, fieldName, initValue).

We have clearly seen that the proposal is destruction of the core concepts of Objects. And "Not Fields" is the only way to stop it.

[^Note 1]: "an object is a collection of zero or more properties." ECMAScript Overview part.

[^Note 2]: The FieldDefinition in proposal-class-fields, New Productions part.

[^Note 3]: The DefineField() algorithms in proposal-class-fields, Modified algorithms part.

2. Implementation: Recreated a var!

For next case:

class f {
    #data = 100;
    foo() {
        this.#data = 200;
    }
}

The effect that the proposal attempts to achieve is similar to the following:

clas f {
    constructor() {
        ... // call super, etc
        var data = 100;
        this.foo = function() {
           data = 200;
        }
    }
}

Note that the proposal essentially does not "Intended" to provide private fields for the object instance this, but instead provides a set of names accessible in a particular context through the lexical environment.

However, as shown in the example above, if you use this method to provide a private name data for this.foo(), then foo() will have a lot of Function instances. To prevent a single object from repeatedly binding multiple function instances (as his method), the proposal puts the above "collection of names" as an internal slot in the class, and provide a internal-slot [[PrivateFieldValues]] as copy (or fork) for this set of names when creating the this object.

But, there is still some distance from the truth. For this proposal, this is just a stopgap measure to avoid creating separate instances for each object and method. The real situation is that it creates the "PrivateNameScope" for each class, for each function, and for each object, and then maintains the scope in the lexical environment. Since each object "may have" a private domain, in this solution, each function call needs to bear the cost of checking this private domain: every time, every place, and every function-related property.

So the proposal skyrocketed! Most of the internally methods need to add the PrivateNameScope parameter to the interface, and each context needs to be added with PrivateNameEnvironment. [^Note 4]

Is it a little familiar? What was the last thing that affected the ECMAScript specification in such a large scope?

That's right, it's VariableEnvironment! It is the var declaration that creates a conflict with the "block-level scope", resulting in a historical design with the new syntax keyword let enabled. The tricky design of Private Fields is so obvious that it comes with VariableEnvironment, but it seems that no one of the reviewers sees it:

3.9.9 PrepareForOrdinaryCall ( F, newTarget )
...
  8. Let localEnv be NewFunctionEnvironment(F, newTarget).
  9. Set the LexicalEnvironment of calleeContext to localEnv.
  10. Set the VariableEnvironment of calleeContext to localEnv.
  11. Set the PrivateNameEnvironment of calleeContext to F.[[PrivateNameEnvironment]].

They reinvented the wheel, again. And, it’s square wheel, again.

Two square wheels!

[^Note 4]: Every call in proposal-class-fields, 3.9.9 PrepareForOrdinaryCall.

3. Obvious: it doesn't work! Inefficient

NOTE: the spec will create a map of each private name, with a unique key and the set name as description. This seems to work but it is very inefficient. thanks for @bakkot .
I have a small omission, but I will not withdraw this issue for this reason.

Next each function has privateNameScope, and so on. So since there are PrivateNameEnvironment or privateNameScope, why should each object have a [[PrivateFieldValues]] internal slot?

The answer is: the internal slot of the object is used to store the field value for the private name [^Note 5], and the environment and scope components are used to detect whether the private name can be accessed [^Note 6]!

However this is not valid. E.g:

But have a problem, E.g:

class f {
    #data = 100;   
}

class faked {
    #data;
    foo() {
        console.log(this.#data);  // try `#data`
    }
}

// access private with a faked class
x = new f;
x.foo = faked.prototype.foo;
x.foo();  // 100?

I have read the complete proposal carefully, yes, the above BUG is inevitable. Because this process is no different:

function privateScope(name) {
    var privated_data = 100;
    return eval(name);
}
console.log(privateScope('privated_data'));

As long as you allow "read variable name" operations on private domains, all privated datas are not safe.

This is a pupil level error, But irreparable, for this proposal.`

NOTE: So, the solution need a map to register each name with a unique key.

[^Note 5]: Access field value in proposal-class-fields, 3.5 PrivateFieldGet.

[^Note 6]: Check name binding status in proposal-class-fields, 3.8.1 GetValue.

4 Conclusion

At the abstract level, (in terms of the impact of abstract concepts on users), this proposal destroys or steals the concept of "Object" in ECMAScript; in terms of implementation, it consumes a lot but does not security and cannot achieve private expected. I have to say that this is a failed, unusable, rough design, and backward thinking.

Recommendation: reject the proposal-class-fields proposal.

@ljharb
Copy link
Member

ljharb commented Oct 16, 2018

Your examples in number 3 would throw; those are different “data” private fields, despite having the same name. If the current spec allows it, it’d be a spec bug that’d need fixing.

@mbrowne
Copy link

mbrowne commented Oct 16, 2018

Related: #144

@bakkot
Copy link
Contributor

bakkot commented Oct 16, 2018

If the current spec allows it

It does not.

@rdking
Copy link

rdking commented Oct 16, 2018

@mbrowne The nearest working model that's similar to what happens when declaring #field is that a new Symbol-like object is created and associated to the class definition. This means that accessing private fields is not about the friendly(?)name "#field" that you use to reference it, but rather the hidden name that can be used to reference it. Consider each class like a function's closure with a Symbol constant for each private field's name. Except that Symbol will never be an own property of any instance of that class. Since the Symbol foo knows about is not known to instances of f, access fails.

@mbrowne
Copy link

mbrowne commented Oct 16, 2018

Thanks @rdking. Did you mean to write that in the other thread, or here?

@aimingoo
Copy link
Author

aimingoo commented Oct 16, 2018

@rdking @bakkot @ljharb

NO. spec will allows the bug!

a name #data will put to PrivateNameEnvironment chain of f.prototype.foo() and faked.prototype.foo(), because the symbol define in their lex scope. the name is simple string.

Next, use this.#data in faked.prototype.foo(), it's allows. but bind x as this by x.foo(), will get a Reference (Specification Type) with base = x and name = ‘#data', allows again. for these, please read "3.7Runtime Semantics: Evaluation“ at here.

If get a privateReference as rhs, will call GetValue() to pick data. now, check binding at 5.b.ii, allow because lex scope has name '#data'; and , get private filed value at 5.b.iv, allows again because the privateReference.base is this === x. please recheck "GetValue()" at here.

The name check in PrivateNameEnvironment chain, and the value get from this instance. so, scope invalid!

The BUG is inevitable! Because spec allow "read variable name" operations on private domains!

Can not fix it on current sepc! Absolutely!

@bakkot
Copy link
Contributor

bakkot commented Oct 16, 2018

@aimingoo, step 5.b.ii of GetValue resolves the name reference to the Private Name value created for faked, which is not the same as the Private Name value created for f despite having the same name. Then step 5.b.iv invokes PrivateFieldGet, which will throw because x does not have that Private Name value in its [[PrivateFieldValues]].

The crucial step is that 5.b.ii resolves #data in "the running execution context's PrivateNameEnvironment", i.e., in the PrivateNameEnvironment of faked.prototype.foo. The resolution of #data to a Private Name value does not at any point involve base.

@rdking
Copy link

rdking commented Oct 16, 2018

@mbrowne probably the other thread... oops!

@aimingoo
Copy link
Author

@bakkot

Oh! Yes, NewPrivateName will return a unique key as name ... god, a map of class and instance's private names!!!

I will note in this issue. thanks.

@aimingoo aimingoo changed the title The proposal should be reject! The proposal should be rejected! Oct 17, 2018
@aimingoo
Copy link
Author

A alternative proposal released at #154 . @ljharb @mbrowne @bakkot @rdking

Thanks for all.

@trusktr
Copy link

trusktr commented Jun 26, 2019

The fact that when running

x.foo = faked.prototype.foo;
x.foo();  // 100?

in Chrome throws

Uncaught TypeError: Read of private field #data from an object which did not contain the field

means private fields do not give true privacy.

Why go through all the hassle of this proposal (which has so much dislike) just to end up having soft privacy instead of the stated goal of hard privacy?

It'd be better than this current proposal to make a better syntax, and other changes, without true privacy then.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2019

It is indeed hard privacy, because you can’t fake it. It’s exactly what would happen if you had a closed-over weakset that stored instances, and threw when the receiver wasn’t in the weakset.

@trusktr
Copy link

trusktr commented Jun 26, 2019

Is it still considered hard privacy if I can use this trick to detect which private members exist in the class by reading the error message?

@nicolo-ribaudo
Copy link
Member

The error message is not specified by the specification. An engine can choose to "leak" the name in the error message, but it can also choose to hide it.
This is the same for every other ECMAScript error.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2019

@trusktr also, any class can wrap the private member access in a try/catch and suppress or fake that error, so that "trick" isn't remotely reliable anyways.

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

No branches or pull requests

7 participants