Support kwargs in method calls (#440)

* remove support for instance literals in constructors

* update all instance creation syntax to use () not {}

* add kwargs to external calls

* support kwargs in ruby methods

* python test for kwarg methods

* rubocop

* Slightly simplify Ruby kwargs handling

* update call macro for kwargs; use Call for constructors in tests instead of InstanceLiteral

* Thread kwargs through integration tests

* add kwarg tests to rust integration_test

* Fix calls with kwargs on Ruby < 2.7

* Comment Ruby version stuff

* add no kwarg check to Java for calls

* ruby test for kwargs with method calls

* broken JS tests, WIP

* Kill SimpleCall lalrpop production

* Fix JS kwargs tests

* Tweak Python kwargs handling

* add kwargs to POLAR_LOG

* format

* whitespace

* whitespace

* Async keyword only needed if await used in function body

* Assert against error type instead of string matching

* Changelog

Co-authored-by: Alex Plotnick <[email protected]>
Co-authored-by: Gabe Jackson <[email protected]>
Co-authored-by: Sam Scott <[email protected]>

docs/changelogs/vNEXT.rst

@@ -7,32 +7,53 @@ NEXT
 Breaking changes
 ================
 
-.. TODO remove warning and replace with "None" if no breaking
-   changes.
-
 .. warning:: This release contains breaking changes. Be sure
    to follow migration steps before upgrading.
 
-Breaking change 1
------------------
+Updated ``new`` operator syntax
+-------------------------------
+
+Previously, the ``new`` operator could take one of two forms:
+
+  .. code-block:: polar
+
+    new Foo()
+
+    new Foo{}
 
-- summary of breaking change
+The former accepted positional arguments, and the latter accepted keyword
+arguments. In this release, the two forms have combined their powers, and
+positional and keyword arguments can both be passed via the parenthetical
+syntax:
 
-Link to migration guide
+  .. code-block:: polar
+
+    new Foo(1, 2, foo: 3, bar: 4)
+
+Keyword arguments *must* follow positional arguments, and they are not
+supported in languages that themselves do not support keyword arguments, such
+as Java and JavaScript (Node.js).
 
+The curly brace syntax (``new Foo{}``) is no longer valid Polar and will fail
+to parse.
+
+Migrate to the new constructor syntax by replacing curly braces with
+parentheses.
 
 New features
 ============
 
-Feature 1
----------
+Keyword arguments now supported in method calls
+-----------------------------------------------
+
+In languages that support keyword arguments, they may now be passed to method
+calls using the following syntax:
 
-- summary
-- of
-- user facing changes
+  .. code-block:: polar
 
-Link to relevant documentation section
+    foo.bar(1, 2, foo: 3, bar: 4)
 
+.. todo:: Link to relevant documentation section
 
 Other bugs & improvements
 =========================

docs/getting-started/policies/application-types.rst

@@ -191,7 +191,7 @@ using the :ref:`operator-new` operator if the class has been **registered**:
         .. code-block:: polar
             :caption: :fa:`oso` policy.polar
 
-            ?= allow(new User{name: "alice", is_admin: true}, "foo", "bar");
+            ?= allow(new User(name: "alice", is_admin: true), "foo", "bar");
 
         Initialization arguments provided in this way are passed as keywords.
         We can also pass positional arguments to the class constructor:
@@ -215,7 +215,7 @@ using the :ref:`operator-new` operator if the class has been **registered**:
         .. code-block:: polar
             :caption: :fa:`oso` policy.polar
 
-            ?= allow(new User{name: "alice", is_admin: true}, "foo", "bar");
+            ?= allow(new User(name: "alice", is_admin: true), "foo", "bar");
 
         Initialization arguments provided in this way are passed as keywords.
         We can also pass positional arguments to the class constructor:

docs/more/dev-tools/tracing.rst

@@ -32,7 +32,7 @@ Example
   [debug]     ]
   [debug]     RULE:
   [debug]     f(x) if
