es private fields in in

[acutmore]

*Issue number of the reported bug or feature request: *
microsoft#42574

Describe your changes
Implements the stage 3 'private-fields-in-in' proposal

https://github.com/tc39/proposal-private-fields-in-in

WIP Progress

• 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
• Add new error messages instead of piggybacking on existing diagnostics
• Use in flowNodes and type narrowing
• language server hints
  • suggest completion for privateFields on typing #
• emit ESNext
• emit down-level
• part of find references results
• private static field/method compatibility

[dragomirtitian]

Maybe we should think of common parsing mistakes like if(#prop.a in foo). This currently issues the error 'in' expected.. I would argue an error along the lines of: You might have meant this.#props.a would be better,

[acutmore]

Looking into this there are existing errors and quickFixes for 'bare' privateFields but my branch was preventing them from running as I was consuming the field in the parser. I've changed the parser to lookahead and only match when the in keyword is present.

[dragomirtitian]

I wonder if we shouldn't be more strict here. I mean for b instanceof Foo it makes sense since you have Symbol.species which can customize how instanceof works. But for #p1 in b there is not universe in which #p1 can be in b and it also be an instance of Bar (if Bar is a class). For other object types this still makes sense... so I'm rather torn if we should issue an error or not.

[acutmore]

Interesting. I'm thinking not unless the rule is applied more generally, as in all productions of ClassA & ClassB would be never if at least one class had private fields, and the other was not a sub/superType.

Surprisingly there are cases where the runtime type here could be Bar and not Foo, though I wouldn't expect this to be common code:

class SuperReturn {
  constructor(v) { return v; }
}
class C extends SuperReturn {
  #priv;
  static isC(v) { return #priv in v }
  cMethod() {}
}
class Unrelated {
  random = 1;
  uMethod();
}

const u = new C(new Unrelated));
if (C.isC(u)) {
  u.random; // 1
  u.uMethod(); // fine
  u.cMethod(); // throws
}

[acutmore]

Another thing I've noticed is that #priv in v is not coming up in 'find all references' searches. So I need to fix that too.
EDIT: done

[acutmore]

@dragomirtitian - I've rebased onto TypeScript post the private methods & static fields PR. And ready for another round of review when you have the time.

[dragomirtitian]

Wasn't there a valueDeclaration that is what is needed here ?

[acutmore]

ah yes there is symbol.valueDeclaration - that looks much better. I'll change to that.

[acutmore]

done :)

[dragomirtitian]

use hasStaticModifier instead

[acutmore]

done :)

[dragomirtitian]

There seems to be a better way to get the instance type:

const classSymbol = getSymbolOfNode(classDecl);
const classInstanceType = <InterfaceType>getDeclaredTypeOfSymbol(classSymbol);

At line (24161, fn: classDeclarationExtendsNull)

Not sure if 100% equivalent or if better, but please consider it.

[acutmore]

done. Much better, thanks!

[dragomirtitian]

Nit: Consider removing this error from the test by changing compiler settings. It is noise here IMO

[acutmore]

done :)

[dragomirtitian]

Took a second to get this was actually for (let f in (#field in v as any)) 😅.

[acutmore]

haha! I hope no one ever does this but at least we support it if they try

[mkubilayk]

Tried it out in VSCode, this is looking good! There is one case I found a bit inconvenient but I'm not sure if we can improve it. When you are inside PrivateIdentifierInInExpression and editing the identifier:

if (# in this) {
     ^ cursor
}

available private identifiers are listed correctly in the suggestions (ctrl+space). But when you pick one if them, you get a property access (this.#foo). If we could use the contextual information to drive this and insert #foo instead, that would be great. If you think people might want this.#foo more often than #foo in this case, please ignore my suggestion.

[mkubilayk]

nit: This can be combined with SyntaxKind.AsExpression case.

[acutmore]

Done :)

[mkubilayk]

Should this use isPrivateIdentifier check?

Suggested change

	nodeVisitor((<PrivateIdentifierInInExpression>node).name, visitor, isExpression),
	nodeVisitor((<PrivateIdentifierInInExpression>node).name, visitor, isPrivateIdentifier),

[acutmore]

If my memory serves correctly: no. As I believe this check is also done on the transformed AST where PrivateIdentifierInInExpression becomes a methodCall.

This was over a month ago so I may be getting this mixed up with a different bit of code. I'll try it out again and confirm either way.

[acutmore]

Played around to jog my memory. The reason this was isExpression and not isPrivateIdentifier was because when the identifier is invalid (e.g. used outside of a class) the classFields transformer emits an empty plain Identifier.

I've changed this to isMemberName now to match both private and standard identifiers.

[acutmore]

Tried it out in VSCode, this is looking good! There is one case I found a bit inconvenient but I'm not sure if we can improve it. When you are inside PrivateIdentifierInInExpression and editing the identifier:

if (# in this) {
     ^ cursor
}

available private identifiers are listed correctly in the suggestions (ctrl+space). But when you pick one if them, you get a property access (this.#foo). If we could use the contextual information to drive this and insert #foo instead, that would be great. If you think people might want this.#foo more often than #foo in this case, please ignore my suggestion.

Yeah I was really unsure on this too. My assumption was that people are more likely to reach for accessing the private field but that's not based on any evidence just a gut feeling.

Maybe it's possible to offer both suggestions a plain #foo identifier when we're preceding an in and the this.#foo. I'll give it a go and see what the API supports.

[acutmore]

I spent a little bit of time seeing if I could change the completions private fields to sometimes complete to just the identifier but couldn't get anything working quickly. I'll have another go next time I have some time.
I believe I've addressed the other comments.

[acutmore]

Merged latest TypeScript main into this branch. All tests still passing.

[mkubilayk]

I think this looks good. Left one comment to prevent against a potential future bug.

We can use microsoft#42458 as an example for the PR description when we open the upstream PR.

[mkubilayk]

This is probably fine today but might be a problem when ES2022 is introduced. shouldTransformPrivateElements will probably be updated to say languageVersion < ScriptTarget.ES2022. But it's possible that proposal-private-fields-in-in won't make it to ES2022.

[acutmore]

The current plan is to assume https://github.com/tc39/proposal-private-fields-in-in will make stage 4 in time for ES2022. There are positive signals of this, as it is already available in latest Chrome and Firefox.