mirror of
https://github.com/apple/pkl.git
synced 2026-04-20 07:21:32 +02: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) {
|
String evaluateOutputText(VmTyped fileOutput) {
|
||||||
return doEvaluate(() -> VmUtils.readTextProperty(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
|
// escape analysis should remove this allocation in compiled code
|
||||||
var typeMismatches = new VmTypeMismatchException[elementTypeNodes.length];
|
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.
|
// 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>`
|
// (we can't know that `new Listing { 0; "hi" }[0]` fails for `Listing<Int>|Listing<String>`
|
||||||
// without checking both index 0 and index 1).
|
// 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++) {
|
for (var i = 0; i < elementTypeNodes.length; i++) {
|
||||||
var elementTypeNode = elementTypeNodes[i];
|
var elementTypeNode = elementTypeNodes[i];
|
||||||
try {
|
try {
|
||||||
if (shouldEagerCheck) {
|
var result =
|
||||||
return elementTypeNode.executeEagerly(frame, value);
|
shouldEagerCheck
|
||||||
} else {
|
? elementTypeNode.executeEagerly(frame, value)
|
||||||
return elementTypeNode.executeLazily(frame, value);
|
: elementTypeNode.executeLazily(frame, value);
|
||||||
}
|
localContext.setInTypeTest(wasInTypeTest);
|
||||||
|
return result;
|
||||||
} catch (VmTypeMismatchException e) {
|
} catch (VmTypeMismatchException e) {
|
||||||
typeMismatches[i] = 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);
|
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
|
// escape analysis should remove this allocation in compiled code
|
||||||
var typeMismatches = new VmTypeMismatchException[elementTypeNodes.length];
|
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++) {
|
for (var i = 0; i < elementTypeNodes.length; i++) {
|
||||||
// eager checks
|
// eager checks
|
||||||
try {
|
try {
|
||||||
return elementTypeNodes[i].executeEagerly(frame, value);
|
var result = elementTypeNodes[i].executeEagerly(frame, value);
|
||||||
|
localContext.setInTypeTest(wasInTypeTest);
|
||||||
|
return result;
|
||||||
} catch (VmTypeMismatchException e) {
|
} catch (VmTypeMismatchException e) {
|
||||||
typeMismatches[i] = 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);
|
throw new VmTypeMismatchException.Union(sourceSection, value, this, typeMismatches);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -28,6 +28,8 @@ public class VmLocalContext {
|
|||||||
*/
|
*/
|
||||||
private int activeTrackerDepth = 0;
|
private int activeTrackerDepth = 0;
|
||||||
|
|
||||||
|
private boolean instrumentationEverUsed = false;
|
||||||
|
|
||||||
public VmLocalContext() {}
|
public VmLocalContext() {}
|
||||||
|
|
||||||
public void shouldEagerTypecheck(boolean shouldEagerTypecheck) {
|
public void shouldEagerTypecheck(boolean shouldEagerTypecheck) {
|
||||||
@@ -48,6 +50,7 @@ public class VmLocalContext {
|
|||||||
|
|
||||||
public void enterTracker() {
|
public void enterTracker() {
|
||||||
activeTrackerDepth++;
|
activeTrackerDepth++;
|
||||||
|
instrumentationEverUsed = true;
|
||||||
}
|
}
|
||||||
|
|
||||||
public void exitTracker() {
|
public void exitTracker() {
|
||||||
@@ -57,4 +60,8 @@ public class VmLocalContext {
|
|||||||
public boolean hasActiveTracker() {
|
public boolean hasActiveTracker() {
|
||||||
return activeTrackerDepth > 0;
|
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) {
|
private fun checkModule(module: PModule) {
|
||||||
assertThat(module.properties.size).isEqualTo(2)
|
assertThat(module.properties.size).isEqualTo(2)
|
||||||
assertThat(module.getProperty("name")).isEqualTo("pigeon")
|
assertThat(module.getProperty("name")).isEqualTo("pigeon")
|
||||||
|
|||||||
Reference in New Issue
Block a user