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.

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

feat(compiler): Bring back the Map<K, V> type #5573

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Copy link
Member

Choose a reason for hiding this comment

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

The thing missing with Map is deciding on how to serialize it. Keeping it as a JS object only work if the key is a string or a number. So as much as it might unlock the number case it now opens up a wide range of things that cannot be represented without defining an alternate schema

For example as:

  • Tuples [K, V][]
  • Key value object {key: K, value: V}[]

So to bring back this type in I believe we'd need to figure how to define how a map is encoded.

Copy link
Member

Choose a reason for hiding this comment

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

yeah Map<K,V> in js is not serializable therefore in TCGC we made an update to avoid of using Map<K,V> in its public APIs, because we have some emitters that need to transport the output of TCGC into another process written in their own language (such as .net, java and python)

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: feature
packages:
- "@typespec/compiler"
---

Bring back the `Map<K, V>` type to intrinsics
10 changes: 9 additions & 1 deletion packages/compiler/lib/intrinsics.tsp
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,16 @@ scalar boolean;
model Array<Element> {}

/**
* @dev Model with string properties where all the properties have type `Property`
* @dev Model with string properties where all the properties have type `Element`
* @template Element The type of the properties
*/
@indexer(string, Element)
model Record<Element> {}

/**
* @dev Model with string properties where all the keys have type `Key` and all the properties have type `Element`
* @template Key The type of the keys
* @template Element The type of the properties
*/
@indexer(Key, Element)
model Map<Key extends string, Element> {}
2 changes: 1 addition & 1 deletion packages/compiler/src/core/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ export type NeverIndexer = {
};

export type ModelIndexer = {
readonly key: Scalar;
readonly key: Enum | Scalar | Union;
Copy link
Member

Choose a reason for hiding this comment

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

does this mean the key here could be an enum, or scalar or union of enums and/or scalars?
How should we understand the Union here?
would a union of models be OK here?
Also I did not see any test cases in this PR when key is an enum or union, therefore I am really curious what the union here stands for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added Union here for inline enum-like types, such as "dog" | "cat". AFAIK "dog" will be a Scalar type but "dog" | "cat" sill be a Union type.

Union of models won't be ok here, at least the key type must be assignable to string for Maps, by the extends keyword. However the ModelIndexer type cannot be constrained to such a limitation.

Please let me know if there is a better approach! 🙏 I will add missing test cases for enums though.

Copy link
Member

Choose a reason for hiding this comment

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

so the union here actually means union of literals of enums and scalars.
if the compiler could prevent all those unexpected usage scenarios, this is actually fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. The compiler can check the key type for Map types, but IIRC the compiler currently doesn't check argument types for @indexer decorator as it's an intrinstics decorator without any declarations.

readonly value: Type;
};

Expand Down
11 changes: 9 additions & 2 deletions packages/compiler/src/lib/intrinsic/decorators.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
import { DocTarget, setDocData } from "../../core/intrinsic-type-state.js";
import type { Program } from "../../core/program.js";
import type { DecoratorContext, ModelIndexer, Scalar, Type } from "../../core/types.js";
import type {
DecoratorContext,
Enum,
ModelIndexer,
Scalar,
Type,
Union,
} from "../../core/types.js";