-  [debug]       new Foo{x: x}.foo() = x
+  [debug]       new Foo(x: x).foo() = x
   [debug]       QUERY: new (Foo{x: _x_10}, _instance_2_11) and .(_instance_2_11, foo(), _value_1_12) and _value_1_12 = _x_10, BINDINGS: {"_x_10": "12"}
   [debug]         QUERY: new (Foo{x: _x_10}, _instance_2_11), BINDINGS: {"_x_10": "12"}
   [debug]         QUERY: .(_instance_2_11, foo(), _value_1_12) and _value_1_12 = _x_10, BINDINGS: {"_instance_2_11": "Foo{x: 12}", "_x_10": "12"}

docs/using/polar-syntax.rst

@@ -282,7 +282,7 @@ The dot ``.`` operator can be used to access the value associated with
 a key in a dictionary or class instance. For example, the rule::
 
   first_name(dict, x) if
-    dict = new Person{} and
+    dict = new Person() and
     x = dict.first_name;
 
 will access the value of the field named ``"first_name"`` in ``dict``,
@@ -498,11 +498,11 @@ the dictionary.  For example::
     not {x: 1, y: 3} matches {x:1, y: 4}
 
     # a type name matches if the value has the same type
-    new Person{} matches Person
+    new Person() matches Person
 
     # The fields are checked in the same manner as dictionaries, and the type is
     # checked like above.
-    new Person{x: 1, y: 2} matches Person{x: 1}
+    new Person(x: 1, y: 2) matches Person{x: 1}
 
 For type matching, subclasses are also considered.  So, a class that is a
 subclass of ``Person`` would match ``Person{x: 1}``.

languages/java/oso/src/main/java/com/osohq/oso/Query.java

