Skip to content

Commit 5715861

Browse files
christopherswensonchristopherswenson
committed
Add default parameter values
1 parent 50f27a8 commit 5715861

File tree

11 files changed

+189
-36
lines changed

11 files changed

+189
-36
lines changed

‎packages/malloy/src/lang/ast/expressions/constant-sub-expression.ts renamed to ‎packages/malloy/src/lang/ast/expressions/constant-expression.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ export class ConstantFieldSpace implements FieldSpace {
4747
}
4848
lookup(_name: unknown): LookupResult {
4949
return {
50-
error: 'Only constants allowed in parameter expressions',
50+
error: 'Only constants allowed in parameter default values',
5151
found: undefined,
5252
};
5353
}
@@ -80,7 +80,7 @@ class DollarReference extends ExpressionDef {
8080
}
8181
}
8282

83-
export class ConstantSubExpression extends ExpressionDef {
83+
export class ConstantExpression extends ExpressionDef {
8484
elementType = 'constantExpression';
8585
private cfs?: ConstantFieldSpace;
8686
constructor(readonly expr: ExpressionDef) {

‎packages/malloy/src/lang/ast/parameters/constant-parameter.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,13 @@
2323

2424
import {isAtomicFieldType, Parameter} from '../../../model/malloy_types';
2525

26-
import {ConstantSubExpression} from '../expressions/constant-sub-expression';
26+
import {ConstantExpression} from '../expressions/constant-expression';
2727
import {HasParameter} from './has-parameter';
2828

2929
export class ConstantParameter extends HasParameter {
3030
constructor(
3131
name: string,
32-
readonly value: ConstantSubExpression
32+
readonly value: ConstantExpression
3333
) {
3434
super({name, isCondition: false});
3535
this.has({value: value});

‎packages/malloy/src/lang/ast/parameters/has-parameter.ts

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,22 @@
2323

2424
import {Parameter, CastType, isCastType} from '../../../model/malloy_types';
2525

26-
import {ConstantSubExpression} from '../expressions/constant-sub-expression';
26+
import {ConstantExpression} from '../expressions/constant-expression';
2727
import {MalloyElement} from '../types/malloy-element';
2828

2929
interface HasInit {
3030
name: string;
3131
isCondition: boolean;
3232
type?: string;
33-
default?: ConstantSubExpression;
33+
default?: ConstantExpression;
3434
}
3535

3636
export class HasParameter extends MalloyElement {
3737
elementType = 'hasParameter';
3838
readonly name: string;
3939
readonly isCondition: boolean;
4040
readonly type?: CastType;
41-
readonly default?: ConstantSubExpression;
41+
readonly default?: ConstantExpression;
4242

4343
constructor(init: HasInit) {
4444
super();
@@ -54,22 +54,35 @@ export class HasParameter extends MalloyElement {
5454
}
5555

5656
parameter(): Parameter {
57-
const name = this.name;
58-
const type = this.type || 'string';
59-
if (this.isCondition) {
60-
const cCond = this.default?.constantCondition(type).value || null;
57+
if (this.default !== undefined) {
58+
const constant = this.default.constantValue();
59+
if (this.type && this.type !== constant.dataType && constant.dataType !== 'error') {
60+
this.default.log(`Default value for parameter does not match declared type \`${this.type}\``);
61+
}
62+
if (!isCastType(constant.dataType) && constant.dataType !== 'error') {
63+
this.default.log(`Default value cannot have type \`${constant.dataType}\``);
64+
return {
65+
value: constant.value,
66+
name: this.name,
67+
type: 'error',
68+
constant: true, // TODO remove condition parameter type
69+
}
70+
}
6171
return {
62-
type,
63-
name,
64-
condition: cCond,
65-
};
72+
value: constant.value,
73+
name: this.name,
74+
type: constant.dataType,
75+
constant: true,
76+
}
77+
}
78+
if (this.type === undefined) {
79+
this.log("Parameter must have default value or declared type");
6680
}
67-
const cVal = this.default?.constantValue().value || null;
6881
return {
69-
value: cVal,
70-
type,
82+
value: null,
7183
name: this.name,
72-
constant: false,
73-
};
84+
type: this.type ?? 'error',
85+
constant: true
86+
}
7487
}
7588
}

‎packages/malloy/src/lang/ast/source-elements/named-source.ts

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ export class NamedSource extends Source {
6666
return this.structDef(parameterSpace);
6767
}
6868
const modelEnt = this.modelEntry(this.ref);
69-
if (modelEnt && !modelEnt.exported) {
69+
// TODO right now we just expand the structRef if it has any parameters; but we should really
70+
// evaluate arguments separately and return them here, to be included in the query;
71+
if (modelEnt && (!modelEnt.exported || modelEnt.entry.type === 'struct' && modelEnt.entry.parameters)) {
7072
// If we are not exporting the referenced structdef, don't
7173
// use the reference
7274
return this.structDef(parameterSpace);
@@ -144,6 +146,7 @@ export class NamedSource extends Source {
144146
parameters[paramName] = {...base.parameters[paramName]};
145147
}
146148

149+
const outArguments = {};
147150
const passedNames = new Set();
148151
for (const argument of this.args ?? []) {
149152
const id =
@@ -163,38 +166,47 @@ export class NamedSource extends Source {
163166
continue;
164167
}
165168
passedNames.add(name);
166-
const decl = parameters[name];
167-
if (!decl) {
169+
const parameter = parameters[name];
170+
if (!parameter) {
168171
id.log(
169172
`\`${this.refName}\` has no declared parameter named \`${id.refString}\``
170173
);
171174
} else {
172-
if (isValueParameter(decl)) {
175+
if (isValueParameter(parameter)) {
173176
const paramSpace = parameterSpace ?? new ParameterSpace(pList ?? []);
174177
const pVal = argument.value.getExpression(paramSpace);
175178
let value = pVal.value;
176-
if (pVal.dataType !== decl.type && isCastType(decl.type)) {
177-
value = castTo(decl.type, pVal.value, pVal.dataType, true);
179+
if (pVal.dataType !== parameter.type && isCastType(parameter.type)) {
180+
value = castTo(parameter.type, pVal.value, pVal.dataType, true);
178181
}
179-
decl.value = value;
182+
outArguments[name] = {
183+
...parameter,
184+
value
185+
};
180186
} else {
181187
throw new Error('UNIMPLEMENTED'); // TODO remove need for this branch?
182188
}
183189
}
184190
}
185-
for (const checkDef in parameters) {
186-
if (!paramHasValue(parameters[checkDef])) {
187-
this.refLog(
188-
`Argument not provided for required parameter \`${checkDef}\``
189-
);
191+
192+
for (const paramName in parameters) {
193+
if (!(paramName in outArguments)) {
194+
if (paramHasValue(parameters[paramName])) {
195+
outArguments[paramName] = parameters[paramName];
196+
} else {
197+
this.refLog(
198+
`Argument not provided for required parameter \`${paramName}\``
199+
);
200+
}
190201
}
191202
}
203+
192204
const outParameters = {};
193205
for (const parameter of pList ?? []) {
194206
const compiled = parameter.parameter();
195207
outParameters[compiled.name] = compiled;
196208
}
197-
const ret = {...base, parameters: outParameters, arguments: parameters};
209+
const ret = {...base, parameters: outParameters, arguments: outArguments};
198210
this.document()?.rememberToAddModelAnnotations(ret);
199211
return ret;
200212
}

‎packages/malloy/src/lang/ast/types/expression-def.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,8 @@ function numeric(
449449
};
450450
}
451451

452+
// TODO make the error location be the operand that is not a number, rather than
453+
// always the first operand
452454
left.log(`Non numeric('${lhs.dataType},${rhs.dataType}') value with '${op}'`);
453455
return errorFor('numbers required');
454456
}

‎packages/malloy/src/lang/grammar/MalloyParser.g4

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ sourceParameters
154154
;
155155

156156
sourceParameter
157-
: parameterNameDef DOUBLECOLON malloyType
157+
: parameterNameDef (DOUBLECOLON malloyType)? (IS fieldExpr)?
158158
;
159159

160160
parameterNameDef: id;

‎packages/malloy/src/lang/malloy-to-ast.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import {
5151
Note,
5252
} from '../model/malloy_types';
5353
import {Tag} from '../tags';
54+
import { ConstantExpression } from './ast/expressions/constant-expression';
5455

5556
class ErrorNode extends ast.SourceQueryElement {
5657
elementType = 'parseErrorSourceQuery';
@@ -355,11 +356,16 @@ export class MalloyToAST
355356
}
356357

357358
getSourceParameter(pcx: parse.SourceParameterContext): ast.HasParameter {
359+
const defaultCx = pcx.fieldExpr();
360+
const defaultValue = defaultCx ? this.astAt(new ConstantExpression(this.getFieldExpr(defaultCx)), defaultCx) : undefined;
361+
const typeCx = pcx.malloyType();
362+
const type = typeCx ? this.getMalloyType(typeCx) : undefined;
358363
return this.astAt(
359364
new ast.HasParameter({
360365
name: getId(pcx.parameterNameDef()),
361366
isCondition: false, // TODO crs remove?
362-
type: this.getMalloyType(pcx.malloyType()),
367+
type,
368+
default: defaultValue
363369
}),
364370
pcx
365371
);

‎packages/malloy/src/lang/test/parameters.spec.ts

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,64 @@ import {markSource} from './test-translator';
99
import './parse-expects';
1010

1111
describe('parameters', () => {
12-
test('can declare parameter', () => {
12+
test('can declare parameter with no default value', () => {
1313
expect(`
1414
##! experimental.parameters
1515
source: ab_new(param::number) is ab
1616
`).toTranslate();
1717
});
18+
test('can declare parameter with default value literal', () => {
19+
expect(`
20+
##! experimental.parameters
21+
source: ab_new(param::number is 7) is ab
22+
`).toTranslate();
23+
});
24+
test('can declare parameter with default value constant', () => {
25+
expect(`
26+
##! experimental.parameters
27+
source: ab_new(param::number is 7 + 7) is ab
28+
`).toTranslate();
29+
});
30+
test('cannot specify default value with incompatible type', () => {
31+
expect(markSource`
32+
##! experimental.parameters
33+
source: ab_new(param::number is ${'"hello"'}) is ab
34+
`).translationToFailWith("Default value for parameter does not match declared type `number`");
35+
});
36+
test('no additional error if default value type is error', () => {
37+
expect(markSource`
38+
##! experimental.parameters
39+
source: ab_new(param::number is 1 + "foo") is ab
40+
`).translationToFailWith("Non numeric('number,string') value with '+'");
41+
});
42+
test('can declare parameter with inferred type', () => {
43+
expect(`
44+
##! experimental.parameters
45+
source: ab_new(param is 7) is ab
46+
`).toTranslate();
47+
});
1848
test('can pass parameter into extended base source', () => {
1949
expect(`
2050
##! experimental.parameters
2151
source: ab_new(param::number) is ab
2252
source: ab_new_new(param::number) is ab_new(param) extend {}
2353
`).toTranslate();
2454
});
55+
test('can pass parameter to override default value with constant', () => {
56+
expect(`
57+
##! experimental.parameters
58+
source: ab_new(param::number is 10) is ab
59+
source: ab_new_new is ab_new(param is 7) extend {}
60+
`).toTranslate();
61+
});
62+
// TODO ensure this passes though
63+
test('can pass parameter to override default value with param value', () => {
64+
expect(`
65+
##! experimental.parameters
66+
source: ab_new(param::number is 10) is ab
67+
source: ab_new_new(param::number is 11) is ab_new(param) extend {}
68+
`).toTranslate();
69+
});
2570
test('can pass parameter into named base source', () => {
2671
expect(`
2772
##! experimental.parameters
@@ -72,6 +117,20 @@ describe('parameters', () => {
72117
run: ab_new(param is 1) -> { select: * }
73118
`).toTranslate();
74119
});
120+
test('can not pass argument for default-valued param', () => {
121+
expect(`
122+
##! experimental.parameters
123+
source: ab_new(param is 1) is ab
124+
run: ab_new -> { select: * }
125+
`).toTranslate();
126+
});
127+
test('can pass zero args for source with default-valued param', () => {
128+
expect(`
129+
##! experimental.parameters
130+
source: ab_new(param is 1) is ab
131+
run: ab_new() -> { select: * }
132+
`).toTranslate();
133+
});
75134
test('can pass non-literal argument for param', () => {
76135
expect(`
77136
##! experimental.parameters
@@ -412,4 +471,12 @@ describe('parameters', () => {
412471
`
413472
).translationToFailWith('`param_3` is not defined');
414473
});
474+
test('error when referencing identifier in default param value', () => {
475+
expect(
476+
markSource`
477+
##! experimental.parameters
478+
source: ab_new_1(param_1 is ${'ident'}) is ab
479+
`
480+
).translationToFailWith('Only constants allowed in parameter default values');
481+
});
415482
});

‎packages/malloy/src/model/malloy_query.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4170,6 +4170,12 @@ class QueryStruct extends QueryNode {
41704170
return this.resolvedParameters;
41714171
}
41724172
this.resolvedParameters = {};
4173+
// First, copy over all parameters, to get default values
4174+
const params = this.fieldDef.parameters ?? {};
4175+
for (const parameterName in params) {
4176+
this.resolvedParameters[parameterName] = params[parameterName];
4177+
}
4178+
// Then, copy over arguments to override default values
41734179
const args = this.fieldDef.arguments ?? {};
41744180
for (const parameterName in args) {
41754181
const orig = args[parameterName];

‎packages/malloy/src/model/malloy_types.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,6 +1004,18 @@ export interface StructDef extends NamedObject, ResultStructMetadata, Filtered {
10041004
modelAnnotation?: ModelAnnotation;
10051005
}
10061006

1007+
/*
1008+
source: foo(x::string) is ....
1009+
// parameters: [{x}]
1010+
1011+
run: foo(x is 1)
1012+
// no parameters
1013+
// arguments: [{x is 1}]
1014+
1015+
1016+
run: foo -> thing
1017+
*/
1018+
10071019
export type ExpressionValueType =
10081020
| AtomicFieldType
10091021
| 'null'

0 commit comments

Comments
 (0)