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

fix: argument validation of parameters with defaults, Map and Set #7435

Merged
merged 2 commits into from
Nov 22, 2024
Merged
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
79 changes: 67 additions & 12 deletions packages/transformers/src/validateArgs/transformProgram.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ export default function transformProgram(
return {
name: p.getName(),
isRest: p.isRestParameter(),
hasInitializer: p.hasInitializer(),
type: p.getType(),
typeName: p.getTypeNode()?.getText(),
};
Expand Down Expand Up @@ -205,7 +206,7 @@ export default function transformProgram(

newSourceText += `

export function validateArgs_${className}_${methodName}(options: any = {}) {
export function validateArgs_${className}_${methodName}() {
return <T extends Function>(__decoratedMethod: T, { kind }: ClassMethodDecoratorContext): T | void => {
if (kind === "method") {
return function ${methodName}(this: any, ${paramSpreadWithUnknown}) {
Expand Down Expand Up @@ -273,6 +274,7 @@ function getTypeName(t: Type<tsm.Type>): string {
interface ParameterInfo {
name: string;
isRest?: boolean;
hasInitializer?: boolean;
type: Type<tsm.Type>;
typeName: string | undefined;
}
Expand Down Expand Up @@ -310,6 +312,24 @@ Type ${param.typeName} recursively references itself`,
}

const ctx = `{ kind: "${kind}", name: "${param.name}" }`;

// Parameters with a default value, but no `| undefined` in the type need to be treated as optional
if (
param.hasInitializer
&& !(
param.type.isUnion()
&& param.type.getUnionTypes().some((t) => t.isUndefined())
)
) {
return `v.optional(${ctx}, ${
getValidationFunction(
context,
{ ...param, hasInitializer: false },
kind,
)
})`;
}

if (
param.type.isNumberLiteral()
|| param.type.isStringLiteral()
Expand Down Expand Up @@ -554,23 +574,58 @@ Unable to find import specifier for class ${param.typeName}.`,
return `v.class(${ctx}, "${declaredName}", ${param.typeName})`;
}

if (isInterface) {
const variableDeclaration = valueDeclaration?.asKind(
tsm.SyntaxKind.VariableDeclaration,
);
const isAmbient = !!variableDeclaration
&& !!(variableDeclaration?.getCombinedModifierFlags()
& tsm.ModifierFlags.Ambient);
const variableDeclaration = valueDeclaration?.asKind(
tsm.SyntaxKind.VariableDeclaration,
);
const isAmbient = !!variableDeclaration
&& !!(variableDeclaration?.getCombinedModifierFlags()
& tsm.ModifierFlags.Ambient);

if (isAmbient) {
if (isInterface) {
if (symbolName === "Date") {
return `v.date(${ctx})`;
}
if (symbolName === "Uint8Array") {
return `v.uint8array(${ctx})`;
}
}

const structure = variableDeclaration?.getStructure();

if (isAmbient && symbolName === "Date") {
return `v.date(${ctx})`;
if (
structure?.name === "Map"
&& structure.type === "MapConstructor"
) {
return `v.class(${ctx}, "Map", Map)`;
}

if (isAmbient && symbolName === "Uint8Array") {
return `v.uint8array(${ctx})`;
if (
structure?.name === "Set"
&& structure.type === "SetConstructor"
) {
return `v.class(${ctx}, "Set", Set)`;
}
}

// Those are not detected as ambient interfaces for some reason
if (
symbolName === "ReadonlyMap"
&& symbol.getDeclarations().every((d) =>
d.isKind(tsm.SyntaxKind.InterfaceDeclaration)
)
) {
return `v.class(${ctx}, "Map", Map)`;
}
if (
symbolName === "ReadonlySet"
&& symbol.getDeclarations().every((d) =>
d.isKind(tsm.SyntaxKind.InterfaceDeclaration)
)
) {
return `v.class(${ctx}, "Set", Set)`;
}

if (isInterface || isObject) {
const expectedKind = isInterface
? tsm.SyntaxKind.InterfaceDeclaration
Expand Down
41 changes: 41 additions & 0 deletions packages/transformers/test/fixtures/testDefaultValue.mts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/* eslint-disable @typescript-eslint/no-unused-expressions */
import { validateArgs } from "@zwave-js/transformers";
import assert from "node:assert";

class Test {
@validateArgs()
foo(arg1: number = 5): void {
arg1;

Check warning

Code scanning / CodeQL

Expression has no effect Warning test

This expression has no effect.
return void 0;
}

@validateArgs()
bar(arg1: number | string = 5): void {
arg1;

Check warning

Code scanning / CodeQL

Expression has no effect Warning test

This expression has no effect.
return void 0;
}
}

const test = new Test();
// These should not throw
test.foo();
test.foo(1);
test.foo(undefined);

test.bar();
test.bar(1);
test.bar("str");
test.bar(undefined);

// These should throw
assert.throws(
// @ts-expect-error
() => test.foo(true),
/optional parameter arg1 to be a number, got true/,
);

assert.throws(
// @ts-expect-error
() => test.bar(true),
/optional parameter arg1 to be one of (number \| string|string \| number), got true/,
);
79 changes: 79 additions & 0 deletions packages/transformers/test/fixtures/testMapAndSet.mts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/* eslint-disable @typescript-eslint/no-unused-expressions */
import { validateArgs } from "@zwave-js/transformers";
import assert from "node:assert";

class Test {
@validateArgs()
map(arg1: Map<any, any>): void {
arg1;

Check warning

Code scanning / CodeQL

Expression has no effect Warning test

This expression has no effect.
return void 0;
}

@validateArgs()
readonlyMap(arg1: ReadonlyMap<any, any>): void {
arg1;

Check warning

Code scanning / CodeQL

Expression has no effect Warning test

This expression has no effect.
return void 0;
}

@validateArgs()
set(arg1: Set<any>): void {
arg1;

Check warning

Code scanning / CodeQL

Expression has no effect Warning test

This expression has no effect.
return void 0;
}

@validateArgs()
readonlySet(arg1: ReadonlySet<any>): void {
arg1;

Check warning

Code scanning / CodeQL

Expression has no effect Warning test

This expression has no effect.
return void 0;
}
}

const test = new Test();
// These should not throw
test.map(new Map());
test.readonlyMap(new Map()); // readonly does not exist at runtime
test.set(new Set());
test.readonlySet(new Set()); // readonly does not exist at runtime

// These should throw
assert.throws(
// @ts-expect-error
() => test.map({}),
/arg1 to be an instance of class Map, got object/,
);
assert.throws(
// @ts-expect-error
() => test.readonlyMap({}),
/arg1 to be an instance of class Map, got object/,
);
assert.throws(
// @ts-expect-error
() => test.map("string"),
/arg1 to be an instance of class Map, got string "string"/,
);
assert.throws(
// @ts-expect-error
() => test.readonlyMap("string"),
/arg1 to be an instance of class Map, got string "string"/,
);

assert.throws(
// @ts-expect-error
() => test.set({}),
/arg1 to be an instance of class Set, got object/,
);
assert.throws(
// @ts-expect-error
() => test.readonlySet({}),
/arg1 to be an instance of class Set, got object/,
);
assert.throws(
// @ts-expect-error
() => test.set("string"),
/arg1 to be an instance of class Set, got string "string"/,
);
assert.throws(
// @ts-expect-error
() => test.readonlySet("string"),
/arg1 to be an instance of class Set, got string "string"/,
);
2 changes: 1 addition & 1 deletion packages/transformers/tsconfig.test.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@
"customConditions": ["@@dev", "@@test_transformers"]
},
"include": ["test/fixtures/**/*.mts"]
// "include": ["test/fixtures/testCrossPackageImports.mts"]
// "include": ["test/fixtures/testMapAndSet.mts"]
}
Loading