const indexTypeKey = Symbol.for(`TypeSpec.index`);
export const indexerDecorator = (
context: DecoratorContext,
target: Type,
key: Scalar,
key: Enum | Scalar | Union,
value: Type,
) => {
const indexer: ModelIndexer = { key, value };
Expand Down
57 changes: 57 additions & 0 deletions packages/compiler/test/checker/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,63 @@ describe("compiler: models", () => {
strictEqual(options[1].name, "string");
});

it("can spread a Map<K, V>", async () => {
testHost.addTypeSpecFile("main.tsp", `@test model Test {...Map<string, int32>;}`);
const { Test } = (await testHost.compile("main.tsp")) as {
Test: Model;
};
ok(isRecordModelType(testHost.program, Test));
strictEqual(Test.indexer?.key.name, "string");
strictEqual(Test.indexer?.value.kind, "Scalar");
strictEqual(Test.indexer?.value.name, "int32");
});

it("can spread a Map<K, V> with different value than existing props", async () => {
testHost.addTypeSpecFile(
"main.tsp",
`
@test model Test {
name: string;
...Map<string, int32>;
}
`,
);
const { Test } = (await testHost.compile("main.tsp")) as {
Test: Model;
};
ok(isRecordModelType(testHost.program, Test));
const nameProp = Test.properties.get("name");
strictEqual(nameProp?.type.kind, "Scalar");
strictEqual(nameProp?.type.name, "string");
strictEqual(Test.indexer?.key.name, "string");
strictEqual(Test.indexer?.value.kind, "Scalar");
strictEqual(Test.indexer?.value.name, "int32");
});

it("can spread different maps", async () => {
testHost.addTypeSpecFile(
"main.tsp",
`
@test model Test {
...Map<string, int32>;
...Map<string, string>;
}
`,
);
const { Test } = (await testHost.compile("main.tsp")) as {
Test: Model;
};
ok(isRecordModelType(testHost.program, Test));
strictEqual(Test.indexer?.key.name, "string");
const indexerValue = Test.indexer?.value;
strictEqual(indexerValue.kind, "Union");
const options = [...indexerValue.variants.values()].map((x) => x.type);
strictEqual(options[0].kind, "Scalar");
strictEqual(options[0].name, "int32");
strictEqual(options[1].kind, "Scalar");
strictEqual(options[1].name, "string");
});

it("emit diagnostic if spreading an T[]", async () => {
testHost.addTypeSpecFile(
"main.tsp",
Expand Down
93 changes: 93 additions & 0 deletions packages/compiler/test/checker/relation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ describe("compiler: checker: type relations", () => {
"numeric",
"float",
"Record<string>",
"Map<string, string>",
"bytes",
"duration",
"plainDate",
Expand Down Expand Up @@ -290,6 +291,7 @@ describe("compiler: checker: type relations", () => {
"numeric",
"float",
"Record<string>",
"Map<string, string>",
"bytes",
"duration",
"plainDate",
Expand Down Expand Up @@ -859,6 +861,97 @@ describe("compiler: checker: type relations", () => {
});
});

describe("Map<x, y> target", () => {
["Map<string, integer>"].forEach((x) => {
it(`can assign ${x}`, async () => {
await expectTypeAssignable({ source: x, target: "Map<string, integer>" });
});
});

it("can assign empty object", async () => {
await expectTypeAssignable({ source: "{}", target: "Map<string, string>" });
});

it("can assign object with property being the same type", async () => {
await expectTypeAssignable({ source: "{foo: string}", target: "Map<string, string>" });
await expectTypeAssignable({
source: "{foo: string, bar: string}",
target: "Map<string, string>",
});
});

it("can assign object with property being the of subtype type", async () => {
await expectTypeAssignable({ source: "{foo: int32}", target: "Map<string, numeric>" });
await expectTypeAssignable({
source: "{foo: float, bar: int64}",
target: "Map<string, numeric>",
});
});

it("can assign a map of subtypes", async () => {
await expectTypeAssignable({ source: "Map<string, int32>", target: "Map<string, numeric>" });
});

it("can assign object that implement the same indexer", async () => {
await expectTypeAssignable({
source: "Foo",
target: "Map<string, string>",
commonCode: `
model Foo is Map<string, string> {
prop1: string;
prop2: string;
}
`,
});
});

it("type with spread indexer allow other properties to no match index", async () => {
await expectTypeAssignable({
source: "{age: int32, other: string}",
target: "Foo",
commonCode: `
model Foo {
age: int32;
...Map<string, string>;
}
`,
});
});

it("emit diagnostic assigning other type", async () => {
await expectTypeNotAssignable(
{ source: `string`, target: "Map<string, string>" },
{
code: "unassignable",
message: "Type 'string' is not assignable to type 'Map<string, string>'",
},
);
});

it("emit diagnostic assigning Map of incompatible type", async () => {
await expectTypeNotAssignable(
{ source: `Map<string, int32>`, target: "Map<string, string>" },
{
code: "unassignable",
message: [
`Type 'Map<string, int32>' is not assignable to type 'Map<string, string>'`,
" Type 'int32' is not assignable to type 'string'",
].join("\n"),
},
);
});

it("emit diagnostic if some properties are different type", async () => {
await expectTypeNotAssignable(
{ source: `{foo: string, bar: int32}`, target: "Map<string, string>" },
{
code: "unassignable",
message: "Type 'int32' is not assignable to type 'string'",
},
);
});
});

describe("models", () => {
it("can assign empty object", async () => {
await expectTypeAssignable({ source: "{}", target: "{}" });
Expand Down
1 change: 1 addition & 0 deletions packages/compiler/test/stdlib.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const intrinsicTypes = [
"decimal",
"Array<string>",
"Record<string>",
"Map<string, integer>",
"string[]",
];

Expand Down
Loading
Loading