Overhaul typechecks for Mapping/Listing #628

bioball · 2024-08-15T23:43:47Z

This follows this SPICE: apple/pkl-evolution#11

This changes how the language performs typechecks for mappings and
listings.

Currently, Pkl will shallow-force any Mapping and Listing to check
the type parameter (e.g. Listing<Person> means each element is checked
to be an instance of Person).

This changes the language to check each member's type when the member
is accessed.

This PR is designed to be reviewed as two separate commits.
The first one contains business logic that changes how mapping/listing
typechecks work.
The second one adjusts the test runner to handle errors thrown within
tests.

Closes #405
Closes #406

pkl-core/src/test/files/LanguageSnippetTests/output/errors/listingTypeCheckError1.err

bioball · 2024-08-15T23:57:38Z

pkl-core/src/test/files/LanguageSnippetTests/input/listings/typeCheck.pkl

@@ -1,41 +1,367 @@
 import "pkl:test"


Note: these don't amend pkl:test because facts and examples are declared as Mapping<String, Listing<Boolean>> and Mapping<String, Listing<unknown>>. These are typed properties that themselves use Mapping/Listing type checks, and problems with typechecking can quickly be conflated between the tests themselves and the test framework.

bioball · 2024-08-16T20:42:32Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

   */
-  public abstract void execute(VirtualFrame frame, Object value);
+  public abstract Object execute(VirtualFrame frame, Object value);


This is one of the biggest changes. Executing a typecheck will return a type-casted value.

For most types, this simply returns value if the value conforms to the type (e.g. Int). For Listing and Mapping, this returns a surrogate listing/mapping object. Other parameterized types may also return a new object if the parameterized type is Listing or Mapping (e.g. List<Listing<Int>>).

bioball · 2024-08-16T20:48:53Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

    }
  }

-  public abstract static class ListTypeNode extends ObjectSlotTypeNode {
+  public static final class ListTypeNode extends ObjectSlotTypeNode {


Truffle's DSL was overriding executeEagerly when making this an abstract class and using specializations. I'm not sure if I'm doing this wrong?

But, regardless, having specializations here doesn't seem that useful anyway? Reading the same member value skips the typecheck the second time around because it gets cached. And reading a new value needs an instanceof call and will de-specialize.

This could be related to my lambda remark above.

I don't think so; I'm referring to the generated code that Truffle produces when you have an abstract class with specializations, which is a different concern than partial evaluation?

bioball · 2024-08-16T20:51:16Z

pkl-core/src/main/java/org/pkl/core/ast/type/VmTypeMismatchException.java

+      this.insertedStackFrames = new HashMap<>();
+    }
+    this.insertedStackFrames.put(callTarget, stackFrame);
+  }


