Skip to content

Commit e0ed663

Browse files
jwunderlhumanappCopilotthomasjball
authored
correct default values for dynamic dispatch (microsoft#11597)
* correct default values for dynamic dispatch * avoid runtime cost for undefined check Co-authored-by: Eric Anderson <2167515+humanapp@users.noreply.github.com> * Fix thumb asm regex checks for inline default-arg guard Co-authored-by: jwunderl <5615930+jwunderl@users.noreply.github.com> * Fix merge conflict artifact in asmchecks.ts (missing closing braces) Co-authored-by: jwunderl <5615930+jwunderl@users.noreply.github.com> * Fix additional merge conflict artifact in tests/hw-ab/gen-project.js Co-authored-by: jwunderl <5615930+jwunderl@users.noreply.github.com> * merge master in, move defaultinitializers to 59 --------- Co-authored-by: Eric Anderson <2167515+humanapp@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: jwunderl <5615930+jwunderl@users.noreply.github.com> Co-authored-by: Thomas Ball <thomasjball@users.noreply.github.com>
1 parent bf24ac0 commit e0ed663

9 files changed

Lines changed: 328 additions & 61 deletions

File tree

‎docs/language.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ out to your editor soon!
3838

3939
* variable declarations with `let`, `const`
4040
* functions with lexical scoping and recursion
41+
* default parameter initializers, evaluated in the called TypeScript function
42+
when an argument is omitted or `undefined`, including on virtual/interface calls
4143
* top-level code in the file; hello world really is `console.log("Hello world")`
4244
* `if ... else if ... else` statements
4345
* `while` and `do ... while` loops
@@ -283,8 +285,6 @@ There are following differences currently, which should be fixed soon.
283285
They are mostly missing bridges between static, nominally typed classes,
284286
and dynamic maps.
285287

286-
* default parameters are resolved at call site; they should be resolved in the
287-
called method so eg. virtual methods can have different defaults
288288
* `x.foo`, where `x` is class and `foo` is method cannot be currently used as a value;
289289
we could make it equivalent to JavaScript's `x.foo.bind(x)`
290290
* `Object.keys()` is currently not implemented for classes; when it will be

‎pxtcompiler/emitter/emitter.ts‎

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2365,10 +2365,14 @@ ${lbl}: .short 0xffff
23652365
}
23662366
args.push(irToNode(expr))
23672367
} else {
2368-
if (!opts.unfetteredInitializers && !isNumericLiteral(prm.initializer)) {
2369-
userError(9212, lf("only numbers, null, true and false supported as default arguments"))
2368+
// Emitted functions apply their own defaults, including on
2369+
// virtual/interface calls where the implementation may differ.
2370+
// Shims and helpers may have no emitted parameter prologue.
2371+
const needsCallerDefault = attrs.helper || attrs.shim;
2372+
if (needsCallerDefault && !opts.unfetteredInitializers && !isNumericLiteral(prm.initializer)) {
2373+
userError(9212, lf("only numbers, null, true and false supported as default arguments for shims and helpers"))
23702374
}
2371-
args.push(prm.initializer)
2375+
args.push(needsCallerDefault ? prm.initializer : irToNode(emitLit(undefined)))
23722376
}
23732377
} else {
23742378
userError(9213, lf("unsupported default argument (shouldn't happen)"))
@@ -3156,13 +3160,9 @@ ${lbl}: .short 0xffff
31563160
}
31573161
}
31583162

3159-
const destructuredParameters: ParameterDeclaration[] = []
31603163
const fieldAssignmentParameters: ParameterDeclaration[] = []
31613164

31623165
proc.args = getParameters(node).map((p, i) => {
3163-
if (p.name.kind === SK.ObjectBindingPattern) {
3164-
destructuredParameters.push(p)
3165-
}
31663166
if (node.kind == SK.Constructor && isCtorField(p)) {
31673167
fieldAssignmentParameters.push(p)
31683168
}
@@ -3182,7 +3182,33 @@ ${lbl}: .short 0xffff
31823182
}
31833183
})
31843184

