allow PrivateIdentifier in QualifiedName

[a-tarasyuk]

Fixes #47696 (comment)

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[jakebailey]

This seems like an API change that has to happen thanks to the cast noted in: #47696 (comment)

[jakebailey]

We looked at this in the design meeting, and we think that it's the case that the right change is to instead make TypeQueryNode['exprName'] be PropertyAccessEntityNameExpression instead of EntityName.

[a-tarasyuk]

@jakebailey As I understand your last comment, need to do the following list of items

1. Change the name type from Identifier to MemberName

TypeScript/src/compiler/types.ts

Lines 2354 to 2358 in 45faac7

	export interface PropertyAccessEntityNameExpression extends PropertyAccessExpression {
	_propertyAccessExpressionLikeQualifiedNameBrand?: any;
	readonly expression: EntityNameExpression;
	readonly name: Identifier;
	}

2. Add PrivateIdentifier parsing support to parsePropertyAccessEntityNameExpression

TypeScript/src/compiler/parser.ts

Lines 8449 to 8457 in 45faac7

	function parsePropertyAccessEntityNameExpression() {
	const pos = getNodePos();
	let node: Identifier | PropertyAccessEntityNameExpression = parseJSDocIdentifierName();
	while (parseOptional(SyntaxKind.DotToken)) {
	const name = parseJSDocIdentifierName();
	node = finishNode(factory.createPropertyAccessExpression(node, name), pos) as PropertyAccessEntityNameExpression;
	}
	return node;
	}

3. Change parseEntityName to parsePropertyAccessEntityNameExpression

   TypeScript/src/compiler/parser.ts

   Line 3191 in 45faac7

   const entityName = parseEntityName(/*allowReservedWords*/ true, /*allowPrivateIdentifiers*/ true);

4. Change exprName: EntityName to expression: PropertyAccessEntityNameExpression and node.exprName = exprName; to node.expression = expression;

TypeScript/src/compiler/factory/nodeFactory.ts

Lines 1885 to 1891 in 45faac7

	function createTypeQueryNode(exprName: EntityName, typeArguments?: readonly TypeNode[]) {
	const node = createBaseNode<TypeQueryNode>(SyntaxKind.TypeQuery);
	node.exprName = exprName;
	node.typeArguments = typeArguments && parenthesizerRules().parenthesizeTypeArguments(typeArguments);
	node.transformFlags = TransformFlags.ContainsTypeScript;
	return node;
	}

Am I right?

[jakebailey]

I think there may have been a miscommunication when I read the notes, because maybe it's PropertyAccessExpression that is the one to be using here. What you've listed seems like too much work. @rbuckton @andrewbranch

[a-tarasyuk]

@jakebailey I think we can try to change TypeQuery from entityName to an expression. It will even resolve this issue #19707.

[jakebailey]

Maybe; I can see that coming up in #48763.

[andrewbranch]

I think we shouldn’t change the behavior of #19707 at this point post-beta. My suggestion would be (for now) to parse only a dotted name still, and pick whatever legal type for the API makes sense. Expression would let us change #19707 in the future without changing the type again, but I’m not too concerned about widening the type of TypeQuery['exprName'] more in the future if we need to.

(Note: changing PropertyAccessEntityNameExpression might make sense in order to make the narrowest possible type for what we will parse here, but its parsing functions and most of its existing uses are JSDoc-specific. I’m not sure it’s worth it to mess with that.)

[jakebailey]

I'm looking at this currently, and the trouble I'm seeing is that TypeQuery contains an EntityName, which is recursive with QualifiedName:

    export interface QualifiedName extends Node {
        readonly kind: SyntaxKind.QualifiedName;
        readonly left: EntityName;
        readonly right: Identifier;
        /*@internal*/ jsdocDotPos?: number;
    }

    export type EntityName = Identifier | QualifiedName;

What was broken about #47696 was that there was a cast to Identifier that hid the fact that while parsing entity names we now could get PrivateIdentifier too and add it to the chain of things in the qualified name.

So if we make TypeQuery have a plain Expresssion rather than an EntityName, that wouldn't change the fact that the new behavior allows PrivateIdentifier in a qualified name, which people in the design meeting didn't seem to like.

I almost think we should revert #47696 (then approach #19707 in a later release), unless you're seeing a more reasonable parser change here than I am. It seems like the options are:

