Use brand types to clear up confusion about entity name expressions #10013
Conversation
ghost
force-pushed
the
resolve_entity_name
branch
from
e6ca44e to
f763280
Compare
| @@ -4947,15 +4959,14 @@ namespace ts { | |||
| const typeReferenceName = getTypeReferenceName(node); | |||
| symbol = resolveTypeReferenceName(node, typeReferenceName); | |||
| type = getTypeReferenceType(node, symbol); | |||
|
|
|||
| links.resolvedSymbol = symbol; | |||
| links.resolvedType = type; | |||
There was a problem hiding this comment.
There were duplicate lines just below these.
…on, not just identifiers.
ghost
force-pushed
the
resolve_entity_name
branch
from
9ed6146 to
f9fd496
Compare
| node.kind === SyntaxKind.ExportAssignment && exportAssignmentIsAlias(<ExportAssignment>node); | ||
| } | ||
|
|
||
| export function exportAssignmentIsAlias(node: ExportAssignment): boolean { |
There was a problem hiding this comment.
exportAssignmentIsAlias [](start = 20, length = 23)
it seems weird that there are 3 functions whose bodies are all just isEntityNameExpression(node.expression). If you express them as a single function, does any common name look at all good? #Closed
…he expression of each `ExpressionWithTypeArguments` is an `EntityNameExpression`.
| * Climbs up parents to an ExpressionWithTypeArguments, and returns its expression, | ||
| * but returns undefined if that expression is not an EntityNameExpression. | ||
| */ | ||
| function climbToEntityNameOfExpressionWithTypeArguments(node: Node): EntityNameExpression | undefined { |
There was a problem hiding this comment.
climbToEntityNameOfExpressionWithTypeArguments [](start = 17, length = 46)
getEntityNameParent[OfExpressionWithTypeArguments]? (get...Parent is the usual pattern used rather than climbTo...)
There was a problem hiding this comment.
This one would also be simpler with recursion.
There was a problem hiding this comment.
We're getting the child of an ancestor (e.g. an aunt) which is an ExpressionWithTypeArguments. So it's more like getExpressionOfExpressionWithTypeArgumentsParent...
There was a problem hiding this comment.
good point, I missed that. Maybe move the .expression access to the caller? This is more like how it works with the rest of your new code post-commit 4 anyway.
In reply to: 73203379 [](ancestors = 73203379)
There was a problem hiding this comment.
Don't want to move .expression to the caller because climbToEntityNameOfExpressionWithTypeArguments is responsible for ensuring that .expression is a EntityNameExpression. Maybe it could just be named getEntityNameForExtendingInterface?
|
👍 once @vladima takes a look at the possible recursions, and once you decide whether to change the name for |
| * but returns undefined if that expression is not an EntityNameExpression. | ||
| */ | ||
| function climbToEntityNameOfExpressionWithTypeArguments(node: Node): EntityNameExpression | undefined { | ||
| for (; ; ) { |
|
I do not think this is not going to work for things that do not share the same symbol.. e.g. class C {
static B: number;
}
namespace C {
export interface B { c: number }
}
export = C.B; |
ghost
mentioned this pull request
Fixes #4325.
This introduces the type
EntityNameExpressionto replaceExpressionused in a context where it must be an entity name (an identifier, or a property access where the LHS is an entity name).This also helped to discover a bug where we treated exported property accesses as entity names even if the LHS wasn't propery.