3185-
destructuredParameters.forEach(dp => emitVariableDeclaration(dp))
3185+
// Arity wrappers pad omitted arguments with undefined. Keep defaults in
3186+
// the shared body so direct, virtual, interface and action calls all run
3187+
// them, including native entries which can skip the arity wrapper.
3188+
const attrs = parseComments(node);
3189+
proc.args.forEach(l => {
3190+
const parameter = l.def as ParameterDeclaration;
3191+
// Simulator shim dummies must match the native caller-side behavior.
3192+
if (parameter.initializer && !attrs.shim && !attrs.helper) {
3193+
const supplied = proc.mkLabel("defaultarg");
3194+
if (isThumb()) {
3195+
// Tagged undefined is 0, so the guard is a bare
3196+
// compare against zero rather than a runtime call.
3197+
proc.emitJmp(supplied, l.load(), ir.JmpMode.IfNotZero);
3198+
} else {
3199+
// The simulator holds real JS values; a raw zero test
3200+
// would treat a supplied 0/false/null as omitted.
3201+
proc.emitJmpZ(supplied, ir.rtcall("pxt::eqq_bool", [l.load(), emitLit(undefined)]));
3202+
}
3203+
proc.emitExpr(l.storeByRef(emitEscapedExpression(parameter.initializer, "initializer")));
3204+
proc.emitLbl(supplied);
3205+
proc.stackEmpty();
3206+
}
3207+
if (parameter.name.kind === SK.ObjectBindingPattern) {
3208+
// Destructure the resolved argument, not the initializer again.
3209+
emitVarOrParam(parameter, l.load(), typeOf(parameter));
3210+
}
3211+
})
31863212

