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.

Sign up for GitHub

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

[typespec-vscode] Add autocomplete of model properties for union type #5483

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[typespec-vscode] Add autocomplete of model properties for union type #5483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b0ac596
Merge pull request #1 from microsoft/main
mzhongl524 Nov 22, 2024
3fcbe52
Merge branch 'microsoft:main' into main
mzhongl524 Nov 25, 2024
c109816
Merge branch 'microsoft:main' into main
mzhongl524 Dec 6, 2024
23f0108
Merge branch 'microsoft:main' into main
mzhongl524 Dec 12, 2024
6f04f65
Merge branch 'microsoft:main' into main
mzhongl524 Dec 23, 2024
688ca03
Merge branch 'microsoft:main' into main
mzhongl524 Dec 24, 2024
de3725e
Merge branch 'microsoft:main' into main
mzhongl524 Dec 30, 2024
8130ffb
Merge branch 'microsoft:main' into main
mzhongl524 Jan 3, 2025
5d256d1
Add auto complete of value/model properties when the type is a union
mzhongl524 Jan 3, 2025
c5f2228
updated
mzhongl524 Jan 13, 2025
478417a
updated
mzhongl524 Jan 15, 2025
67ab6e8
updated
mzhongl524 Jan 17, 2025
5d5de9e
updated
mzhongl524 Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 93 additions & 49 deletions packages/compiler/src/core/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timotheeguerin, any suggestion on this? any concern to change resolveIdentifier to return sym[] | undefined? or better suggestion? thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah its fine I think, but as its breaking already would also rename it to be a bit more accurate of what its doing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sym = getMemberSymbol(model.node!.symbol, id.sv);
if (model.length === 1) {
sym = getMemberSymbol(model[0].node!.symbol, id.sv);
} else {
return undefined;
}
Expand Down Expand Up @@ -2617,7 +2617,7 @@ export function createChecker(program: Program, resolver: NameResolver): Checker

function getReferencedModel(
propertyNode: ObjectLiteralPropertyNode | ModelPropertyNode,
): Model | undefined {
): Model[] {
type ModelOrArrayValueNode = ArrayLiteralNode | ObjectLiteralNode;
type ModelOrArrayTypeNode = ModelExpressionNode | TupleExpressionNode;
type ModelOrArrayNode = ModelOrArrayValueNode | ModelOrArrayTypeNode;
Expand Down Expand Up @@ -2660,9 +2660,8 @@ export function createChecker(program: Program, resolver: NameResolver): Checker
refType = getReferencedTypeFromConstAssignment(foundNode as ModelOrArrayValueNode);
break;
}
return refType?.kind === "Model" || refType?.kind === "Tuple"
? getNestedModel(refType, path)
: undefined;

return getNestedModel(refType, path);

function pushToModelPath(node: Node, preNode: Node | undefined, path: PathSeg[]) {
if (node.kind === SyntaxKind.ArrayLiteral || node.kind === SyntaxKind.TupleExpression) {
Expand All @@ -2681,38 +2680,75 @@ export function createChecker(program: Program, resolver: NameResolver): Checker
}
}