• Make qualified name allow a private identifier in the chain (people didn't like this)
• Parse out a PropertyAccessExpression instead, which is a chain that allows it and is similar, but that expression could technically be something like this.foo['xyz'].bar which is more than a dotted name.
• Parse out EntityNameOrEntityNameExpression by adding private identifiers to PropertyAccessEntityNameExpression, somehow, but as mentioned, that's all JSDoc stuff. But, EntityName/QualifiedName are already also very weird too and aren't really reachable via normal expressions?

I do think making it Expression is the "right" thing long term, at least on my view of the operator being "the type of what you get when you write out this expression".

[a-tarasyuk]

I almost think we should revert #47696 (then approach #19707 in a later release),

Oke, I'll add changes to revert #47696

[jakebailey]

To be clear, this is just my opinion; I wouldn't change anything just yet as we haven't actually re-met to discuss a way forward (I'm just writing my thoughts down so I don't lose them). If you want to do that, I would send a different PR so we can still see what's going on in this one.

[andrewbranch]

So if we make TypeQuery have a plain Expresssion rather than an EntityName, that wouldn't change the fact that the new behavior allows PrivateIdentifier in a qualified name, which people in the design meeting didn't seem to like.

What? This doesn’t follow. Changing TypeQuery was proposed specifically to avoid changing QualifiedName/EntityName. Using Expression or PropertyAccessExpression for exprName avoids that. Above, I was proposing that TypeQuery['exprName'] be a PropertyAccessExpression or Expression, but we should only parse a dotted name into it for now, in order to make a fairly minimal parsing change this late in the release cycle. The API type will therefore be much broader than the values that inhabit it.

I actually think instead of Expression or PropertyAccessExpression, perhaps we should make a new type alias which is essentially the version of PropertyAccessEntityNameExpression that can include private identifiers—in other words, the narrowest accurate type we can write for TypeQuery['exprName']. This should give an easier migration path to API consumers (including us). The other option is to make a new type identical to QualifiedName except that its right is MemberName instead of Identifier. This would probably be the least breaky API change we could make, but we should be sure we won’t want to change it to Expression or something later and then have a totally unused Node type in our public API.

I do think making it Expression is the "right" thing long term, at least on my view of the operator being "the type of what you get when you write out this expression".

FWIW this interpretation of typeof was explored and explicitly rejected: #6606 (comment)

It does sound like we should probably discuss this again briefly next Wednesday before putting more work into it, as what we do with the API now depends on what we want to do with the API long-term, which I think depends at least on whether we actually want to address #19707. Sorry for the confusion @a-tarasyuk.

[jakebailey]

What? This doesn’t follow. Changing TypeQuery was proposed specifically to avoid changing QualifiedName/EntityName. Using Expression or PropertyAccessExpression for exprName avoids that. Above, I was proposing that TypeQuery['exprName'] be a PropertyAccessExpression or Expression, but we should only parse a dotted name into it for now, in order to make a fairly minimal parsing change this late in the release cycle. The API type will therefore be much broader than the values that inhabit it.

Sorry, I was just trying to state that regardless of the decision to change TypeQuery['exprName'] to Expression that we'd still have to come up with a plan for what the actual tree looks like underneath it, since the current state isn't desirable. I missed the fact that EntityName isn't an Expression at all (QualifiedName is just extends Node), so therefore changing the type to Expression can only imply changing the tree too.,

[andrewbranch]

The plan for what the tree actually looks like will follow directly from what we choose for the API type:

• Expression, PropertyAccessExpression, or new type that is recursive PropertyAccessExpression allowing only itself and MemberName: tree looks like a PropertyAccessExpression
• New type that looks like QualifiedName but allows MemberName: tree looks like it does now

There’s not so much a problem with the shape of the tree as there is a disagreement between the type and reality.

[jakebailey]

We decided to revert the original change; I'll be sending a PR shortly to do it.

[rbuckton]

I think a better approach would have been to allow #name in an indexed access type, rather than in a type query, i.e.:

class C {
  #x: Foo;
  private _x: Foo;

  method() {
    const p1: this[#x] = this.#x;
    const p2: this["_x"] = this._x;

    const obj = this;
    const p3: typeof obj[#x] = this.#x; // parsed as `(typeof obj)[#x]`
    const p4: typeof obj["_x"] = this._x; // parsed as `(typeof obj)["_x"]`
  }
}

This would mean adding a PrivateNameTypeNode and we could then avoid changing TypeQueryNode or QualifiedName in any way. I would also expect that #x in a type is only legal inside of the class body, just like how a runtime private name is scoped.

If you need to access the type of a private named class member outside of the class, you would need to introduce a type alias or interface for it. If a class makes a private-named index access type public, our declaration emit would also need to synthesize a type for it outside of the class. This means that the type import("foo").Bar[#baz] would only be legal inside of a class body that declares #baz.

[jakebailey]

@rbuckton can you stick that on #47595? I think Ryan there was arguing against it, so good to note in case we actually do have this feature.