@@ -109,23 +109,21 @@ private HashMap<String, Object> nextResult() throws Exceptions.OsoException {
           if (host.hasInstance(id)) {
             throw new Exceptions.DuplicateInstanceRegistrationError(id);
           }
+
           JSONObject constructor = data.getJSONObject("constructor").getJSONObject("value");
-          JSONArray initargs;
-          if (constructor.has("InstanceLiteral")) {
-            // Keyword initargs are not supported in Java.
-            className = constructor.getJSONObject("InstanceLiteral").getString("tag");
-            throw new Exceptions.InstantiationError(className);
-          } else if (constructor.has("Call")) {
+          if (constructor.has("Call")) {
             className = constructor.getJSONObject("Call").getString("name");
-            initargs = constructor.getJSONObject("Call").getJSONArray("args");
+            JSONArray initargs = constructor.getJSONObject("Call").getJSONArray("args");
+
+            // kwargs should always be null in Java
             if (constructor.getJSONObject("Call").get("kwargs") != JSONObject.NULL) {
               throw new Exceptions.InstantiationError(className);
             }
+            host.makeInstance(className, host.polarListToJava(initargs), id);
+            break;
           } else {
             throw new Exceptions.InvalidConstructorError("Bad constructor");
           }
-          host.makeInstance(className, host.polarListToJava(initargs), id);
-          break;
         case "ExternalCall":
           instance = data.getJSONObject("instance");
           callId = data.getLong("call_id");
@@ -135,6 +133,9 @@ private HashMap<String, Object> nextResult() throws Exceptions.OsoException {
           if (!data.get("args").equals(null)) {
             jArgs = Optional.of(data.getJSONArray("args"));
           }
+          if (!data.get("kwargs").equals(null)) {
+            throw new Exceptions.InvalidCallError("Java does not support keyword arguments");
+          }
           handleCall(attrName, jArgs, instance, callId);
           break;
         case "ExternalIsa":

languages/java/oso/src/test/java/com/osohq/oso/PolarTest.java

@@ -280,8 +280,12 @@ public void testMakeInstanceFromPolar() throws Exception {
 
   @Test
   public void testNoKeywordArgs() throws Exception {
-    p.loadStr("f(x) if x = new MyClass(\"test\", id: 1);");
-    assertThrows(Exceptions.InstantiationError.class, () -> p.query("f(x)"));
+    p.registerConstant("MyClass", true);
+    assertThrows(
+        Exceptions.InstantiationError.class, () -> p.query("x = new MyClass(\"test\", id: 1)"));
+    assertThrows(
+        Exceptions.InvalidCallError.class,
+        () -> p.query("x = (new MyClass(\"test\", 1)).foo(\"test\", id: 1)"));
   }
 
   @Test

languages/js/src/Polar.test.ts

@@ -31,6 +31,7 @@ import {
   DuplicateClassAliasError,
   InlineQueryFailedError,
   InvalidConstructorError,
+  KwargsError,
   PolarFileNotFoundError,
   PolarFileExtensionError,
 } from './errors';
@@ -149,6 +150,12 @@ Application error: Foo { a: 'A' }.a is not a function at line 1, column 1`
     expect(await qvar(p, 'try(new X(), x)', 'x')).toStrictEqual([]);
   });
 
+  test('rejects keyword arguments in method calls', () => {
+    const p = new Polar();
+    p.registerClass(A);
+    expect(query(p, 'x = (new A()).a(arg: 1)')).rejects.toThrow(KwargsError);
+  });
+
   describe('animal tests', () => {
     const wolf =
       'new Animal({species: "canis lupus", genus: "canis", family: "canidae"})';
@@ -466,6 +473,13 @@ describe('#makeInstance', () => {
     const instance = p.__host().getInstance(1);
     expect(instance).toStrictEqual(new ConstructorArgs(1, 2));
   });
+
+  test('rejects keyword args', () => {
+    const p = new Polar();
+    p.registerClass(ConstructorArgs);
+    const q = 'x = new ConstructorArgs(first: 1, second: 2)';
+    expect(query(p, q)).rejects.toThrow(KwargsError);
+  });
 });
 
 describe('#registerConstant', () => {

languages/js/src/errors.ts

@@ -62,12 +62,10 @@ export class InvalidQueryEventError extends PolarError {
   }
 }
 
-export class KwargsConstructorError extends PolarError {
-  constructor(tag: string) {
-    super(
-      `To construct a JavaScript instance, use the positional args constructor syntax: new ${tag}(...)`
-    );
-    Object.setPrototypeOf(this, KwargsConstructorError.prototype);
+export class KwargsError extends PolarError {
+  constructor() {
+    super('JavaScript does not support keyword arguments');
+    Object.setPrototypeOf(this, KwargsError.prototype);
   }
 }
 

languages/js/src/helpers.ts

@@ -1,11 +1,7 @@
 import { inspect } from 'util';
 import { readFile as _readFile } from 'fs';
 
-import {
-  InvalidQueryEventError,
-  KwargsConstructorError,
-  PolarError,
-} from './errors';
+import { InvalidQueryEventError, KwargsError, PolarError } from './errors';
 import { isPolarTerm, QueryEventKind } from './types';
 import type { obj, QueryEvent } from './types';
 
@@ -102,11 +98,10 @@ function parseResult({ bindings }: obj): QueryEvent {
  */
 function parseMakeExternal(d: obj): QueryEvent {
   const instanceId = d.instance_id;
-  const ctor = d['constructor']?.value;
-  if (ctor?.InstanceLiteral !== undefined)
-    throw new KwargsConstructorError(ctor?.InstanceLiteral?.tag);
-  const tag = ctor?.Call?.name;
-  const fields = ctor?.Call?.args;
+  const ctor = d['constructor']?.value?.Call;
+  const tag = ctor?.name;
+  const fields = ctor?.args;
+  if (ctor?.kwargs) throw new KwargsError();
   if (
     !Number.isSafeInteger(instanceId) ||
     typeof tag !== 'string' ||
@@ -128,10 +123,12 @@ function parseMakeExternal(d: obj): QueryEvent {
  */
 function parseExternalCall({
   args,
+  kwargs,
   attribute,
   call_id: callId,
   instance,
 }: obj): QueryEvent {
+  if (kwargs) throw new KwargsError();
   if (
     !Number.isSafeInteger(callId) ||
     !isPolarTerm(instance) ||

languages/python/oso/oso/test_oso.polar

@@ -3,18 +3,18 @@ allow(actor, action, resource) if
     actorInRole(actor, role, resource);
 
 allow(token, action, resource) if
-    jwt = new Jwt{token: token} and
+    jwt = new Jwt(token: token) and
     jwt.attributes() = attributes and
     allow(attributes, action, resource);
 
 allow(_: {sub: sub}, action, resource) if
-    allow(new Actor{name: sub}, action, resource);
+    allow(new Actor(name: sub), action, resource);
 
 allow("guest", action, resource) if
-    allow(new Actor{name: "guest"}, action, resource);
+    allow(new Actor(name: "guest"), action, resource);
 
 allow(_: {username: name}, action, resource) if
-    allow(new Actor{name: name}, action, resource);
+    allow(new Actor(name: name), action, resource);
 
 allow(_actor: Actor, "get", _resource: Widget);
 allow(actor: Actor, "create", resource: Company) if

languages/python/oso/polar/query.py

@@ -68,18 +68,11 @@ def run(self):
     def handle_make_external(self, data):
         id = data["instance_id"]
         constructor = data["constructor"]["value"]
-        if "InstanceLiteral" in constructor:
-            cls_name = constructor["InstanceLiteral"]["tag"]
-            fields = constructor["InstanceLiteral"]["fields"]["fields"]
-            args = []
-            kwargs = {k: self.host.to_python(v) for k, v in fields.items()}
-        elif "Call" in constructor:
+        if "Call" in constructor:
             cls_name = constructor["Call"]["name"]
             args = [self.host.to_python(arg) for arg in constructor["Call"]["args"]]
-            kwargs = constructor["Call"]["kwargs"]
-            kwargs = (
-                {k: self.host.to_python(v) for k, v in kwargs.items()} if kwargs else {}
-            )
+            kwargs = constructor["Call"]["kwargs"] or {}
+            kwargs = {k: self.host.to_python(v) for k, v in kwargs.items()}
         else:
             raise PolarApiException("Bad constructor")
         self.host.make_instance(cls_name, args, kwargs, id)
@@ -103,7 +96,9 @@ def handle_external_call(self, data):
                 callable(attr) and not data["args"] is None
             ):  # If it's a function, call it with the args.
                 args = [self.host.to_python(arg) for arg in data["args"]]
-                result = attr(*args)
+                kwargs = data["kwargs"] or {}
+                kwargs = {k: self.host.to_python(v) for k, v in kwargs.items()}
+                result = attr(*args, **kwargs)
             elif not data["args"] is None:
                 raise RuntimeError(
                     f"tried to call '{attribute}' but it is not callable"

languages/python/oso/tests/parity/policies/test_api.polar

@@ -7,19 +7,19 @@ actorInRole(actor, role, resource: Widget) if
 
 allow(actor, "get", _: Http{path: path}) if
     new PathMapper("/widget/{id}").map(path) = {id: id} and
-    allow(actor, "get", new Widget{id: id});
+    allow(actor, "get", new Widget(id: id));
 
 allow(actor, "post", _: Http{path: path}) if
-    new PathMapper{template: "/widget/"}.map(path) = {} and
-    allow(actor, "create", new Widget{});
+    new PathMapper(template: "/widget/").map(path) = {} and
+    allow(actor, "create", new Widget());
 
 allow(actor, "what", _: Http{path: path}) if
     new PathMapper("/widget/{id}").map(path) = {id: id} and
-    allow(actor, "unparameterised_get", new Widget{id: id});
+    allow(actor, "unparameterised_get", new Widget(id: id));
 
 allow(actor, "what", _: Http{path: path, query: {param: "foo"}}) if
     new PathMapper("/widget/{id}").map(path) = {id: id} and
-    allow(actor, "parameterised_get", new Widget{id: id});
+    allow(actor, "parameterised_get", new Widget(id: id));
 
 allow(actor, "get", resource: Widget) if resource.frob("Widget") = x;
 allow(actor, "get", resource: DooDad) if resource.frob("DooDad") = x;
@@ -51,7 +51,7 @@ allow_two(actor, action, resource) if checkResource(_x, resource);
 checkResource(1, resource: Widget); # two slightly different specs so need to check
 checkResource("1", resource: Widget); # which to prioritise
 
-?= allow_two(_actor, _action, new Widget{});
+?= allow_two(_actor, _action, new Widget());
 
 # for testing lists
 allow(actor: Actor, "invite", resource: Widget) if

languages/python/oso/tests/parity/test_api.py

@@ -189,7 +189,7 @@ def test_instance_round_trip(polar, query, qvar):
 def test_instance_initialization(polar, query, qvar):
     # test round trip through kb query
     user = Actor("sam")
-    env = query('new Actor{name:"sam"} = returned_user')[0]
+    env = query('new Actor(name:"sam") = returned_user')[0]
     assert polar.host.to_python(env["returned_user"]) == user
 
     env = query('new Actor(name:"sam") = returned_user')[0]