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

Design Meeting Notes, 6/9/2017 #16415

Closed
DanielRosenwasser opened this issue Jun 9, 2017 · 22 comments
Closed

Design Meeting Notes, 6/9/2017 #16415

DanielRosenwasser opened this issue Jun 9, 2017 · 22 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 9, 2017

Stricter generic signature checks

  • Already did some work in Infer from generic function return types #16072 to perfom type argument inference using the return type of a signature as a target.
  • In Contextual generic function types #16305 we added support for signatures to gain type parameters from their contextual types.
    • This behavior is more appropriate because it respects the constraints and the universality of each usage of type parameters.
  • To follow up with this, we're introducing stricter generic signature checks (Stricter generic signature checks #16368).
    • Previously, when relating type parameters, we would erase them with the type any.
    • This was fairly unsafe.
    • Previously the following would work, but with Stricter generic signature checks #16368, the following is an error.
      type A = <T, U>(x: T, y: U) => [T, U];
      type B = <S>(x: S, y: S) => [S, S];
      
      function f(a: A, b: B) {
          a = b;  // Error
          b = a;  // Ok
      }
    • This is kind of a form of unification between the two parameters.
      • We infer using the type parameters of the target, as if we were calling the source with the target's parameters.
        • In other words, in a = b above, it simulates calling b with a's parameters.
    • The caveat is that this logic is only applied to single-signature types.
      • Any times you have overloads, it becomes extremely difficult to perform these checks.
  • So the question is: do we put this behind a flag?
    • Most errors are good, but it's hard to figure out what to do for some advanced scenarios.
    • Get it into master and see what we can work with.

Private Slots

Proposal: https://github.com/tc39/proposal-private-fields/ (Permalink)

Repo: https://tc39.github.io/proposal-private-fields/

  • How does this work with existing modifiers?

  • Can we offer a true downlevel translation?

    • To ES5?
      • No.
      • Need weakmaps
    • Yes above ES6.
  • Could give a mangled name in ES5/ES3.

    • Has potential problems.
      • Proxies, enumeration, etc.
        • Proxies and the like don't exist in ES5.
        • Best effort might be fine.
  • What about using Symbols in ES6?

    • Symbols aren't truly private - can be used by proxies, can be iterated over, etc.
  • Weakmaps make it truly private.

    • Are they performant?
      • Concerns over the type of GC pressure it may cause.
      • Edge is good at this!
        • Implementation is that every object has a list of weakmaps so that the lookup is inverted.
      • Others not as much at this point in time.
        • But will eventually improve!
    • But it's all opt-in.
    • Is it possible that all users really want is design-time private?
      • There are definitely people who've wanted private state.
  • P.S. everyone hates the syntax. we're not big on the syntax

  • Completely unrelated

    • The proposal breaks JScript 5.8 in IE 8.
      • ¯\_(ツ)_/¯
  • We'll all need to go over the FAQ more closely on the private state proposal repo.

@DanielRosenwasser DanielRosenwasser added the Design Notes Notes from our design meetings label Jun 9, 2017
@mhegazy mhegazy closed this as completed Jun 12, 2017
@kitsonk
Copy link
Contributor

kitsonk commented Jun 13, 2017

Just a comment with the ES5 down-emit with private members, if you considered the async/await down emit pattern by assuming WeakMap it would be nice to have the down-emit and have those of us stuck in ES5 provide the polyfill.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 13, 2017

Also from a @dojo perspective we have debated a lot of putting private state in weak maps. We just know that if it is accessible, people will abuse it and the only way to to ensure it to promote it being truly private. The biggest challenge is that we have in some cases run into having to name privates in descendent classes silly things because of private member name conflicts, which makes code unclear.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 13, 2017

And P.S. I hate the syntax for the record too... 😂

@littledan
Copy link

Hey, thanks for the feedback about the syntax. The rationale is documented in the FAQ. Does anyone have any other suggestions?

The proposal breaks JScript 5.8 in IE 8.

What's the concern here?

@styfle
Copy link
Contributor

styfle commented Jun 20, 2017

@littledan The FAQ doesn't address why the octothorpe (#) sigil was chosen.

I would think the js community would prefer the underscore (_) which is a popular convention used to denote private member variables in current code as well as python and other languages.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 20, 2017

I would think the js community would prefer the underscore (_) which is a popular convention used to denote private member variables in current code as well as python and other languages.

The problem is that would break the internet, because the runtime engine must lookup the property differently. If every _foo suddenly became truly private, imagine the horror. The sigil must be something that isn't currently a valid identifier in JavaScript to ensure that nothing breaks.

@littledan
Copy link

@styfle This is a great point. I added a new FAQ entry to explain the reasoning.

@styfle
Copy link
Contributor

styfle commented Jun 20, 2017

@littledan Excellent, thank you for the detailed write up! I submitted a PR with some typo fixes.

I noticed the proposal-private-fields is Stage 1 but the proposal-class-fields is Stage 2 and seems to include private fields. Is this expected and if so, why?

@mihailik
Copy link
Contributor

Why WeakMaps not closures?

@littledan
Copy link

