mirror of
https://github.com/apple/pkl.git
synced 2026-03-26 11:01:14 +01:00
Do not activate power assertions when a single union member containing a type constraint fails (#1462)
Prior to this change, this code would activate powers assertions /
instrumentation permanently:
```pkl
foo: String(contains("a")) | String(contains("b")) = "boo"
```
This is because the `contains("a")` constraint would fail, triggering
power assertions, but the subsequent check of the union's
`contains("b")` branch would succeed.
As observed in #1419, once instrumentation is enabled, all subsequent
evaluation slows significantly.
As with #1419, the fix here is to disable power assertions via
`VmLocalContext` until we know that all union members failed type
checking; then, each member is re-executed with power assertions allowed
to provide the improved user-facing error.
This commit is contained in:
@@ -336,6 +336,16 @@ public final class EvaluatorImpl implements Evaluator {
|
||||
}
|
||||
}
|
||||
|
||||
// for use in tests to determine whether an evaluator ever triggered instrumentation
|
||||
boolean isInstrumentationEverUsed() {
|
||||
polyglotContext.enter();
|
||||
try {
|
||||
return VmLanguage.get(null).localContext.get().isInstrumentationEverUsed();
|
||||
} finally {
|
||||
polyglotContext.leave();
|
||||
}
|
||||
}
|
||||
|
||||
String evaluateOutputText(VmTyped fileOutput) {
|
||||
return doEvaluate(() -> VmUtils.readTextProperty(fileOutput));
|
||||
}
|
||||
|
||||
@@ -985,6 +985,11 @@ public abstract class TypeNode extends PklNode {
|
||||
// escape analysis should remove this allocation in compiled code
|
||||
var typeMismatches = new VmTypeMismatchException[elementTypeNodes.length];
|
||||
|
||||
// disallow power assertions from triggering in case one union member checks successfully
|
||||
var localContext = VmLanguage.get(this).localContext.get();
|
||||
var wasInTypeTest = localContext.isInTypeTest();
|
||||
localContext.setInTypeTest(true);
|
||||
|
||||
// Do eager checks (shallow-force) if there are two listings or two mappings represented.
|
||||
// (we can't know that `new Listing { 0; "hi" }[0]` fails for `Listing<Int>|Listing<String>`
|
||||
// without checking both index 0 and index 1).
|
||||
@@ -992,15 +997,36 @@ public abstract class TypeNode extends PklNode {
|
||||
for (var i = 0; i < elementTypeNodes.length; i++) {
|
||||
var elementTypeNode = elementTypeNodes[i];
|
||||
try {
|
||||
if (shouldEagerCheck) {
|
||||
return elementTypeNode.executeEagerly(frame, value);
|
||||
} else {
|
||||
return elementTypeNode.executeLazily(frame, value);
|
||||
}
|
||||
var result =
|
||||
shouldEagerCheck
|
||||
? elementTypeNode.executeEagerly(frame, value)
|
||||
: elementTypeNode.executeLazily(frame, value);
|
||||
localContext.setInTypeTest(wasInTypeTest);
|
||||
return result;
|
||||
} catch (VmTypeMismatchException e) {
|
||||
typeMismatches[i] = e;
|
||||
}
|
||||
}
|
||||
|
||||
// all members failed to type check
|
||||
// if enabled, re-execute type checks to generate power assertions
|
||||
localContext.setInTypeTest(wasInTypeTest);
|
||||
if (VmContext.get(this).getPowerAssertionsEnabled()
|
||||
&& (!wasInTypeTest || localContext.hasActiveTracker())) {
|
||||
for (var i = 0; i < elementTypeNodes.length; i++) {
|
||||
var elementTypeNode = elementTypeNodes[i];
|
||||
try {
|
||||
if (shouldEagerCheck) {
|
||||
elementTypeNode.executeEagerly(frame, value);
|
||||
} else {
|
||||
elementTypeNode.executeLazily(frame, value);
|
||||
}
|
||||
} catch (VmTypeMismatchException e) {
|
||||
typeMismatches[i] = e;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
throw new VmTypeMismatchException.Union(sourceSection, value, this, typeMismatches);
|
||||
}
|
||||
|
||||
@@ -1011,14 +1037,36 @@ public abstract class TypeNode extends PklNode {
|
||||
// escape analysis should remove this allocation in compiled code
|
||||
var typeMismatches = new VmTypeMismatchException[elementTypeNodes.length];
|
||||
|
||||
// disallow power assertions from triggering in case one union member checks successfully
|
||||
var localContext = VmLanguage.get(this).localContext.get();
|
||||
var wasInTypeTest = localContext.isInTypeTest();
|
||||
localContext.setInTypeTest(true);
|
||||
|
||||
for (var i = 0; i < elementTypeNodes.length; i++) {
|
||||
// eager checks
|
||||
try {
|
||||
return elementTypeNodes[i].executeEagerly(frame, value);
|
||||
var result = elementTypeNodes[i].executeEagerly(frame, value);
|
||||
localContext.setInTypeTest(wasInTypeTest);
|
||||
return result;
|
||||
} catch (VmTypeMismatchException e) {
|
||||
typeMismatches[i] = e;
|
||||
}
|
||||
}
|
||||
|
||||
// all members failed to type check
|
||||
// if enabled, re-execute type checks to generate power assertions
|
||||
localContext.setInTypeTest(wasInTypeTest);
|
||||
if (VmContext.get(this).getPowerAssertionsEnabled()
|
||||
&& (!wasInTypeTest || localContext.hasActiveTracker())) {
|
||||
for (var i = 0; i < elementTypeNodes.length; i++) {
|
||||
try {
|
||||
elementTypeNodes[i].executeEagerly(frame, value);
|
||||
} catch (VmTypeMismatchException e) {
|
||||
typeMismatches[i] = e;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
throw new VmTypeMismatchException.Union(sourceSection, value, this, typeMismatches);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -28,6 +28,8 @@ public class VmLocalContext {
|
||||
*/
|
||||
private int activeTrackerDepth = 0;
|
||||
|
||||
private boolean instrumentationEverUsed = false;
|
||||
|
||||
public VmLocalContext() {}
|
||||
|
||||
public void shouldEagerTypecheck(boolean shouldEagerTypecheck) {
|
||||
@@ -48,6 +50,7 @@ public class VmLocalContext {
|
||||
|
||||
public void enterTracker() {
|
||||
activeTrackerDepth++;
|
||||
instrumentationEverUsed = true;
|
||||
}
|
||||
|
||||
public void exitTracker() {
|
||||
@@ -57,4 +60,8 @@ public class VmLocalContext {
|
||||
public boolean hasActiveTracker() {
|
||||
return activeTrackerDepth > 0;
|
||||
}
|
||||
|
||||
public boolean isInstrumentationEverUsed() {
|
||||
return instrumentationEverUsed;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -566,6 +566,69 @@ class EvaluatorTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `constraint failures activate instrumentation`() {
|
||||
val evaluator =
|
||||
with(EvaluatorBuilder.preconfigured()) {
|
||||
powerAssertionsEnabled = true
|
||||
build()
|
||||
}
|
||||
|
||||
val exc =
|
||||
assertThrows<PklException> {
|
||||
evaluator.evaluate(
|
||||
text(
|
||||
"""
|
||||
foo: String(chars.first == "a") = "boo"
|
||||
"""
|
||||
.trimIndent()
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
assertThat((evaluator as EvaluatorImpl).isInstrumentationEverUsed()).isTrue
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `union single-member constraint failures do not activate instrumentation`() {
|
||||
val evaluator =
|
||||
with(EvaluatorBuilder.preconfigured()) {
|
||||
powerAssertionsEnabled = true
|
||||
build()
|
||||
}
|
||||
|
||||
evaluator.evaluate(
|
||||
text(
|
||||
"""
|
||||
foo: String(startsWith("a")) | String(startsWith("b")) | String(startsWith("c")) = "cool"
|
||||
"""
|
||||
.trimIndent()
|
||||
)
|
||||
)
|
||||
|
||||
assertThat((evaluator as EvaluatorImpl).isInstrumentationEverUsed()).isFalse
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `type test failures do not activate instrumentation`() {
|
||||
val evaluator =
|
||||
with(EvaluatorBuilder.preconfigured()) {
|
||||
powerAssertionsEnabled = true
|
||||
build()
|
||||
}
|
||||
|
||||
evaluator.evaluate(
|
||||
text(
|
||||
"""
|
||||
foo = "bar" is Int(this > 0)
|
||||
"""
|
||||
.trimIndent()
|
||||
)
|
||||
)
|
||||
|
||||
assertThat((evaluator as EvaluatorImpl).isInstrumentationEverUsed()).isFalse
|
||||
}
|
||||
|
||||
private fun checkModule(module: PModule) {
|
||||
assertThat(module.properties.size).isEqualTo(2)
|
||||
assertThat(module.getProperty("name")).isEqualTo("pigeon")
|
||||
|
||||
Reference in New Issue
Block a user