Added this so we can allow a VmTypeMismatchException to blame a source section when a typecheck fails (see ListingOrMappingTypeNode#doEagerCheck)

bioball · 2024-08-17T00:00:27Z

pkl-core/src/main/java/org/pkl/core/stdlib/base/MappingNodes.java

+            var alreadyVisited = !visited.add(key);
+            // important to record hidden member as visited before skipping it
+            // because any overriding member won't carry a `hidden` identifier
+            if (alreadyVisited || member.isLocalOrExternalOrHidden()) return true;


This implementation is changed because iterateMemberValues might cause a VmMapping/VmListing to throw, because VmListingOrMapping#getCachedValue() can possibly perform an additional typecheck. Mapping#length should not need to execute any members, so, it should not need to run typechecks.

This changes how the language performs typechecks for mappings and listings. Currently, Pkl will shallow-force any Mapping and Listing to check it the type parameter (e.g. Listing<Person> means each element is checked to be an instance of Person). This changes the language to check each member's type when the member is accessed. Note: List/Map/Set are still eager checks, because these types are not VmObject and not lazy in the first place. Note 2: Unions of Listings or Mappings are still eager (we must force the listing to know if it satisfies Listing<A>|Listing<B>).

With the change to make mapping/listing typechecks lazy, we can now correctly handle thrown errors from within a single test case. This adjusts the test runner to consider any thrown errors as a failure for that specific test case.

stackoverflow

Nice solution! Less code than I expected. Just have some small things.

stackoverflow · 2024-09-04T12:06:48Z

pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java

+            ctx.objectBody(),
+            new GetParentForTypeNode(
+                createSourceSection(ctx),
+                visitType(typeCtx),


Can't you use parentType here as you already visited it?

Good catch!

stackoverflow · 2024-09-04T12:24:59Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeCastNode.java

@@ -45,10 +47,10 @@ public Object executeGeneric(VirtualFrame frame) {
      unresolvedTypeNode = null;
    }

+    // TODO: throw if typeNode is FunctionTypeNode (it's impossible to check)


Good idea to create an issue for that (if it doesn't exist yet) and link it here for tracking.

stackoverflow · 2024-09-04T13:13:17Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

+      var seenCollections = new MutableLong(0);
+      this.acceptTypeNode(
+          (typeNode) -> {
+            if (typeNode instanceof ListingTypeNode) {


nit: if TypeNode had a isParametric property you could just check for that and store the Class in a set and check if the class is not in the set or return false.

stackoverflow · 2024-09-04T13:27:18Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

-      for (var elem : value) {
-        elementTypeNode.execute(frame, elem);
+      for (var elem : vmList) {
+        elementTypeNode.executeEagerly(frame, elem);


Any reason why you are not checking the return of executeEargerly to check if the value didn't change, like in execute?

executeEagerly is a type test rather than a type cast. It will never return a different value; instead, it will throw if the type test fails.

stackoverflow · 2024-09-05T09:19:03Z

pkl-core/src/test/files/LanguageSnippetTests/input/listings/listing5.pkl

+      }
+    )
+    module.catch(() -> l[0][1])
+    module.catch(() -> l[0][2])


Do you really want to check l[0][1] and l[0][2] here instead of [0][0] and [0][1]? I don't think the index out of bounds check is needed here.

Whoops, good catch!

stackoverflow · 2024-09-05T11:20:51Z

pkl-core/src/test/files/LanguageSnippetTests/input/listings/listings7.pkl

@@ -0,0 +1,4 @@
+// ensure that this member is only evaluated once (should result in only one trace)


nit: all other tests are in the singular (listing4/5/6.pkl), only this one is plural.

Good catch!

holzensp

Approving to unblock, but some significant questions; especially memory pressure!

holzensp · 2024-09-05T14:48:16Z

pkl-cli/src/test/kotlin/org/pkl/cli/CliTestRunnerTest.kt

@@ -107,6 +107,138 @@ class CliTestRunnerTest {
    assertThat(err.toString()).isEqualTo("")
  }

+  @Test
+  fun `CliTestRunner thrown error in facts`(@TempDir tempDir: Path) {


Suggested change

fun `CliTestRunner thrown error in facts`(@TempDir tempDir: Path) {

fun `CliTestRunner throws error in facts`(@TempDir tempDir: Path) {

?

[throughout]

Well, it's not CliTestRunner that throws the error. I'll change this to CliTestRunner with thrown error in facts

holzensp · 2024-09-05T16:06:29Z

pkl-core/src/main/java/org/pkl/core/ast/member/ObjectMember.java

+    if (isEntry()) {
+      while (ch == ']' || ch == '=' || Character.isWhitespace(ch)) {
+        ch = text.charAt(++skip);
+      }
+    } else {
+      while (ch == '=' || Character.isWhitespace(ch)) {
+        ch = text.charAt(++skip);
+      }


Suggested change

if (isEntry()) {

while (ch == ']' || ch == '=' || Character.isWhitespace(ch)) {

ch = text.charAt(++skip);

}

} else {

while (ch == '=' || Character.isWhitespace(ch)) {

ch = text.charAt(++skip);

}

while ((ch == ']' && isEntry()) || ch == '=' || Character.isWhitespace(ch)) {

ch = text.charAt(++skip);

}

holzensp · 2024-09-05T16:29:09Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

+    @Override
+    protected void acceptTypeNode(Consumer<TypeNode> visitor) {
+      visitor.accept(this);
+    }


Is there a reason not to put this default implementation in TypeNode and reduce some of this duplication (only override when visiting is something other than visitor.accept())?

Writing it this way is less error prone--having a default implementation made it too easy to miss if something did need to change for any one TypeNode.

holzensp · 2024-09-05T16:40:21Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

-      if (skipElementTypeChecks) return;
+    protected void acceptTypeNode(Consumer<TypeNode> visitor) {
+      visitor.accept(this);
+      //noinspection ForLoopReplaceableByForEach


Is there a reason not to use foreach?

Yeah; writing it this way allows Truffle to unroll this loop into a series of statements. See ExplodeLoop for details.

holzensp · 2024-09-05T16:45:59Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

+      var seenPairs = new MutableLong(0);
+      var seenCollections = new MutableLong(0);
+      this.acceptTypeNode(
+          (typeNode) -> {


We usually avoid lambdas in any FooNode, because of the partial evaluation of node.execute(). Did you inspect this with IdealGraphVisualizer to confirm this one's okay?

Besides; this visits exhaustively, even when any of these observations hits >= 2. Is it worth pruning that (let visitors return a boolean to mark they want to continue; instead of passing in a lambda, define a class with this logic, create and instance and pass a method reference on said instance as the visitor)?

There is a truffle boundary here, so there's no impact on partial evaluation/inlining.

instead of passing in a lambda, define a class with this logic, create and instance and pass a method reference on said instance as the visitor)

Not sure what the benefit of this is? The current function syntax is sugar for new Consumer<TypeNode>(), and passing that as a reference.

But, fair point on returning early, will implement!

holzensp · 2024-09-05T16:50:09Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

+            if (typeNode instanceof ListingTypeNode) {
+              seenListings.getAndIncrement();
+            }
+            if (typeNode instanceof MappingTypeNode) {


might be marginal and I see the point of easy translation to a switch, but that's still far off... else if?

holzensp · 2024-09-05T16:54:17Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

    }
  }

-  public abstract static class ListTypeNode extends ObjectSlotTypeNode {
+  public static final class ListTypeNode extends ObjectSlotTypeNode {


This could be related to my lambda remark above.

holzensp · 2024-09-05T17:01:08Z

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

-      throw typeMismatch(value, BaseModule.getListClass());
+    @Override
+    public Object execute(VirtualFrame frame, Object value) {
+      if (!(value instanceof VmList vmList)) {


Is this a newly introduced scenario, or a previously missed one?

This is neither; I rewrote this class to remove specializations (see comment here: https://github.com/apple/pkl/pull/628/files#r1720334213)

holzensp · 2024-09-05T17:17:24Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

+  /** The original that this listing/mapping is a surrogate of. It might have its own surrogatee. */
+  private final @Nullable SELF surrogatee;


On the surrogate(e) naming; what about s/surrogate/typeCheckedThis and s/surrogatee/uncheckedThis?

How about "delegate"? That seems like a more common term for this type of thing.

holzensp · 2024-09-05T17:20:41Z

pkl-core/src/main/java/org/pkl/core/runtime/VmListingOrMapping.java

+  private final @Nullable SELF surrogatee;
+
+  private final ListingOrMappingTypeCheckNode typeCheckNode;
+  private final MaterializedFrame typeNodeFrame;
+  private final EconomicMap<Object, ObjectMember> cachedMembers = EconomicMaps.create();


I'm a little worried about the memory load this imposes (2x this for all Listings and Mappings; especially the cachedMembers). Have you measured the cost of this on a significant corpus?

This only introduces new cached members on some mappings and listings.

For example, this still contains 2 cached members, because foo is hidden and never executed:

hidden foo: Mapping = new { ["first"] = 1 ["second"] = 2 } bar: Mapping<String, Int> = foo

Same here:

hidden foo: Mapping<String, Int> = new { ["first"] = 1 }

However, this example means we get four cached members, because foo is not hidden:

foo: Mapping = new { ["first"] = 1 ["second"] = 2 } bar: Mapping<String, Int> = foo

We can certainly profile this on some real-world projects to see what this change looks like.

I added an additional optimization here: now, we will only store cached members on the delegated object if the typecast results in a new object.

This results in two cached members:

foo: Mapping = new { ["first"] = 1 ["second"] = 2 } bar: Mapping<String, Int> = foo

This results in four cached members, due to the Listing to Listing<Int> typecast:

foo: Mapping<Listing> = new { ["first"] = new { 1 } ["second"] = new { 2 } } bar: Mapping<String, Listing<Int>> = foo

I also added another optimization: A typecast will only return a new delegating object if the types are different.

This results in no delegation:

foo: Listing<String> bar = foo as Listing<String> // no delegation here because `foo` is already `Listing<String>`

* Don't create a delegated VmListingOrMapping if the element type check is the same (don't create a new object for: `new Listing<String> {} as Listing<String>`) * Don't store cached values on a delegated object unless a typecast returns a different value

bioball force-pushed the mapping-listing-typechecks branch from 39664d6 to cedaf22 Compare August 15, 2024 23:44

bioball commented Aug 15, 2024

View reviewed changes

pkl-core/src/test/files/LanguageSnippetTests/output/errors/listingTypeCheckError1.err Show resolved Hide resolved

bioball commented Aug 15, 2024

View reviewed changes

bioball force-pushed the mapping-listing-typechecks branch 7 times, most recently from ce4f445 to 86d6d7b Compare August 16, 2024 18:05

bioball mentioned this pull request Aug 16, 2024

Improve testing with stats and errors per test section #498

Merged

bioball commented Aug 16, 2024

View reviewed changes

bioball commented Aug 17, 2024

View reviewed changes

bioball force-pushed the mapping-listing-typechecks branch from aa9eba3 to 38369c2 Compare August 17, 2024 00:02

bioball mentioned this pull request Aug 17, 2024

SPICE-0010: Overhauled mapping/listing typechecks apple/pkl-evolution#11

Merged

bioball force-pushed the mapping-listing-typechecks branch from 38369c2 to 25e45a0 Compare August 17, 2024 00:03

bioball added 2 commits August 16, 2024 17:37

bioball force-pushed the mapping-listing-typechecks branch from 25e45a0 to 670d67e Compare August 17, 2024 00:37

stackoverflow approved these changes Sep 5, 2024

View reviewed changes

holzensp approved these changes Sep 5, 2024

View reviewed changes

bioball added 4 commits September 5, 2024 21:50

Address PR comments

997c5e7

Abort early on shouldEagerCheck

410358e

Adjust comments

be207a7

bioball merged commit 7868d9d into apple:main Sep 6, 2024
5 checks passed

bioball deleted the mapping-listing-typechecks branch September 6, 2024 22:05

		@@ -0,0 +1,4 @@
		// ensure that this member is only evaluated once (should result in only one trace)

	fun `CliTestRunner thrown error in facts`(@TempDir tempDir: Path) {
	fun `CliTestRunner throws error in facts`(@TempDir tempDir: Path) {

		/** The original that this listing/mapping is a surrogate of. It might have its own surrogatee. */
		private final @Nullable SELF surrogatee;

Overhaul typechecks for Mapping/Listing #628

Overhaul typechecks for Mapping/Listing #628

Conversation

bioball commented Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackoverflow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holzensp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bioball commented Aug 15, 2024 •

edited

Loading

bioball Sep 5, 2024 •

edited

Loading

bioball Sep 5, 2024 •

edited

Loading

bioball Sep 6, 2024 •

edited

Loading

bioball Sep 6, 2024 •

edited

Loading