@mihailik Because there's still just one method identity on the prototype, rather than the instance.

@mihailik
Copy link
Contributor

A rough prototype (play with it on CodePen)

function Apple() {
  
  // carrying private state
  function Private() {}
  Private.prototype = this;
  var private = new Private();
  
  // bind methods to private state
  this.getColor = private.getColor.bind(private);
  this.setRed = private.setRed.bind(private);
  this.setGreen = private.setGreen.bind(private);
}

Apple.prototype.setRed = function() {
  this._color = 'red';
  this.constructor.prototype.caption = 'Red Apple'; // mutating of public state, a bit convoluted
};

Apple.prototype.setGreen = function() {
  this._color = 'green';
  this.constructor.prototype.caption = 'Green Apple'; // mutating of public state
};

Apple.prototype.getColor = function() {
  return this._color;
};

var s = new Apple();
s.setRed();
console.log('setRed: ', s.getColor(), s.caption, s._color);
s.setGreen();
console.log('setGreen: ', s.getColor(), s.caption, s._color);

s.caption = 'Orange Apple?';
s._color = 'orange';
console.log('mutating directly: ', s.getColor(), s.caption, s._color);

@littledan
Copy link

Bound methods are being pursued separately, cc @rbuckton

In some engines, using bound methods all the time is slower. Anyway, ES6 class does not have bound methods. Ordinary methods should be able to read private fields without suddenly switching into an entirely different mode, right?

@mihailik
Copy link
Contributor

Having to re-bind methods for each instance is the cost of implementation using closures.

Is WeakMap implementation cheaper? Not obvious. Especially considering nontrivial performance/GC/JIT pressures of WeakMap.

Closure implementation has another win: it works on ES5 too.

@mihailik
Copy link
Contributor

mihailik commented Jun 21, 2017

Besides, the approach works when emitting ES6 classes too (again, CodePen to try):

class Apple {
  
  constructor() {
    // carrying private state
    function Private() {}
    Private.prototype = this;
    var _private = new Private();

    // bind methods to private state
    this.getColor = _private.getColor.bind(_private);
    this.setRed = _private.setRed.bind(_private);
    this.setGreen = _private.setGreen.bind(_private);
  }
  
  setRed() {
    this._color = 'red';
    this.constructor.prototype.caption = 'Red Apple'; // mutating of public state, a bit convoluted
  }
  
  setGreen() {
    this._color = 'green';
    this.constructor.prototype.caption = 'Green Apple'; // mutating of public state
  }
  
  getColor() {
    return this._color;
  }
  
}


var s = new Apple();
s.setRed();
console.log('setRed: ', s.getColor(), s.caption, s._color);
s.setGreen();
console.log('setGreen: ', s.getColor(), s.caption, s._color);

s.caption = 'Orange Apple?';
s._color = 'orange';
console.log('mutating directly: ', s.getColor(), s.caption, s._color);

@littledan
Copy link

littledan commented Jun 21, 2017

If you're talking about how TypeScript should implement this, then the desugaring you have there has observably different semantics than the spec. In particular, you can .call a method which uses private state on a different receiver.

If you're talking about what semantics to adopt to be efficiently implementable: WeakMaps in V8 aren't the fastest, but private fields will allow implementation with V8's internal-only "private symbols", which are more efficient. Not sure about other engines.

@mihailik
Copy link
Contributor

mihailik commented Jun 21, 2017

True! Desugaring is already observably different of course — so it's a matter of weighing pros and cons.

Besides, I think the whole approach may be worth considering in JS spec as well. Using private keyword instead of '#' and making class methods accesses "tinted" so for any X.Y access the runtime would check if X is the same class and Y is a field/method declared as private.

It's more complex in the engine, but much benefit for the coders, and more sane mental model.

@littledan
Copy link

This isn't an engine issue, it's a language design issue. Please refer to the private fields FAQ to find documentation for the design.

I hope the TypeScript team tries to minimize observable differences, though I understand this isn't always possible. If differences are minimized, there will be a smaller "bump" for users who come from or to standard JS. In this case, which receiver is used is a pretty big difference.

@mihailik
Copy link
Contributor

mihailik commented Jun 21, 2017

@littledan out curiosity, in which practical case you expect this be observed? In which cases do people define methods on a class, only to bind it to something else?

The JS FAQ is cautious on private keyword due to complexity of VM implementation, not because language or mental model is hard. I don't think it's easy to find a JS user who would prefer the hash to private.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 21, 2017

There is extensive discussion of this in the TC39 proposal. No good re-hashing it here, on an issue about the TypeScript implementation of the proposal. It would be best to focus the discussion in the proper forum, which considers the matter closed.

@RyanCavanaugh
Copy link
Member

As usual I'd echo what @kitsonk just said

@mihailik
Copy link
Contributor

My closure-based suggestion is related to TypeScript. Any comments on that, regardless of JS implementation?

@littledan
Copy link

@mihailik I'd encourage TypeScript to not use that kind of closure-based implementation, since it'd diverge from the possible native implementations in a pretty major way. TypeScript users may start depending on this behavior and be disappointed when they have to change their code later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

7 participants