31873213
// for constructor(public foo:number) generate this.foo = foo;
31883214
for (let p of fieldAssignmentParameters) {

‎tests/compile-test/lang-test0/56ifacedispatch.ts‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -117,12 +117,8 @@ namespace IfaceDispatch {
117117

118118
// --- optional and defaulted parameters through an interface ----------
119119
//
120-
// The emitter fills in a defaulted argument at the call site from the
121-
// statically known signature. Through an interface- or any-typed
122-
// reference there is no such signature, so an omitted argument arrives as
123-
// undefined and the callee's own default does not apply. What must hold
124-
// is that the interface and any paths agree with each other, and that a
125-
// callee written to test for undefined works on every path.
120+
// Defaults are applied in the runtime callee, so omitted arguments work
121+
// through concrete, interface and any-typed references alike (#11562).
126122

127123
interface QzOptIface {
128124
qzOpt(a: number, b?: number): number
@@ -146,16 +142,16 @@ namespace IfaceDispatch {
146142
const iface: QzOptIface = impl
147143
const dyn: any = impl
148144

149-
// a concrete call gets the default filled in at the call site
145+
// a concrete call still applies the default
150146
assert(impl.qzOpt(1) == 6, "opt:concrete")
151147

152148
// explicit arguments behave the same on every path
153149
assert(impl.qzOpt(1, 2) == 3, "opt:concrete2")
154150
assert(iface.qzOpt(1, 2) == 3, "opt:iface2")
155151
assert(dyn.qzOpt(1, 2) == 3, "opt:any2")
156152

157-
// omitting the argument yields the same result on both dynamic paths
158-
assert(("" + iface.qzOpt(1)) == ("" + dyn.qzOpt(1)), "opt:dynagree")
153+
assert(iface.qzOpt(1) == 6, "opt:iface-default")
154+
assert(dyn.qzOpt(1) == 6, "opt:any-default")
159155

160156
// a callee that defaults explicitly works everywhere
161157
assert(impl.qzOptSafe(1) == 6, "opt:safe1")
Lines changed: 107 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,6 @@
1-
// Reproduces a known defect: default parameter values are not applied when a
2-
// method is invoked through dynamic (interface or any-typed) dispatch.
3-
//
4-
// Mechanism: a statically resolved call site fills omitted arguments with the
5-
// declared defaults, because the compiler can see the declaration. A dynamic
6-
// call site pushes only the arguments written at the call, and the arity
7-
// wrapper on the callee fills the missing slots with `undefined` -- the
8-
// parameter initializer (`b = 5` below) never runs. The method then computes
9-
// 1 + undefined = NaN.
10-
//
11-
// The assertions that fail today are commented out (block marked REPRO) so
12-
// the suite stays green. To reproduce: uncomment that block and run
13-
// `gulp testlang` -- qzdp:iface and qzdp:any fail with the calls yielding
14-
// NaN instead of 6. A fix for the defect makes the REPRO block pass; it can
15-
// then be uncommented permanently and the qzdp:agree assertion below retired.
16-
//
17-
// 56ifacedispatch.ts documents the same constraint where it affects the
18-
// dispatch corpus; this file is the minimal standalone repro.
1+
// Regression for #11562. Default initializers belong to the runtime callee,
2+
// not the statically known call signature. Both omitted and explicit undefined
3+
// arguments trigger defaults; other falsy values must survive unchanged.
194

205
interface DfShape {
216
dfOpt(a: number, b?: number): number;
@@ -35,23 +20,118 @@ function testDefaultParamDispatch() {
3520
const viaIface: DfShape = direct;
3621
const viaAny: any = direct;
3722

38-
// Statically resolved call: the compiler applies the default. Passes.
23+
// Statically resolved calls still apply the default.
3924
assert(direct.dfOpt(1) == 6, "qzdp:direct");
4025

4126
// Passing the argument explicitly works on every path.
4227
assert(direct.dfOpt(1, 2) == 3, "qzdp:direct2");
4328
assert(viaIface.dfOpt(1, 2) == 3, "qzdp:iface2");
4429
assert(viaAny.dfOpt(1, 2) == 3, "qzdp:any2");
4530

46-
// REPRO -- uncomment these two lines to surface the defect:
47-
// assert(viaIface.dfOpt(1) == 6, "qzdp:iface");
48-
// assert(viaAny.dfOpt(1) == 6, "qzdp:any");
31+
// Regression for #11562: dynamic calls must apply the implementation's default.
32+
assert(viaIface.dfOpt(1) == 6, "qzdp:iface");
33+
assert(viaAny.dfOpt(1) == 6, "qzdp:any");
4934

50-
// Kept active so any CHANGE in the current behavior is noticed: the two
51-
// dynamic paths must at least agree with each other. Compared as strings
52-
// so this holds both before a fix (NaN == NaN fails as numbers) and
53-
// after one.
54-
assert("" + viaIface.dfOpt(1) == "" + viaAny.dfOpt(1), "qzdp:agree");
35+
assert(direct.dfOpt(1, undefined) === 6, "qzdp:direct-undefined");
36+
assert(viaIface.dfOpt(1, undefined) === 6, "qzdp:iface-undefined");
37+
assert(viaAny.dfOpt(1, undefined) === 6, "qzdp:any-undefined");
38+
assert(viaIface.dfOpt(1, 0) === 1, "qzdp:zero");
39+
assert(viaIface.dfOpt(1, null) === 1, "qzdp:null");
40+
}
41+
42+
namespace DefaultDispatch {
43+
class Base implements DfShape {
44+
dfOpt(a: number, b = 5): number { return a + b; }
45+
}
46+
class Derived extends Base {
47+
dfOpt(a: number, b = 9): number { return a + b; }
48+
fromSuper(): number { return super.dfOpt(1); }
49+
}
50+
class ParameterProperty {
51+
constructor(public value = 13) { }
52+
}
53+
class DerivedConstructor extends ParameterProperty {
54+
constructor(value = 17) { super(value); }
55+
}
56+
interface OptionalValues {
57+
value(n?: any): any;
58+
pair(a?: number, b?: number): number;
59+
capture(n?: number): () => number;
60+
}
61+
class Values implements OptionalValues {
62+
value(n: any = 11): any { return n; }
63+
pair(a = 2, b = 3): number { return a * 10 + b; }
64+
capture(n = 7): () => number { return () => ++n; }
65+
}
66+
function free(n = 19): number { return n; }
67+
function bool(n = true): boolean { return n; }
68+
function ref(n: number[] = null): number[] { return n; }
69+
function missing(n?: number): number { return n; }
70+
function immutableCapture(n = 29): () => number { return () => n; }
71+
72+
// Native calls use TD_ID; simjs runs the TypeScript dummy body.
73+
//% shim=TD_ID
74+
function shimDefault(n = 31): number { return n; }
75+
//% shim=TD_ID
76+
//% n.defl=37 explicitDefaults="n"
77+
function annotatedDefault(n?: number): number { return n; }
78+
79+
export function run() {
80+
const derived = new Derived();
81+
const base: Base = derived;
82+
const iface: DfShape = derived;
83+
const dynamic: any = derived;
84+
assert(derived.dfOpt(1) === 10, "qzdp:derived");
85+
assert(base.dfOpt(1) === 10, "qzdp:virtual-default");
86+
assert(iface.dfOpt(1) === 10, "qzdp:override-iface");
87+
assert(dynamic.dfOpt(1) === 10, "qzdp:override-any");
88+
assert(base.dfOpt(1, undefined) === 10, "qzdp:virtual-undefined");
89+
assert(base.dfOpt(1, 2) === 3, "qzdp:virtual-explicit");
90+
assert(derived.fromSuper() === 6, "qzdp:super");
91+
92+
const action: (n?: number) => number = free;
93+
const dynamicAction: any = free;
94+
const arrow: (n?: number) => number = (n = 23) => n;
95+
assert(free() === 19 && free(undefined) === 19, "qzdp:free");
96+
assert(action() === 19 && action(undefined) === 19, "qzdp:action");
97+
assert(dynamicAction() === 19, "qzdp:any-action");
98+
assert(arrow() === 23 && arrow(0) === 0, "qzdp:arrow");
99+
const literal: DfShape = { dfOpt: (a: number, b = 41) => a + b };
100+
assert(literal.dfOpt(1) === 42, "qzdp:literal-action");
101+
102+
const values: OptionalValues = new Values();
103+
const anyValues: any = values;
104+
assert(values.value() === 11, "qzdp:value-default");
105+
assert(values.value(undefined) === 11, "qzdp:value-undefined");
106+
assert(values.value(null) === null, "qzdp:preserve-null");
107+
assert(values.value(0) === 0, "qzdp:preserve-zero");
108+
assert(values.value(false) === false, "qzdp:preserve-false");
109+
assert(values.value("") === "", "qzdp:preserve-empty-string");
110+
const nan = values.value(0 / 0);
111+
assert(nan !== nan, "qzdp:preserve-nan");
112+
assert(values.pair() === 23, "qzdp:multiple");
113+
assert(anyValues.pair() === 23, "qzdp:arity-padding");
114+
assert(values.pair(undefined, 8) === 28, "qzdp:argument-hole");
115+
assert(values.pair(4) === 43, "qzdp:trailing");
116+
const counter = values.capture();
117+
const explicitCounter = values.capture(10);
118+
assert(counter() === 8 && counter() === 9, "qzdp:boxed-param");
119+
assert(explicitCounter() === 11, "qzdp:boxed-explicit");
120+
assert(immutableCapture()() === 29, "qzdp:immutable-capture");
121+
122+
assert(new ParameterProperty().value === 13, "qzdp:constructor-property");
123+
assert(new ParameterProperty(undefined).value === 13, "qzdp:constructor-undefined");
124+
assert(new DerivedConstructor().value === 17, "qzdp:derived-constructor");
125+
assert(new DerivedConstructor(0).value === 0, "qzdp:constructor-zero");
126+
assert(bool() && !bool(false), "qzdp:boolean");
127+
const array = [1, 2];
128+
assert(ref() === null && ref(array) === array, "qzdp:reference");
129+
assert(missing() === undefined, "qzdp:no-default");
130+
assert(shimDefault() === 31 && shimDefault(0) === 0, "qzdp:shim-default");
131+
assert(shimDefault(undefined) === undefined, "qzdp:shim-undefined-unchanged");
132+
assert(annotatedDefault() === 37, "qzdp:annotated-default");
133+
}
55134
}
56135

57136
testDefaultParamDispatch();
137+
DefaultDispatch.run();
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
// Defaults run once, in the callee's lexical scope and parameter order, after
2+
// every explicit call argument is evaluated. No unfetteredInitializers needed.
3+
namespace DefaultInitializers {
4+
let trace = "";
5+
let calls = 0;
6+
function next(): number { trace += "d"; return ++calls; }
7+
function supplied(): number { trace += "a"; return 10; }
8+
function absent(): number { calls++; return undefined; }
9+
10+
interface Factory {
11+
make(n?: number, other?: number): number;
12+
empty(n?: number): number;
13+
array(value?: number[]): number[];
14+
text(value?: string): string;
15+
}
16+
class Maker implements Factory {
17+
offset = 100;
18+
constructor() { }
19+
make(n = next(), other = n + this.offset): number { return n + other; }
20+
empty(n = absent()): number { return n; }
21+
array(value = [next()]): number[] { return value; }
22+
text(value = "hello"): string { return value; }
23+
}
24+
function destructured({ x } = { x: next() }, n = x + 1): number { return x + n; }
25+
function captured(n = next()): () => number { return () => ++n; }
26+
function defaults(text = "world", negative = -1, missing: number = undefined): string {
27+
return text + ":" + negative + ":" + missing;
28+
}
29+
function scope() {
30+
let outer = 40;
31+
const f: (n?: number) => number = (n = ++outer) => n;
32+
assert(f() === 41 && f(undefined) === 42, "qzdpi:lexical");
33+
assert(f(0) === 0 && outer === 42, "qzdpi:lexical-explicit");
34+
}
35+
export function run() {
36+
const direct = new Maker();
37+
const iface: Factory = direct;
38+
const dynamic: any = direct;
39+
assert(iface.make() === 102 && calls === 1, "qzdpi:iface-order-this");
40+
assert(dynamic.make() === 104 && calls === 2, "qzdpi:any-order-this");
41+
trace = "";
42+
assert(direct.make(undefined, supplied()) === 13, "qzdpi:direct");
43+
assert(trace === "ad" && calls === 3, "qzdpi:argument-evaluation-order");
44+
assert(iface.make(7, 8) === 15 && calls === 3, "qzdpi:skip-supplied");
45+
assert(direct.empty() === undefined && calls === 4, "qzdpi:undefined-default-once");
46+
assert(dynamic.empty() === undefined && calls === 5, "qzdpi:any-undefined-once");
47+
const a = iface.array();
48+
const b = iface.array();
49+
a[0] = 99;
50+
assert(a !== b && b[0] === 7, "qzdpi:fresh-reference");
51+
assert(iface.array(a) === a && calls === 7, "qzdpi:reference-supplied");
52+
assert(destructured() === 17 && calls === 8, "qzdpi:destructured-once");
53+
assert(destructured({ x: 20 }) === 41 && calls === 8, "qzdpi:destructured-explicit");
54+
const action: (n?: number) => () => number = captured;
55+
const counter = action();
56+
assert(counter() === 10 && counter() === 11 && calls === 9, "qzdpi:boxed-default");
57+
assert(iface.text() === "hello" && dynamic.text(undefined) === "hello", "qzdpi:string-default");
58+
assert(direct.text("") === "", "qzdpi:string-explicit");
59+
assert(defaults() === "world:-1:undefined", "qzdpi:other-initializers");
60+
scope();
61+
}
62+
}
63+
DefaultInitializers.run();

0 commit comments

Comments
 (0)