Skip to content

Commit

Permalink
Fix: Model with spread indexer shouldn't validate explicit properites (
Browse files Browse the repository at this point in the history
…#3163)

This was causing errors in doing things like this 

```ts
model Info {
  age: int32;
  ...Record<string>;
}

model Bar<T extends Info> {}

model Test {
  t: Bar<{
    age: 123;
    other: "abc";
  }>;
}

```
  • Loading branch information
timotheeguerin authored Apr 12, 2024
1 parent 7bbe89c commit f30120d
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 58 deletions.
8 changes: 8 additions & 0 deletions .chronus/changes/fix-spread-assignment-2024-3-12-18-23-17.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
changeKind: fix
packages:
- "@typespec/compiler"
---

Fix: Model with spread indexer shouldn't validate explicit properites
130 changes: 72 additions & 58 deletions packages/compiler/src/core/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5556,8 +5556,12 @@ export function createChecker(program: Program): Checker {
}),
],
];
} else if (target.kind === "Model" && target.indexer !== undefined && source.kind === "Model") {
return isIndexerValid(
} else if (
target.kind === "Model" &&
isArrayModelType(program, target) &&
source.kind === "Model"
) {
return hasIndexAndIsAssignableTo(
source,
target as Model & { indexer: ModelIndexer },
diagnosticTarget,
Expand Down Expand Up @@ -5718,6 +5722,8 @@ export function createChecker(program: Program): Checker {
): [Related, Diagnostic[]] {
relationCache.set([source, target], Related.maybe);
const diagnostics: Diagnostic[] = [];
const remainingProperties = new Map(source.properties);

for (const prop of walkPropertiesInherited(target)) {
const sourceProperty = getProperty(source, prop.name);
if (sourceProperty === undefined) {
Expand All @@ -5735,6 +5741,8 @@ export function createChecker(program: Program): Checker {
);
}
} else {
remainingProperties.delete(prop.name);

const [related, propDiagnostics] = isTypeAssignableToInternal(
sourceProperty.type,
prop.type,
Expand All @@ -5746,6 +5754,30 @@ export function createChecker(program: Program): Checker {
}
}
}

if (target.indexer) {
const [_, indexerDiagnostics] = arePropertiesAssignableToIndexer(
remainingProperties,
target.indexer.value,
diagnosticTarget,
relationCache
);
diagnostics.push(...indexerDiagnostics);

// For anonymous models we don't need an indexer
if (source.name !== "" && target.indexer.key.name !== "integer") {
const [related, indexDiagnostics] = hasIndexAndIsAssignableTo(
source,
target as any,
diagnosticTarget,
relationCache
);
if (!related) {
diagnostics.push(...indexDiagnostics);
}
}
}

return [diagnostics.length === 0 ? Related.true : Related.false, diagnostics];
}

Expand All @@ -5756,75 +5788,57 @@ export function createChecker(program: Program): Checker {
);
}

function isIndexerValid(
source: Model,
target: Model & { indexer: ModelIndexer },
diagnosticTarget: DiagnosticTarget,
relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related>
): [Related, readonly Diagnostic[]] {
// Model expressions should be able to be assigned.
if (source.name === "" && target.indexer.key.name !== "integer") {
return isIndexConstraintValid(target.indexer.value, source, diagnosticTarget, relationCache);
} else {
if (source.indexer === undefined || source.indexer.key !== target.indexer.key) {
return [
Related.false,
[
createDiagnostic({
code: "missing-index",
format: {
indexType: getTypeName(target.indexer.key),
sourceType: getTypeName(source),
},
target: diagnosticTarget,
}),
],
];
}
return isTypeAssignableToInternal(
source.indexer.value!,
target.indexer.value,
diagnosticTarget,
relationCache
);
}
}
/**
* @param constraintType Type of the constraints(All properties must have this type).
* @param type Type of the model that should be respecting the constraint.
* @param diagnosticTarget Diagnostic target unless something better can be inferred.
*/
function isIndexConstraintValid(
constraintType: Type,
type: Model,
function arePropertiesAssignableToIndexer(
properties: Map<string, ModelProperty>,
indexerConstaint: Type,
diagnosticTarget: DiagnosticTarget,
relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related>
relationCache: MultiKeyMap<[Type, Type], Related>
): [Related, readonly Diagnostic[]] {
for (const prop of type.properties.values()) {
const [related, diagnostics] = isTypeAssignableTo(
for (const prop of properties.values()) {
const [related, diagnostics] = isTypeAssignableToInternal(
prop.type,
constraintType,
diagnosticTarget
);
if (!related) {
return [Related.false, diagnostics];
}
}

if (type.baseModel) {
const [related, diagnostics] = isIndexConstraintValid(
constraintType,
type.baseModel,
indexerConstaint,
diagnosticTarget,
relationCache
);
if (!related) {
return [Related.false, diagnostics];
}
}

return [Related.true, []];
}

/** Check that the source model has an index, the index key match and the value of the source index is assignable to the target index. */
function hasIndexAndIsAssignableTo(
source: Model,
target: Model & { indexer: ModelIndexer },
diagnosticTarget: DiagnosticTarget,
relationCache: MultiKeyMap<[Type | ValueType, Type | ValueType], Related>
): [Related, readonly Diagnostic[]] {
if (source.indexer === undefined || source.indexer.key !== target.indexer.key) {
return [
Related.false,
[
createDiagnostic({
code: "missing-index",
format: {
indexType: getTypeName(target.indexer.key),
sourceType: getTypeName(source),
},
target: diagnosticTarget,
}),
],
];
}
return isTypeAssignableToInternal(
source.indexer.value!,
target.indexer.value,
diagnosticTarget,
relationCache
);
}

function isTupleAssignableToTuple(
source: Tuple,
target: Tuple,
Expand Down
13 changes: 13 additions & 0 deletions packages/compiler/test/checker/relation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,19 @@ describe("compiler: checker: type relations", () => {
});
});

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;
...Record<string>;
}
`,
});
});

it("emit diagnostic assigning other type", async () => {
await expectTypeNotAssignable(
{ source: `string`, target: "Record<string>" },
Expand Down

0 comments on commit f30120d

Please sign in to comment.