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

Proposal private-fields-in-in is stage-3 #42574

Closed
5 tasks
Kingwl opened this issue Feb 1, 2021 · 4 comments · Fixed by #44648
Closed
5 tasks

Proposal private-fields-in-in is stage-3 #42574

Kingwl opened this issue Feb 1, 2021 · 4 comments · Fixed by #44648
Labels
ES Next New featurers for ECMAScript (a.k.a. ESNext) feature-request A request for a new feature Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@Kingwl
Copy link
Contributor

Kingwl commented Feb 1, 2021

Suggestion

https://github.com/tc39/proposal-private-fields-in-in is stage 3 now.

🔍 Search Terms

esnext. private field. class

List of keywords you searched for before creating this issue. Write them down here so that others can find this suggestion more easily and help provide feedback.

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

📃 Motivating Example

class C {
  #brand = 42;

  static isC(obj: C) {
    return #brand in obj; // should work
  }
}

💻 Use Cases

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Feb 1, 2021
@DanielRosenwasser DanielRosenwasser added the ES Next New featurers for ECMAScript (a.k.a. ESNext) label Feb 2, 2021
@acutmore
Copy link
Contributor

Good news! A few of us at @bloomberg have started discussing working on this as a TS contribution

cc: @dragomirtitian @robpalme

@acutmore
Copy link
Contributor

acutmore commented Feb 20, 2021

Work has now started on this and is going well (basic parsing with simple errors and type checking).

  • Work merged to bloomberg/TypeScript
  • Add new 'PrivateIdentifierInInExpression' expression
    • 'prop' in obj is a BinaryExpression, but this isn't because syntactically the left side can only be a PrivateIdentifier
    • suggestions for alternative names instead of 'PrivateIdentifierInInExpression' ?
  • Error when the PrivateIdentifier can't be looked-up symbolically
    • outside of a class
    • no containing class with matching private field
  • Expression returns boolean
  • RHS must be object or any (and not null in strictNullChecks)
    • i.e. same rules as 'prop' in obj
  • Helpful error messages
  • Use in flowNodes and type narrowing
  • language server hints
    • suggest completion for #${privateFields} in on typing #
  • emit ESNext
  • emit down-level
  • add static private methods/fields support

Type Narrowing

class MyClass {
  #branded = true;
  prop = Math.random();

  equals(x: unknown) {
    if (typeof x === 'object' && #branded in x) {
      return x.prop === this.prop;
      //     ^ - should we narrow 'x' to 'MyClass' here?
    }
    return false;
  }
}

Pragmatically #branded in x could narrow right down from object to MyClass as in most cases the only way x could have the privateField is by being an instance of the class. Unlike 'prop' in x where there can easily be other classes that also have a 'prop' field. So this feels like it would be a nice optimisation for the developer experience.

However this check can also be made to pass in slightly more esoteric code where x is not an instance of MyClass:

class SuperReturn {
  constructor(x: any) { return x; }
}

class MyClass extends SuperReturn {
  #branded = true;
  
  constructor(x: unknown) {
    super(x);
  }

  static getBranded(x: MyClass) {
    return x.#branded;
  }
}

const a = {};
new MyClass(a);
// 'a' will now have the #branded private field
assertTrue(MyClass.getBranded(a as any));
// but is not an instance of the class
assertFalse(a instanceOf MyClass);

Do we want the type narrow to go for the (assumed) common case or for the correct case? Or maybe there is a middle ground e.g. only narrow when the classes does not extend another class (I believe this is the only way the check can be 'corrupted')

Looking at instanceOf klass checks, these will narrow to klass, even though they can also be fooled by manually setting the proptype of an object. So seems like there is good precedence here already to 'do the pragmatic thing' instead of the 'correct' thing.

@sosoba
Copy link

sosoba commented Aug 4, 2021

This feature has been marked by TC39 as finished.

I have such questions.

  1. Currently configuration target=ES2021 means transpiling to WeakMap and target=ESNext leaves code unchanged. Will it stay this way or will ES2021 containt native private fileds or will ES2022 has been added?
  2. Is there a table in the documentation that maps the target profile to transpilation scenarry?
  3. Whether the title of this issue needs to be updated?

Best regards.

@acutmore
Copy link
Contributor

acutmore commented Aug 4, 2021

ES2021 was finalised before this proposal went to stage 4 so it will be part of ES2022

@sandersn sandersn added feature-request A request for a new feature Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Aug 12, 2021
@sandersn sandersn added this to the Backlog milestone Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ES Next New featurers for ECMAScript (a.k.a. ESNext) feature-request A request for a new feature Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants