From 072a0e3eb919bc2d6c6913967e03c646aa075092 Mon Sep 17 00:00:00 2001 From: Leina McDermott Date: Mon, 21 Sep 2020 21:20:49 -0400 Subject: [PATCH] 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 Co-authored-by: Gabe Jackson Co-authored-by: Sam Scott --- docs/changelogs/vNEXT.rst | 47 +++++-- .../policies/application-types.rst | 4 +- docs/more/dev-tools/tracing.rst | 2 +- docs/using/polar-syntax.rst | 6 +- .../src/main/java/com/osohq/oso/Query.java | 19 +-- .../test/java/com/osohq/oso/PolarTest.java | 8 +- languages/js/src/Polar.test.ts | 14 +++ languages/js/src/errors.ts | 10 +- languages/js/src/helpers.ts | 17 ++- languages/python/oso/oso/test_oso.polar | 8 +- languages/python/oso/polar/query.py | 17 +-- .../oso/tests/parity/policies/test_api.polar | 12 +- languages/python/oso/tests/parity/test_api.py | 2 +- languages/python/oso/tests/test_polar.py | 39 ++++++ languages/ruby/lib/oso/polar/host.rb | 1 + languages/ruby/lib/oso/polar/query.rb | 52 ++++---- languages/ruby/spec/oso/oso_spec.rb | 4 +- languages/ruby/spec/oso/polar/polar_spec.rb | 90 ++++++++------ languages/rust/oso/src/query.rs | 9 +- polar-core/benches/bench.rs | 10 +- polar-core/src/events.rs | 5 +- polar-core/src/macros.rs | 24 +++- polar-core/src/parser.rs | 4 +- polar-core/src/polar.lalrpop | 29 +---- polar-core/src/rewrites.rs | 30 ++--- polar-core/src/vm.rs | 55 +++++---- polar-core/tests/integration_tests.rs | 115 +++++++++++------- polar-core/tests/mock_externals.rs | 16 ++- polar-core/tests/serialize.rs | 13 +- 29 files changed, 397 insertions(+), 265 deletions(-) diff --git a/docs/changelogs/vNEXT.rst b/docs/changelogs/vNEXT.rst index 8000957bd0..86ee53275b 100644 --- a/docs/changelogs/vNEXT.rst +++ b/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 ========================= diff --git a/docs/getting-started/policies/application-types.rst b/docs/getting-started/policies/application-types.rst index 8061e26719..030a1cf53f 100644 --- a/docs/getting-started/policies/application-types.rst +++ b/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: diff --git a/docs/more/dev-tools/tracing.rst b/docs/more/dev-tools/tracing.rst index 813b52ac8b..99baacffd4 100644 --- a/docs/more/dev-tools/tracing.rst +++ b/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"} diff --git a/docs/using/polar-syntax.rst b/docs/using/polar-syntax.rst index 3eb2897580..6c75a16968 100644 --- a/docs/using/polar-syntax.rst +++ b/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}``. diff --git a/languages/java/oso/src/main/java/com/osohq/oso/Query.java b/languages/java/oso/src/main/java/com/osohq/oso/Query.java index 5704a29e36..4057c2f683 100644 --- a/languages/java/oso/src/main/java/com/osohq/oso/Query.java +++ b/languages/java/oso/src/main/java/com/osohq/oso/Query.java @@ -109,23 +109,21 @@ private HashMap 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 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": diff --git a/languages/java/oso/src/test/java/com/osohq/oso/PolarTest.java b/languages/java/oso/src/test/java/com/osohq/oso/PolarTest.java index a913a479c5..d141585d61 100644 --- a/languages/java/oso/src/test/java/com/osohq/oso/PolarTest.java +++ b/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 diff --git a/languages/js/src/Polar.test.ts b/languages/js/src/Polar.test.ts index dc4054718b..f1f5bdc646 100644 --- a/languages/js/src/Polar.test.ts +++ b/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', () => { diff --git a/languages/js/src/errors.ts b/languages/js/src/errors.ts index 224d309ae1..87f2d311ca 100644 --- a/languages/js/src/errors.ts +++ b/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); } } diff --git a/languages/js/src/helpers.ts b/languages/js/src/helpers.ts index 8adda94fe4..649e64ea39 100644 --- a/languages/js/src/helpers.ts +++ b/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) || diff --git a/languages/python/oso/oso/test_oso.polar b/languages/python/oso/oso/test_oso.polar index cadd8a9de7..25e546709e 100644 --- a/languages/python/oso/oso/test_oso.polar +++ b/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 diff --git a/languages/python/oso/polar/query.py b/languages/python/oso/polar/query.py index 0326ab8e39..bf70999936 100644 --- a/languages/python/oso/polar/query.py +++ b/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" diff --git a/languages/python/oso/tests/parity/policies/test_api.polar b/languages/python/oso/tests/parity/policies/test_api.polar index 367b0bb2b0..548c157f34 100644 --- a/languages/python/oso/tests/parity/policies/test_api.polar +++ b/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 diff --git a/languages/python/oso/tests/parity/test_api.py b/languages/python/oso/tests/parity/test_api.py index a514450941..56cd845345 100644 --- a/languages/python/oso/tests/parity/test_api.py +++ b/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] diff --git a/languages/python/oso/tests/test_polar.py b/languages/python/oso/tests/test_polar.py index 726eecdfaf..229308fe12 100644 --- a/languages/python/oso/tests/test_polar.py +++ b/languages/python/oso/tests/test_polar.py @@ -473,6 +473,7 @@ def __init__(self, a, b, bar, baz): polar.register_class(Foo) + # test positional args instance = qvar( "instance = new Foo(1,2,3,4)", "instance", @@ -483,6 +484,7 @@ def __init__(self, a, b, bar, baz): assert instance.bar == 3 assert instance.baz == 4 + # test positional and kwargs instance = qvar( "instance = new Foo(1, 2, bar: 3, baz: 4)", "instance", @@ -493,6 +495,7 @@ def __init__(self, a, b, bar, baz): assert instance.bar == 3 assert instance.baz == 4 + # test kwargs instance = qvar( "instance = new Foo(bar: 3, a: 1, baz: 4, b: 2)", "instance", @@ -700,3 +703,39 @@ def map(self, f): polar.register_class(Foo) polar.load_str("f(x: Foo) if x.map(Foo.plus_one) = [2, 3, 4];") assert next(polar.query_rule("f", Foo([1, 2, 3]))) + + +def test_method_with_kwargs(polar, qvar): + class Test: + def kwarg_method(self, x=1, y=2): + self.x = x + self.y = y + return True + + polar.register_class(Test) + rules = """ + defaults(result) if + test = new Test() and + test.kwarg_method() and + result = [test.x, test.y]; + + kwargs(result) if + test = new Test() and + test.kwarg_method(y: 4, x: 3) and + result = [test.x, test.y]; + + args(result) if + test = new Test() and + test.kwarg_method(5, 6) and + result = [test.x, test.y]; + + mixed(result) if + test = new Test() and + test.kwarg_method(7, y: 8) and + result = [test.x, test.y]; + """ + polar.load_str(rules) + qvar("defaults(result)", "result") == [1, 2] + qvar("kwargs(result)", "result") == [3, 4] + qvar("args(result)", "result") == [5, 6] + qvar("mixed(result)", "result") == [7, 8] diff --git a/languages/ruby/lib/oso/polar/host.rb b/languages/ruby/lib/oso/polar/host.rb index ef85252340..894dad3e39 100644 --- a/languages/ruby/lib/oso/polar/host.rb +++ b/languages/ruby/lib/oso/polar/host.rb @@ -121,6 +121,7 @@ def cache_instance(instance, id: nil) # @raise [PolarRuntimeError] if instance construction fails. def make_instance(cls_name, args:, kwargs:, id:) # rubocop:disable Metrics/MethodLength constructor = get_constructor(cls_name) + # The kwargs.empty? checks are for Ruby < 2.7. instance = if constructor == :new if kwargs.empty? get_class(cls_name).__send__(:new, *args) diff --git a/languages/ruby/lib/oso/polar/query.rb b/languages/ruby/lib/oso/polar/query.rb index 1374d04410..68d8bbf226 100644 --- a/languages/ruby/lib/oso/polar/query.rb +++ b/languages/ruby/lib/oso/polar/query.rb @@ -43,16 +43,18 @@ def question_result(result, call_id:) # @param args [Array] # @raise [InvalidCallError] if the method doesn't exist on the instance or # the args passed to the method are invalid. - def register_call(attribute, call_id:, instance:, args:) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + def register_call(attribute, call_id:, instance:, args:, kwargs:) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength return if calls.key?(call_id) instance = host.to_ruby(instance) - if args.nil? - result = instance.__send__(attribute) - else - args = args.map { |a| host.to_ruby(a) } - result = instance.__send__(attribute, *args) - end + args = args.map { |a| host.to_ruby(a) } + kwargs = Hash[kwargs.map { |k, v| [k.to_sym, host.to_ruby(v)] }] + # The kwargs.empty? check is for Ruby < 2.7. + result = if kwargs.empty? + instance.__send__(attribute, *args) + else + instance.__send__(attribute, *args, **kwargs) + end result = [result].to_enum unless result.is_a? Enumerator # Call must be a generator. calls[call_id] = result.lazy rescue ArgumentError, NoMethodError @@ -93,8 +95,8 @@ def application_error(message) # @param call_id [Integer] # @param instance [Hash] # @raise [Error] if the FFI call raises one. - def handle_call(attribute, call_id:, instance:, args:) - register_call(attribute, call_id: call_id, instance: instance, args: args) + def handle_call(attribute, call_id:, instance:, args:, kwargs:) + register_call(attribute, call_id: call_id, instance: instance, args: args, kwargs: kwargs) result = JSON.dump(next_call_result(call_id)) call_result(result, call_id: call_id) rescue InvalidCallError => e @@ -104,28 +106,17 @@ def handle_call(attribute, call_id:, instance:, args:) call_result(nil, call_id: call_id) end - def handle_make_external(data) # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity + def handle_make_external(data) # rubocop:disable Metrics/AbcSize id = data['instance_id'] raise DuplicateInstanceRegistrationError, id if host.instance? id constructor = data['constructor']['value'] - if constructor.key? 'InstanceLiteral' - cls_name = constructor['InstanceLiteral']['tag'] - fields = constructor['InstanceLiteral']['fields']['fields'] - kwargs = Hash[fields.map { |k, v| [k.to_sym, host.to_ruby(v)] }] - args = [] - elsif constructor.key? 'Call' - cls_name = constructor['Call']['name'] - args = constructor['Call']['args'].map { |arg| host.to_ruby(arg) } - kwargs = constructor['Call']['kwargs'] - kwargs = if kwargs.nil? - {} - else - Hash[kwargs.map { |k, v| [k.to_sym, host.to_ruby(v)] }] - end - else - raise InvalidConstructorError - end + raise InvalidConstructorError unless constructor.key? 'Call' + + cls_name = constructor['Call']['name'] + args = constructor['Call']['args'].map { |arg| host.to_ruby(arg) } + kwargs = constructor['Call']['kwargs'] || {} + kwargs = Hash[kwargs.map { |k, v| [k.to_sym, host.to_ruby(v)] }] host.make_instance(cls_name, args: args, kwargs: kwargs, id: id) end @@ -134,7 +125,7 @@ def handle_make_external(data) # rubocop:disable Metrics/AbcSize, Metrics/Method # @yieldparam [Hash] # @return [Enumerator] # @raise [Error] if any of the FFI calls raise one. - def start # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength + def start # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity Enumerator.new do |yielder| # rubocop:disable Metrics/BlockLength loop do # rubocop:disable Metrics/BlockLength event = ffi_query.next_event @@ -149,8 +140,9 @@ def start # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metri call_id = event.data['call_id'] instance = event.data['instance'] attribute = event.data['attribute'] - args = event.data['args'] - handle_call(attribute, call_id: call_id, instance: instance, args: args) + args = event.data['args'] || [] + kwargs = event.data['kwargs'] || {} + handle_call(attribute, call_id: call_id, instance: instance, args: args, kwargs: kwargs) when 'ExternalIsSubSpecializer' instance_id = event.data['instance_id'] left_tag = event.data['left_class_tag'] diff --git a/languages/ruby/spec/oso/oso_spec.rb b/languages/ruby/spec/oso/oso_spec.rb index eaf82c1113..ac5db2d11a 100644 --- a/languages/ruby/spec/oso/oso_spec.rb +++ b/languages/ruby/spec/oso/oso_spec.rb @@ -29,7 +29,7 @@ def initialize(name:) end subject.load_str <<~POLAR allow(u: User{}, 1, 2) if - x = new User{name: "alice"} + x = new User(name: "alice") and x.name = u.name and x.special = true; POLAR @@ -81,7 +81,7 @@ def initialize(id:) subject.load_str <<~POLAR 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, "get", widget: Widget{}) if widget.id = "12"; POLAR widget12 = Oso::Http.new('host', '/widget/12', {}) diff --git a/languages/ruby/spec/oso/polar/polar_spec.rb b/languages/ruby/spec/oso/polar/polar_spec.rb index 129cf44ca5..1166ffee37 100644 --- a/languages/ruby/spec/oso/polar/polar_spec.rb +++ b/languages/ruby/spec/oso/polar/polar_spec.rb @@ -414,18 +414,18 @@ def h subject.register_class(Bar) subject.register_class(Foo, from_polar: -> { Foo.new('A') }) - expect(qvar(subject, 'new Foo{}.a = x', 'x', one: true)).to eq('A') - expect(qvar(subject, 'new Foo{}.a() = x', 'x', one: true)).to eq('A') - expect(qvar(subject, 'new Foo{}.b = x', 'x', one: true)).to eq('b') - expect(qvar(subject, 'new Foo{}.b() = x', 'x', one: true)).to eq('b') - expect(qvar(subject, 'new Foo{}.c = x', 'x', one: true)).to eq('c') - expect(qvar(subject, 'new Foo{}.c() = x', 'x', one: true)).to eq('c') - expect(qvar(subject, 'new Foo{} = f and f.a() = x', 'x', one: true)).to eq('A') - expect(qvar(subject, 'new Foo{}.bar().y() = x', 'x', one: true)).to eq('y') - expect(qvar(subject, 'new Foo{}.e = x', 'x')).to eq([[1, 2, 3]]) - expect(qvar(subject, 'new Foo{}.f = x', 'x')).to eq([[1, 2, 3], [4, 5, 6], 7]) - expect(qvar(subject, 'new Foo{}.g.hello = x', 'x', one: true)).to eq('world') - expect(qvar(subject, 'new Foo{}.h = x', 'x', one: true)).to be true + expect(qvar(subject, 'new Foo().a = x', 'x', one: true)).to eq('A') + expect(qvar(subject, 'new Foo().a() = x', 'x', one: true)).to eq('A') + expect(qvar(subject, 'new Foo().b = x', 'x', one: true)).to eq('b') + expect(qvar(subject, 'new Foo().b() = x', 'x', one: true)).to eq('b') + expect(qvar(subject, 'new Foo().c = x', 'x', one: true)).to eq('c') + expect(qvar(subject, 'new Foo().c() = x', 'x', one: true)).to eq('c') + expect(qvar(subject, 'new Foo().a() = x', 'x', one: true)).to eq('A') + expect(qvar(subject, 'new Foo().bar().y() = x', 'x', one: true)).to eq('y') + expect(qvar(subject, 'new Foo().e = x', 'x')).to eq([[1, 2, 3]]) + expect(qvar(subject, 'new Foo().f = x', 'x')).to eq([[1, 2, 3], [4, 5, 6], 7]) + expect(qvar(subject, 'new Foo().g.hello = x', 'x', one: true)).to eq('world') + expect(qvar(subject, 'new Foo().h = x', 'x', one: true)).to be true end it 'respects the Ruby inheritance hierarchy for class specialization' do # rubocop:disable Metrics/BlockLength @@ -479,24 +479,40 @@ def x try(_: A{}, res) if res = 1; POLAR - expect(qvar(subject, 'new A{}.a = x', 'x', one: true)).to eq('A') - expect(qvar(subject, 'new A{}.x = x', 'x', one: true)).to eq('A') - expect(qvar(subject, 'new B{}.a = x', 'x', one: true)).to eq('A') - expect(qvar(subject, 'new B{}.b = x', 'x', one: true)).to eq('B') - expect(qvar(subject, 'new B{}.x = x', 'x', one: true)).to eq('B') - expect(qvar(subject, 'new C{}.a = x', 'x', one: true)).to eq('A') - expect(qvar(subject, 'new C{}.b = x', 'x', one: true)).to eq('B') - expect(qvar(subject, 'new C{}.c = x', 'x', one: true)).to eq('C') - expect(qvar(subject, 'new C{}.x = x', 'x', one: true)).to eq('C') - expect(qvar(subject, 'new X{}.x = x', 'x', one: true)).to eq('X') + expect(qvar(subject, 'new A().a = x', 'x', one: true)).to eq('A') + expect(qvar(subject, 'new A().x = x', 'x', one: true)).to eq('A') + expect(qvar(subject, 'new B().a = x', 'x', one: true)).to eq('A') + expect(qvar(subject, 'new B().b = x', 'x', one: true)).to eq('B') + expect(qvar(subject, 'new B().x = x', 'x', one: true)).to eq('B') + expect(qvar(subject, 'new C().a = x', 'x', one: true)).to eq('A') + expect(qvar(subject, 'new C().b = x', 'x', one: true)).to eq('B') + expect(qvar(subject, 'new C().c = x', 'x', one: true)).to eq('C') + expect(qvar(subject, 'new C().x = x', 'x', one: true)).to eq('C') + expect(qvar(subject, 'new X().x = x', 'x', one: true)).to eq('X') - expect(query(subject, 'test(new A{})').length).to be 1 - expect(query(subject, 'test(new B{})').length).to be 2 + expect(query(subject, 'test(new A())').length).to be 1 + expect(query(subject, 'test(new B())').length).to be 2 - expect(qvar(subject, 'try(new A{}, x)', 'x')).to eq([1]) - expect(qvar(subject, 'try(new B{}, x)', 'x')).to eq([2, 1]) - expect(qvar(subject, 'try(new C{}, x)', 'x')).to eq([3, 2, 1]) - expect(qvar(subject, 'try(new X{}, x)', 'x')).to eq([]) + expect(qvar(subject, 'try(new A(), x)', 'x')).to eq([1]) + expect(qvar(subject, 'try(new B(), x)', 'x')).to eq([2, 1]) + expect(qvar(subject, 'try(new C(), x)', 'x')).to eq([3, 2, 1]) + expect(qvar(subject, 'try(new X(), x)', 'x')).to eq([]) + end + + context 'when calling methods on ruby instances' do + before do + stub_const('TestClass', + Class.new do + def my_method(one, two, three:, four:) + [one, two, three, four] + end + end) + subject.register_class(TestClass) + end + + it 'accepts positional and keyword args' do + expect(qvar(subject, 'x = (new TestClass()).my_method(1, 2, four: 4, three: 3)', 'x')).to eq([[1, 2, 3, 4]]) + end end context 'animal tests' do # rubocop:disable Metrics/BlockLength @@ -520,16 +536,16 @@ def ==(other) subject.register_class(Animal) end - let(:wolf) { 'new Animal{species: "canis lupus", genus: "canis", family: "canidae"}' } - let(:dog) { 'new Animal{species: "canis familiaris", genus: "canis", family: "canidae"}' } - let(:canine) { 'new Animal{genus: "canis", family: "canidae"}' } - let(:canid) { 'new Animal{family: "canidae"}' } - let(:animal) { 'new Animal{}' } + let(:wolf) { 'new Animal(species: "canis lupus", genus: "canis", family: "canidae")' } + let(:dog) { 'new Animal(species: "canis familiaris", genus: "canis", family: "canidae")' } + let(:canine) { 'new Animal(genus: "canis", family: "canidae")' } + let(:canid) { 'new Animal(family: "canidae")' } + let(:animal) { 'new Animal()' } it 'can unify instances' do subject.load_str <<~POLAR - yup() if new Animal{family: "steve"} = new Animal{family: "steve"}; - nope() if new Animal{family: "steve"} = new Animal{family: "gabe"}; + yup() if new Animal(family: "steve") = new Animal(family: "steve"); + nope() if new Animal(family: "steve") = new Animal(family: "gabe"); POLAR expect(query(subject, 'yup()')).to eq([{}]) expect(query(subject, 'nope()')).to eq([]) @@ -709,8 +725,8 @@ def foo end end) subject.register_class(Foo) - expect(query(subject, 'new Foo{} = {bar: "bar"}')).to eq([]) - expect { query(subject, 'new Foo{}.bar = "bar"') }.to raise_error do |e| + expect(query(subject, 'new Foo() = {bar: "bar"}')).to eq([]) + expect { query(subject, 'new Foo().bar = "bar"') }.to raise_error do |e| expect(e).to be_an Oso::Polar::PolarRuntimeError end end diff --git a/languages/rust/oso/src/query.rs b/languages/rust/oso/src/query.rs index 74e25c4930..d8539d7cfb 100644 --- a/languages/rust/oso/src/query.rs +++ b/languages/rust/oso/src/query.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::collections::HashMap; use std::sync::{Arc, Mutex}; @@ -56,7 +57,8 @@ impl Query { instance, attribute, args, - } => self.handle_external_call(call_id, instance, attribute, args), + kwargs, + } => self.handle_external_call(call_id, instance, attribute, args, kwargs), QueryEvent::ExternalOp { call_id, operator, @@ -114,7 +116,6 @@ impl Query { fn handle_make_external(&mut self, instance_id: u64, constructor: Term) -> crate::Result<()> { let mut host = self.host.lock().unwrap(); match constructor.value() { - Value::InstanceLiteral(InstanceLiteral { .. }) => todo!("instantiate from literal"), Value::Call(Call { name, args, .. }) => { let _instance = host.make_instance(name, args.clone(), instance_id); } @@ -163,7 +164,11 @@ impl Query { instance: Term, name: Symbol, args: Option>, + kwargs: Option>, ) -> crate::Result<()> { + if kwargs.is_some() { + return lazy_error!("Invalid call error: kwargs not supported in Rust."); + } let instance = Instance::from_polar(&instance, &mut self.host.lock().unwrap()).unwrap(); if let Err(e) = self.register_call(call_id, instance, name, args) { self.application_error(e); diff --git a/polar-core/benches/bench.rs b/polar-core/benches/bench.rs index ee9a1a37da..ff4606ecc1 100644 --- a/polar-core/benches/bench.rs +++ b/polar-core/benches/bench.rs @@ -188,8 +188,8 @@ pub fn n_plus_one_queries(c: &mut Criterion) { // Make some instances. The literals dont change anything, but is convenient for // us to track things. - let child = runner.make_external(instance!("Person")); - let grandchild = runner.make_external(instance!("Person")); + let child = runner.make_external(call!("Person")); + let grandchild = runner.make_external(call!("Person")); let n_children = term!(vec![child; n]); // n children in a list let one_grandchild = term!(vec![grandchild]); @@ -228,7 +228,7 @@ pub fn n_plus_one_queries(c: &mut Criterion) { b.iter_batched_ref( || { let mut runner = - runner_from_query("has_grandchild_called(new Person{}, \"bert\")"); + runner_from_query("has_grandchild_called(new Person(), \"bert\")"); runner.register_pseudo_class("Person"); runner.load_str(policy).unwrap(); n_results(&mut runner, *n); @@ -337,11 +337,11 @@ impl Runner { fn handle_debug(&mut self, _: String) {} - fn make_external(&mut self, literal: InstanceLiteral) -> Term { + fn make_external(&mut self, constructor: Call) -> Term { let instance_id = self.polar.get_external_id(); Term::new_from_test(Value::ExternalInstance(ExternalInstance { instance_id, - constructor: Some(Term::new_from_test(Value::InstanceLiteral(literal))), + constructor: Some(Term::new_from_test(Value::Call(constructor))), repr: None, })) } diff --git a/polar-core/src/events.rs b/polar-core/src/events.rs index f4a6fd0b69..68f339ac61 100644 --- a/polar-core/src/events.rs +++ b/polar-core/src/events.rs @@ -2,6 +2,7 @@ use super::kb::*; use super::terms::*; use super::traces::*; use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; #[allow(clippy::large_enum_variant)] #[must_use] @@ -28,8 +29,10 @@ pub enum QueryEvent { /// Field name to lookup or method name to call. A class name indicates a constructor /// should be called. attribute: Symbol, - /// List of arguments to use if this is a method call. + /// List of arguments to a method call. args: Option>, + /// A map of keyword arguments to a method call. + kwargs: Option>, }, /// Checks if the instance is an instance of (a subclass of) the class_tag. diff --git a/polar-core/src/macros.rs b/polar-core/src/macros.rs index b071a70e16..96b394f5a4 100644 --- a/polar-core/src/macros.rs +++ b/polar-core/src/macros.rs @@ -102,9 +102,23 @@ macro_rules! sym { }; } +#[macro_export] +macro_rules! string { + ($arg:expr) => { + Value::String($arg.into()) + }; +} + // TODO: support kwargs #[macro_export] macro_rules! call { + ($name:expr) => { + Call { + name: sym!($name), + args: vec![], + kwargs: None + } + }; ($name:expr, [$($args:expr),*]) => { Call { name: sym!($name), @@ -114,8 +128,14 @@ macro_rules! call { kwargs: None } }; - ($name:expr) => { - Value::String($name.into()) + ($name:expr, [$($args:expr),*], $fields:expr) => { + Call { + name: sym!($name), + args: vec![ + $(term!($args)),* + ], + kwargs: Some($fields) + } }; } diff --git a/polar-core/src/parser.rs b/polar-core/src/parser.rs index ffe36f066f..d3a5a2eaf8 100644 --- a/polar-core/src/parser.rs +++ b/polar-core/src/parser.rs @@ -233,9 +233,9 @@ mod tests { #[test] fn test_parse_new() { - let f = r#"a(x) if x = new Foo{a: 1};"#; + let f = r#"a(x) if x = new Foo(a: 1);"#; let results = parse_rules(0, f).unwrap(); - assert_eq!(results[0].to_polar(), r#"a(x) if x = new Foo{a: 1};"#); + assert_eq!(results[0].to_polar(), r#"a(x) if x = new Foo(a: 1);"#); } #[test] diff --git a/polar-core/src/polar.lalrpop b/polar-core/src/polar.lalrpop index 5192f60b2f..d26b0a1e34 100644 --- a/polar-core/src/polar.lalrpop +++ b/polar-core/src/polar.lalrpop @@ -106,24 +106,20 @@ RestVar: Value = "*" => { Value::RestVariable(n) }; -// Calls that don't support kwargs. -// For now only constructors allow kwargs. -// One day maybe external lookups and rules. -SimpleCall: Value = { +Call: Value = { + // No args. "(" ")" => { let args = vec![]; let kwargs = None; Value::Call(Call{name, args, kwargs}) }, + // Positional args only. "(" ",")*> ")" => { args.push(arg); let kwargs = None; Value::Call(Call{name, args, kwargs}) }, -}; - -Call: Value = { - => call, + // Positional args + kwargs. "(" ",")*> >>)>")" => { let kwargs = Some(fields); Value::Call(Call{name, args, kwargs}) @@ -163,11 +159,6 @@ DictionaryPattern: Value = >> => { Value::Pattern(Pattern::Dictionary(fields)) }; -InstanceLiteralTerm: Value = >> => { - let instance = InstanceLiteral{tag, fields }; - Value::InstanceLiteral(instance) -}; - InstanceLiteralPattern: Value = >> => { let instance = InstanceLiteral{tag, fields}; Value::Pattern(Pattern::Instance(instance)) @@ -181,14 +172,6 @@ BuiltinOperator: Operator = { }; New: Value = { - // Keyword argument constructor. - "new" > => { - let args = vec![literal]; - let op = Operation{operator: Operator::New, args}; - Value::Expression(op) - }, - - // Mixed argument constructor. "new" > => { let args = vec![call]; let op = Operation{operator: Operator::New, args}; @@ -264,7 +247,7 @@ Exp10: Term = { } CallTerm: Value = { - , + , => Value::String(s.0), "(" ")", } @@ -496,7 +479,7 @@ pub Value: Value = { , , , - , + , >, }; diff --git a/polar-core/src/rewrites.rs b/polar-core/src/rewrites.rs index 805f2a8a34..926412ee48 100644 --- a/polar-core/src/rewrites.rs +++ b/polar-core/src/rewrites.rs @@ -294,60 +294,60 @@ mod tests { #[test] fn rewrite_nested_literal() { let mut kb = KnowledgeBase::new(); - let mut term = parse_query("new Foo { x: bar.y }"); - assert_eq!(term.to_polar(), "new Foo{x: bar.y}"); + let mut term = parse_query("new Foo(x: bar.y)"); + assert_eq!(term.to_polar(), "new Foo(x: bar.y)"); rewrite_term(&mut term, &mut kb); assert_eq!( term.to_polar(), - ".(bar, \"y\", _value_2) and new (Foo{x: _value_2}, _instance_1) and _instance_1" + ".(bar, \"y\", _value_2) and new (Foo(x: _value_2), _instance_1) and _instance_1" ); - let mut term = parse_query("f(new Foo { x: bar.y })"); - assert_eq!(term.to_polar(), "f(new Foo{x: bar.y})"); + let mut term = parse_query("f(new Foo(x: bar.y))"); + assert_eq!(term.to_polar(), "f(new Foo(x: bar.y))"); rewrite_term(&mut term, &mut kb); assert_eq!( term.to_polar(), - ".(bar, \"y\", _value_4) and new (Foo{x: _value_4}, _instance_3) and f(_instance_3)" + ".(bar, \"y\", _value_4) and new (Foo(x: _value_4), _instance_3) and f(_instance_3)" ); } #[test] fn rewrite_class_constructor() { let mut kb = KnowledgeBase::new(); - let mut term = parse_query("new Foo{a: 1, b: 2}"); - assert_eq!(term.to_polar(), "new Foo{a: 1, b: 2}"); + let mut term = parse_query("new Foo(a: 1, b: 2)"); + assert_eq!(term.to_polar(), "new Foo(a: 1, b: 2)"); rewrite_term(&mut term, &mut kb); // @ means external constructor assert_eq!( term.to_polar(), - "new (Foo{a: 1, b: 2}, _instance_1) and _instance_1" + "new (Foo(a: 1, b: 2), _instance_1) and _instance_1" ); } #[test] fn rewrite_nested_class_constructor() { let mut kb = KnowledgeBase::new(); - let mut term = parse_query("new Foo{a: 1, b: new Foo{a: 2, b: 3}}"); - assert_eq!(term.to_polar(), "new Foo{a: 1, b: new Foo{a: 2, b: 3}}"); + let mut term = parse_query("new Foo(a: 1, b: new Foo(a: 2, b: 3))"); + assert_eq!(term.to_polar(), "new Foo(a: 1, b: new Foo(a: 2, b: 3))"); rewrite_term(&mut term, &mut kb); assert_eq!( term.to_polar(), - "new (Foo{a: 2, b: 3}, _instance_2) and new (Foo{a: 1, b: _instance_2}, _instance_1) and _instance_1" + "new (Foo(a: 2, b: 3), _instance_2) and new (Foo(a: 1, b: _instance_2), _instance_1) and _instance_1" ); } #[test] fn rewrite_rules_constructor() { let mut kb = KnowledgeBase::new(); - let mut rules = parse_rules("rule_test(new Foo{a: 1, b: 2});"); - assert_eq!(rules[0].to_polar(), "rule_test(new Foo{a: 1, b: 2});"); + let mut rules = parse_rules("rule_test(new Foo(a: 1, b: 2));"); + assert_eq!(rules[0].to_polar(), "rule_test(new Foo(a: 1, b: 2));"); rewrite_rule(&mut rules[0], &mut kb); assert_eq!( rules[0].to_polar(), - "rule_test(_instance_1) if new (Foo{a: 1, b: 2}, _instance_1);" + "rule_test(_instance_1) if new (Foo(a: 1, b: 2), _instance_1);" ) } diff --git a/polar-core/src/vm.rs b/polar-core/src/vm.rs index 0c1ea1e7d3..47ac055e91 100644 --- a/polar-core/src/vm.rs +++ b/polar-core/src/vm.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::collections::{HashMap, HashSet}; use std::fmt::Write; use std::rc::Rc; @@ -1136,16 +1137,25 @@ impl PolarVirtualMachine { field: &Term, check_errors: bool, ) -> PolarResult { - let (field_name, args): (Symbol, Option>) = match self.deref(field).value() { - Value::Call(Call { - name, - args, - kwargs: None, - }) => ( + let (field_name, args, kwargs): ( + Symbol, + Option>, + Option>, + ) = match self.deref(field).value() { + Value::Call(Call { name, args, kwargs }) => ( name.clone(), Some(args.iter().map(|arg| self.deep_deref(arg)).collect()), + match kwargs { + Some(unwrapped) => Some( + unwrapped + .iter() + .map(|(k, v)| (k.to_owned(), self.deep_deref(v))) + .collect(), + ), + None => None, + }, ), - Value::String(field) => (Symbol(field.clone()), None), + Value::String(field) => (Symbol(field.clone()), None, None), v => { return Err(self.type_error( &field, @@ -1168,17 +1178,19 @@ impl PolarVirtualMachine { self.log_with( || { let mut msg = format!("LOOKUP: {}.{}", instance.to_string(), field_name); - if let Some(arguments) = &args { - msg.push('('); - msg.push_str( - &arguments - .iter() - .map(|a| a.to_polar()) - .collect::>() - .join(", "), - ); - msg.push(')'); - } + msg.push('('); + let args = args + .clone() + .unwrap_or_else(Vec::new) + .into_iter() + .map(|a| a.to_polar()); + let kwargs = kwargs + .clone() + .unwrap_or_else(BTreeMap::new) + .into_iter() + .map(|(k, v)| format!("{}: {}", k, v.to_polar())); + msg.push_str(&args.chain(kwargs).collect::>().join(", ")); + msg.push(')'); msg }, &[], @@ -1189,6 +1201,7 @@ impl PolarVirtualMachine { instance: self.deep_deref(instance), attribute: field_name, args, + kwargs, }) } @@ -2895,7 +2908,7 @@ mod tests { let dict = Dictionary { fields }; vm.push_goal(Goal::Lookup { dict: dict.clone(), - field: term!(call!("x")), + field: term!(string!("x")), value: term!(1), }) .unwrap(); @@ -2907,7 +2920,7 @@ mod tests { // Lookup with incorrect value vm.push_goal(Goal::Lookup { dict: dict.clone(), - field: term!(call!("x")), + field: term!(string!("x")), value: term!(2), }) .unwrap(); @@ -2917,7 +2930,7 @@ mod tests { // Lookup with unbound value vm.push_goal(Goal::Lookup { dict, - field: term!(call!("x")), + field: term!(string!("x")), value: term!(sym!("y")), }) .unwrap(); diff --git a/polar-core/tests/integration_tests.rs b/polar-core/tests/integration_tests.rs index b929dcbe89..2ee5dc567c 100644 --- a/polar-core/tests/integration_tests.rs +++ b/polar-core/tests/integration_tests.rs @@ -5,7 +5,7 @@ use maplit::btreemap; use permute::permute; use std::cell::RefCell; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::iter::FromIterator; use polar_core::{ @@ -22,7 +22,13 @@ use polar_core::{ type QueryResults = Vec<(HashMap, Option)>; use mock_externals::MockExternal; -fn no_results(_: u64, _: Term, _: Symbol, _: Option>) -> Option { +fn no_results( + _: u64, + _: Term, + _: Symbol, + _: Option>, + _: Option>, +) -> Option { None } @@ -54,7 +60,7 @@ fn query_results( mut message_handler: K, ) -> QueryResults where - F: FnMut(u64, Term, Symbol, Option>) -> Option, + F: FnMut(u64, Term, Symbol, Option>, Option>) -> Option, G: FnMut(&str) -> String, H: FnMut(u64, Term), I: FnMut(Term, Symbol) -> bool, @@ -83,11 +89,12 @@ where instance, attribute, args, + kwargs, } => { query .call_result( call_id, - external_call_handler(call_id, instance, attribute, args), + external_call_handler(call_id, instance, attribute, args, kwargs), ) .unwrap(); } @@ -170,7 +177,7 @@ fn query_results_with_externals(query: Query) -> (QueryResults, MockExternal) { ( query_results( query, - |a, b, c, d| mock.borrow_mut().external_call(a, b, c, d), + |a, b, c, d, e| mock.borrow_mut().external_call(a, b, c, d, e), |a, b| mock.borrow_mut().make_external(a, b), |a, b| mock.borrow_mut().external_isa(a, b), |a, b, c| mock.borrow_mut().external_is_subspecializer(a, b, c), @@ -204,7 +211,7 @@ fn qext(polar: &mut Polar, query_str: &str, external_results: Vec) -> Que .rev() .collect(); let query = polar.new_query(query_str, false).unwrap(); - query_results!(query, |_, _, _, _| external_results.pop()) + query_results!(query, |_, _, _, _, _| external_results.pop()) } #[track_caller] @@ -537,7 +544,7 @@ fn test_instance_lookup() { // Q: Not sure if this should be allowed? I can't get (new a{x: 1}).x to parse, but that might // be the only thing we should permit assert_eq!( - qext(&mut polar, "new a{x: 1}.x = 1", vec![value!(1)]).len(), + qext(&mut polar, "new a(x: 1).x = 1", vec![value!(1)]).len(), 1 ); } @@ -638,18 +645,18 @@ fn test_dict_specializers() { assert!(qnull(&mut polar, "g({y: 1})")); // Test unifying & isa-ing instances against our rules. - assert!(qnull(&mut polar, "f(new a{x: 1})")); + assert!(qnull(&mut polar, "f(new a(x: 1))")); assert_eq!( - qext(&mut polar, "g(new a{x: 1})", vec![value!(1), value!(1)]).len(), + qext(&mut polar, "g(new a(x: 1))", vec![value!(1), value!(1)]).len(), 1 ); - assert!(qnull(&mut polar, "f(new a{})")); - assert!(qnull(&mut polar, "f(new a{x: {}})")); - assert!(qext(&mut polar, "g(new a{x: 2})", vec![value!(2), value!(2)]).is_empty()); + assert!(qnull(&mut polar, "f(new a())")); + assert!(qnull(&mut polar, "f(new a(x: {}))")); + assert!(qext(&mut polar, "g(new a(x: 2))", vec![value!(2), value!(2)]).is_empty()); assert_eq!( qext( &mut polar, - "g(new a{y: 2, x: 1})", + "g(new a(y: 2, x: 1))", vec![value!(1), value!(1)] ) .len(), @@ -693,11 +700,11 @@ fn test_bindings() { fn test_lookup_derefs() { let polar = Polar::new(); polar - .load_str("f(x) if x = y and g(y); g(y) if new Foo{}.get(y) = y;") + .load_str("f(x) if x = y and g(y); g(y) if new Foo().get(y) = y;") .unwrap(); let query = polar.new_query("f(1)", false).unwrap(); let mut foo_lookups = vec![term!(1)]; - let mock_foo = |_, _, _, args: Option>| { + let mock_foo = |_, _, _, args: Option>, _| { // check the argument is bound to an integer assert!(matches!(args.unwrap()[0].value(), Value::Number(_))); foo_lookups.pop() @@ -707,7 +714,7 @@ fn test_lookup_derefs() { assert_eq!(results.len(), 1); let mut foo_lookups = vec![term!(1)]; - let mock_foo = |_, _, _, args: Option>| { + let mock_foo = |_, _, _, args: Option>, _| { assert!(matches!(args.unwrap()[0].value(), Value::Number(_))); foo_lookups.pop() }; @@ -753,38 +760,52 @@ fn test_load_str_with_query() { } } +/// Test using a constructor with positional + kwargs. #[test] -fn test_externals_instantiated() { +fn test_make_external() { let polar = Polar::new(); - polar.register_constant(sym!("Foo"), term!(true)); - polar - .load_str("f(x, foo: Foo) if foo.bar(new Bar{x: x}) = 1;") + let query = polar + .new_query("x = new Bar(1, a: 2, b: 3)", false) .unwrap(); - let mut foo_lookups = vec![term!(1)]; - let mock_foo = |_, _, _, args: Option>| { - // make sure that what we get as input is an external instance - // with the fields set correctly - match &args.as_ref().unwrap()[0].value() { - Value::ExternalInstance(ExternalInstance { - constructor: Some(ref term), - .. - }) => assert!( - matches!(term.value(), Value::InstanceLiteral(InstanceLiteral { - ref tag, ref fields - }) if tag.0 == "Bar" && fields.fields == btreemap!{sym!("x") => term!(1)}), - "expected external instance Bar {{ x: 1 }}, found: {}", - args.unwrap()[0].value().to_polar() - ), - _ => panic!("Expected external instance"), - } - foo_lookups.pop() + let mock_make_bar = |_, constructor: Term| match constructor.value() { + Value::Call(Call { + name, + args, + kwargs: Some(kwargs), + }) if name == &sym!("Bar") + && args == &vec![term!(1)] + && kwargs == &btreemap! {sym!("a") => term!(2), sym!("b") => term!(3)} => {} + _ => panic!("Expected call with args and kwargs"), }; - let query = polar.new_query("f(1, new Foo{})", false).unwrap(); - let results = query_results!(query, mock_foo); + let results = query_results!(query, no_results, mock_make_bar, no_debug); assert_eq!(results.len(), 1); } +/// Test external call with positional + kwargs. +#[test] +fn test_external_call() { + let polar = Polar::new(); + polar.register_constant(sym!("Foo"), term!(true)); + let mut foo_lookups = vec![term!(1)]; + + let query = polar + .new_query("(new Foo()).bar(1, a: 2, b: 3) = 1", false) + .unwrap(); + + let mock_foo_lookup = + |_, _, _, args: Option>, kwargs: Option>| { + assert_eq!(args.unwrap()[0], term!(1)); + assert_eq!( + kwargs.unwrap(), + btreemap! {sym!("a") => term!(2), sym!("b") => term!(3)} + ); + foo_lookups.pop() + }; + let results = query_results!(query, mock_foo_lookup); + assert_eq!(results.len(), 1); + assert!(foo_lookups.is_empty()); +} #[test] #[ignore] // ignore because this take a LONG time (could consider lowering the goal limit) #[should_panic(expected = "Goal count exceeded! MAX_EXECUTED_GOALS = 10000")] @@ -1257,21 +1278,21 @@ fn test_unify_rule_head() { assert!(matches!( polar - .load_str("f(new Foo{a: Foo{a: 1}});") + .load_str("f(new Foo(a: Foo{a: 1}));") .expect_err("Must have a parser error"), PolarError { kind: ErrorKind::Parse(_), .. } )); assert!(matches!( polar - .load_str("f(x: new Foo{a: 1});") + .load_str("f(x: new Foo(a: 1));") .expect_err("Must have a parser error"), PolarError { kind: ErrorKind::Parse(_), .. } )); assert!(matches!( polar - .load_str("f(x: Foo{a: new Foo{a: 1}});") + .load_str("f(x: Foo{a: new Foo(a: 1)});") .expect_err("Must have a parser error"), PolarError { kind: ErrorKind::Parse(_), .. } )); @@ -1282,12 +1303,12 @@ fn test_unify_rule_head() { .load_str("g(_: Foo{a: Foo{a: 1}}, x) if x = 1;") .unwrap(); - let query = polar.new_query("f(new Foo{a: 1}, x)", false).unwrap(); + let query = polar.new_query("f(new Foo(a: 1), x)", false).unwrap(); let (results, _externals) = query_results_with_externals(query); assert_eq!(results[0].0[&sym!("x")], value!(1)); let query = polar - .new_query("g(new Foo{a: new Foo{a: 1}}, x)", false) + .new_query("g(new Foo(a: new Foo(a: 1)), x)", false) .unwrap(); let (results, _externals) = query_results_with_externals(query); assert_eq!(results[0].0[&sym!("x")], value!(1)); @@ -1557,11 +1578,11 @@ fn test_external_unify() { let polar = Polar::new(); polar.load_str("selfEq(x) if eq(x, x); eq(x, x);").unwrap(); - let query = polar.new_query("selfEq(new Foo{})", false).unwrap(); + let query = polar.new_query("selfEq(new Foo())", false).unwrap(); let (results, _externals) = query_results_with_externals(query); assert_eq!(results.len(), 1); - let query = polar.new_query("eq(new Foo{}, new Foo{})", false).unwrap(); + let query = polar.new_query("eq(new Foo(), new Foo())", false).unwrap(); let (results, _externals) = query_results_with_externals(query); assert!(results.is_empty()); } diff --git a/polar-core/tests/mock_externals.rs b/polar-core/tests/mock_externals.rs index 7077aeb177..9b8088dc1f 100644 --- a/polar-core/tests/mock_externals.rs +++ b/polar-core/tests/mock_externals.rs @@ -1,5 +1,5 @@ /// Utils for mocking externals in tests. -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use polar_core::terms::{ExternalInstance, Symbol, Term, Value}; @@ -29,8 +29,12 @@ impl MockExternal { instance: Term, attribute: Symbol, args: Option>, + kwargs: Option>, ) -> Option { - assert!(args.is_none(), "Only support field lookups."); + assert!( + args.is_none() && kwargs.is_none(), + "Only support field lookups." + ); if self.calls.remove(&call_id) { // Calls only return one result, so we have none if the call is in progress. @@ -43,8 +47,8 @@ impl MockExternal { _ => panic!("expected external instance"), }; match self.get_external(instance_id) { - Value::InstanceLiteral(literal) => literal.fields.fields.get(&attribute).cloned(), - _ => panic!("expected instance literal"), + Value::Call(call) => call.kwargs.clone().unwrap().get(&attribute).cloned(), + _ => panic!("expected call with kwargs"), } } @@ -56,8 +60,8 @@ impl MockExternal { // True if class tags match if let Value::ExternalInstance(ExternalInstance { instance_id, .. }) = instance.value() { match self.get_external(*instance_id) { - Value::InstanceLiteral(literal) => literal.tag == class_tag, - _ => panic!("expected instance literal"), + Value::Call(call) => call.name == class_tag, + _ => panic!("expected call"), } } else { false diff --git a/polar-core/tests/serialize.rs b/polar-core/tests/serialize.rs index 2fe8b9f22b..9d4956599d 100644 --- a/polar-core/tests/serialize.rs +++ b/polar-core/tests/serialize.rs @@ -25,6 +25,7 @@ mod tests { Term::new_from_test(value!(0)), Term::new_from_test(value!("hello")), ]), + kwargs: None, }; eprintln!("{}", serde_json::to_string(&event).unwrap()); let term = Term::new_from_test(value!(1)); @@ -35,13 +36,17 @@ mod tests { Symbol::new("world"), Term::new_from_test(Value::String("something".to_owned())), ); - let literal = InstanceLiteral { - tag: Symbol::new("Foo"), - fields: Dictionary { fields }, + let constructor = Call { + name: Symbol::new("Foo"), + args: vec![ + term!(1234), + Term::new_from_test(Value::String("something".to_owned())), + ], + kwargs: Some(fields), }; let event = QueryEvent::MakeExternal { instance_id: 12345, - constructor: Term::new_from_test(Value::InstanceLiteral(literal)), + constructor: Term::new_from_test(Value::Call(constructor)), }; eprintln!("{}", serde_json::to_string(&event).unwrap()); let external = Term::new_from_test(Value::ExternalInstance(ExternalInstance {