function getNestedModel(
modelOrTuple: Model | Tuple | undefined,
path: PathSeg[],
): Model | undefined {
let cur: Type | undefined = modelOrTuple;
for (const seg of path) {
function getNestedModel(modelOrTupleOrUnion: Type | undefined, path: PathSeg[]): Model[] {
let cur = modelOrTupleOrUnion;

if (cur && cur.kind !== "Model" && cur.kind !== "Tuple" && cur.kind !== "Union") {
return [];
}

if (path.length === 0) {
// Handle union and model type nesting when path is empty
switch (cur?.kind) {
case "Tuple":
if (
seg.tupleIndex !== undefined &&
seg.tupleIndex >= 0 &&
seg.tupleIndex < cur.values.length
) {
cur = cur.values[seg.tupleIndex];
} else {
return undefined;
}
break;
case "Model":
if (cur.name === "Array" && seg.tupleIndex !== undefined) {
cur = cur.templateMapper?.args[0] as Model;
} else if (cur.name !== "Array" && seg.propertyName) {
cur = cur.properties.get(seg.propertyName)?.type;
} else {
return undefined;
return [cur];
case "Union":
const models: Model[] = [];
for (const variant of cur.variants.values()) {
if (
variant.type.kind === "Model" ||
variant.type.kind === "Tuple" ||
variant.type.kind === "Union"
) {
models.push(...(getNestedModel(variant.type, path) ?? []));
}
}
break;
return models;
default:
return undefined;
return [];
}
}
return cur?.kind === "Model" ? cur : undefined;

const seg = path[0];
switch (cur?.kind) {
case "Tuple":
if (
seg.tupleIndex !== undefined &&
seg.tupleIndex >= 0 &&
seg.tupleIndex < cur.values.length
) {
return getNestedModel(cur.values[seg.tupleIndex], path.slice(1));
} else {
return [];
}

case "Model":
if (cur.name === "Array" && seg.tupleIndex !== undefined) {
cur = cur.templateMapper?.args[0] as Model;
} else if (cur.name !== "Array" && seg.propertyName) {
cur = cur.properties.get(seg.propertyName)?.type;
} else {
return [];
}
return getNestedModel(cur, path.slice(1));

case "Union":
// When seg.property name exists, it means that it is in the union model or tuple,
// and the corresponding model or tuple needs to be found recursively.
const models: Model[] = [];
for (const variant of cur.variants.values()) {
if (
variant.type.kind === "Model" ||
variant.type.kind === "Tuple" ||
variant.type.kind === "Union"
) {
models.push(...(getNestedModel(variant.type, path) ?? []));
}
}
return models;
default:
return [];
}
}

function getReferencedTypeFromTemplateDeclaration(node: ModelOrArrayNode): Type | undefined {
Expand Down Expand Up @@ -2905,26 +2941,12 @@ export function createChecker(program: Program, resolver: NameResolver): Checker
kind === IdentifierKind.ObjectLiteralProperty
) {
const model = getReferencedModel(ancestor as ModelPropertyNode | ObjectLiteralPropertyNode);
if (!model) {
if (model.length <= 0) {
return completions;
}
const curModelNode = ancestor.parent as ModelExpressionNode | ObjectLiteralNode;

for (const prop of walkPropertiesInherited(model)) {
if (
identifier.sv === prop.name ||
!curModelNode.properties.find(
(p) =>
(p.kind === SyntaxKind.ModelProperty ||
p.kind === SyntaxKind.ObjectLiteralProperty) &&
p.id.sv === prop.name,
)
) {
const sym = getMemberSymbol(model.node!.symbol, prop.name);
if (sym) {
addCompletion(prop.name, sym);
}
}
for (const curModel of model) {
addInheritedPropertyCompletions(curModel, curModelNode);
}
} else if (identifier.parent && identifier.parent.kind === SyntaxKind.MemberExpression) {
let base = resolver.getNodeLinks(identifier.parent.base).resolvedSymbol;
Expand Down Expand Up @@ -2986,6 +3008,28 @@ export function createChecker(program: Program, resolver: NameResolver): Checker

return completions;

function addInheritedPropertyCompletions(
model: Model,
curModelNode: ModelExpressionNode | ObjectLiteralNode,
) {
for (const prop of walkPropertiesInherited(model)) {
if (
identifier.sv === prop.name ||
!curModelNode.properties.find(
(p) =>
(p.kind === SyntaxKind.ModelProperty ||
p.kind === SyntaxKind.ObjectLiteralProperty) &&
p.id.sv === prop.name,
)
) {
const sym = getMemberSymbol(model.node!.symbol, prop.name);
if (sym) {
addCompletion(prop.name, sym);
}
}
}
}

function addCompletions(table: SymbolTable | undefined) {
if (!table) {
return;
Expand Down
112 changes: 112 additions & 0 deletions packages/compiler/test/server/completion.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,118 @@ describe("identifiers", () => {
);
});

it("completes union variants(models) of template parameters", async () => {
const completions = await complete(
`
model Options {
a: string;
b: Nested;
}
model Nested {
c:Foo2;
}
model Foo1 {
foo1: string;
}
model Foo2 {
foo2: string;
}

model Test<T extends valueof string | Foo1 | Options> {}

alias A = Test<#{┆}>;
`,
);

check(
completions,
[
{
label: "foo1",
insertText: "foo1",
kind: CompletionItemKind.Field,
documentation: {
kind: MarkupKind.Markdown,
value: "(model property)\n```typespec\nFoo1.foo1: string\n```",
},
},
{
label: "a",
insertText: "a",
kind: CompletionItemKind.Field,
documentation: {
kind: MarkupKind.Markdown,
value: "(model property)\n```typespec\nOptions.a: string\n```",
},
},
{
label: "b",
insertText: "b",
kind: CompletionItemKind.Field,
documentation: {
kind: MarkupKind.Markdown,
value: "(model property)\n```typespec\nOptions.b: Nested\n```",
},
},
],
{
allowAdditionalCompletions: false,
},
);
});

it("completes specific type in union variants(models) of template parameters", async () => {
const completions = await complete(
`
model Options {
a: string;
b: Nested;
}
model Nested {
c:Foo2;
d:string;
}
model Foo1 {
foo1: string;
}
model Foo2 {
foo2: string;
}

model Test<T extends valueof string | Foo1 | Options> {}

alias A = Test<#{a:"",b:#{┆}}>;
`,
);

check(
completions,
[
{
label: "c",
insertText: "c",
kind: CompletionItemKind.Field,
documentation: {
kind: MarkupKind.Markdown,
value: "(model property)\n```typespec\nNested.c: Foo2\n```",
},
},
{
label: "d",
insertText: "d",
kind: CompletionItemKind.Field,
documentation: {
kind: MarkupKind.Markdown,
value: "(model property)\n```typespec\nNested.d: string\n```",
},
},
],
{
allowAdditionalCompletions: false,
},
);
});

it("completes namespace operations", async () => {
const completions = await complete(
`
Expand Down
Loading