[typespec-vscode] Add autocomplete of model properties for union type #5483
Conversation
mzhongl524
requested review from
bterlson,
markcowl,
allenjzhang and
timotheeguerin
as code owners
microsoft-github-policy-service
bot
added
the
compiler:core
|
❌ There is undocummented changes. Run The following packages have changes but are not documented.
Show changes |
|
You can try these changes here
|
| @@ -2549,8 +2549,8 @@ export function createChecker(program: Program, resolver: NameResolver): Checker | |||
| case IdentifierKind.ModelExpressionProperty: | |||
| case IdentifierKind.ObjectLiteralProperty: | |||
| const model = getReferencedModel(node as ModelPropertyNode | ObjectLiteralPropertyNode); | |||
| if (model) { | |||
There was a problem hiding this comment.
@timotheeguerin , @chrisradek , could you help to suggest what we should do here if multiple models are returned here because of union? Shall we change the resolveIdentifier to return an array too?
There was a problem hiding this comment.
I think this method got hijacked a little bit for what it was supposed to do(resolve the identifier this points to) and
is now used to show which identifier it should use for completion.
I think we need to refactor things a little here so that either a new function porvide all the potential linked types or we just expose the completions and the resolution is internal
There was a problem hiding this comment.
Aren't they the same thing? Could you provide more detail about the difference? I mean the resolved model(s) is the same thing for completion or for resolving identifier, isn't it? thanks.
There was a problem hiding this comment.
well kind of but also not really, it was originally meant as a way to resolve to which declaration an identifier points to so you can do the highlight, complete, etc. but with the addition of the model/value completion it now is looking at the linked type of the value not itself which kinda make this name confusing.
There was a problem hiding this comment.
yeah, you are right. They are slightly different. But we still have the problem that an identifierNode may be resolved to multiple sym when Union is considered like below, any suggestion on it? shall we update the resolveIdentifier to return sym array? I am a little concern about the impact from that change.
There was a problem hiding this comment.
@timotheeguerin, any suggestion on this? any concern to change resolveIdentifier to return sym[] | undefined? or better suggestion? thanks.
There was a problem hiding this comment.
yeah its fine I think, but as its breaking already would also rename it to be a bit more accurate of what its doing
There was a problem hiding this comment.
To make sure I understand it correctly. The current logic here is first looking at the linked types for property identifier node, and then after the linked types are found, it will further use getMemberSymbol(...) to get and return the sym of the corresponding property (the identifier node). So do you mean this sym is not a declaration and different from other sym as declaration? Do you have any suggestion for the rename? thanks.
RodgeFu
changed the title
Fix : #4024
add code and tests