Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support keyword application #85

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: support keyword application #85

wants to merge 1 commit into from

Conversation

phrohdoh
Copy link
Contributor

@phrohdoh phrohdoh commented Jul 8, 2021

Similar to #84.

(:k {:k "v"})               ;; => "v"
(:k ((fn [] {:k "v"})))     ;; => "v"
(:k)                        ;; => arity error
(:k a b c)                  ;; => arity error
(:k {} b)                   ;; => nil (should be b, see todo)
(:k "does not eval to map") ;; => nil
(:x {:k "v"})               ;; => nil
$ cargo run -- -e '(:k {:k "v"})'
v

$ cargo run -- -e '(:k ((fn [] {:k "v"})))'
v

$ cargo run -- -e '(:k)'
#Condition["Wrong number of arguments given to function (Given: 0, Expected: [1, 2])"]

$ cargo run -- -e '(:k a b c)'
#Condition["Wrong number of arguments given to function (Given: 3, Expected: [1, 2])"]

$ cargo run -- -e '(:k {} b)'
nil

$ cargo run -- -e '(:k "does not eval to map")'
nil

$ cargo run -- -e '(:x {:k "v"})'
nil
$ cargo t apply_keyword
    Finished test [unoptimized + debuginfo] target(s) in 5.57s
     Running target/debug/deps/rust_clojure-9f49d835809a6228

running 7 tests
test value::tests::apply_keyword_to_map ... ok
test value::tests::apply_keyword_evals_arg ... ok
test value::tests::apply_keyword_too_few_args ... ok
test value::tests::apply_keyword_too_many_args ... ok
test value::tests::apply_keyword_default ... ignored
test value::tests::apply_keyword_to_non_map ... ok
test value::tests::apply_keyword_to_map_without_key ... ok

test result: ok. 6 passed; 0 failed; 1 ignored; 0 measured; 188 filtered out

@phrohdoh
Copy link
Contributor Author

phrohdoh commented Jul 8, 2021

(:k) being an arity error seems to conflict with (ns my.ns (:gen-class)).
Since ns doesn't (shouldn't, not sure if this is currently the case in ClojureRS) eval its first arg (the symbol declaring the namespace) we may need to handle that case in a special way, or make keyword application with 0 args not an error, somehow.

@phrohdoh
Copy link
Contributor Author

phrohdoh commented Jul 8, 2021

(:k) being an arity error seems to conflict with (ns my.ns (:gen-class)).
Since ns doesn't (shouldn't, not sure if this is currently the case in ClojureRS) eval its args we may need to handle that case in a special way, or make keyword application with 0 args not an error, somehow.

The culprit here seems to be our current implementation of IFn for the ns macro:

impl IFn for NsMacro {
    fn invoke(&self, args: Vec<Rc<Value>>) -> Value {
        if args.len() != 1 {
            return error_message::wrong_arg_count(1, args.len());
        }

        let namespace = args.get(0).unwrap();
        match &**namespace {
            Value::Symbol(sym) => {
                self.enclosing_environment.change_or_create_namespace(sym);
                Value::Nil
            }
            _ => error_message::type_mismatch(TypeTag::Symbol, &**namespace),
        }
    }
}

Specifically, the args.len() != 1 check and subsequent error.

See https://clojure.github.io/clojure/clojure.core-api.html#clojure.core/ns, reproduced below, for valid ns forms.

Usage: (ns name docstring? attr-map? references*)
Sets *ns* to the namespace named by name (unevaluated), creating it
if needed.  references can be zero or more of: (:refer-clojure ...)
(:require ...) (:use ...) (:import ...) (:load ...) (:gen-class)
with the syntax of refer-clojure/require/use/import/load/gen-class
respectively, except the arguments are unevaluated and need not be
quoted. (:gen-class ...), when supplied, defaults to :name
corresponding to the ns name, :main true, :impl-ns same as ns, and
:init-impl-ns true. All options of gen-class are
supported. The :gen-class directive is ignored when not
compiling. If :gen-class is not supplied, when compiled only an
nsname__init.class will be generated. If :refer-clojure is not used, a
default (refer 'clojure.core) is used.  Use of ns is preferred to
individual calls to in-ns/require/use/import:

(ns foo.bar
  (:refer-clojure :exclude [ancestors printf])
  (:require (clojure.contrib sql combinatorics))
  (:use (my.lib this that))
  (:import (java.util Date Timer Random)
           (java.sql Connection Statement)))
Specs:
  Args: (cat
         :ns-name simple-symbol?
         :docstring (? string?)
         :attr-map (? map?)
         :ns-clauses :clojure.core.specs.alpha/ns-clauses)
  Ret:  any?

@phrohdoh
Copy link
Contributor Author

phrohdoh commented Jul 14, 2021

Per https://clojure.org/reference/data_structures#Keywords

Keywords implement IFn for invoke of one argument (a map) with an optional second argument (a default value). For example (:mykey my-hash-map :none) means the same as (get my-hash-map :mykey :none). See get.

so (:k a b) must not be an arity error, instead (the evaluated value of) b must be the default value (if a, a PersistentListMap which implements IPersistentMap, does not contain the key :k).

IPersistentMap's current get function has the following signature:

fn get(&self, key: &Rc<Value>) -> Rc<Value>;

Which does not allow for a default value.

We could hardcode/specialize keyword application to support a default, or wait until IPersistentMap::get supports a default Value before updating keyword application to match the above text from the Clojure reference.

    (:k {:k "v"})               ;; => "v"
    (:k ((fn [] {:k "v"})))     ;; => "v"
    (:k)                        ;; => arity error
    (:k a b c)                  ;; => arity error
    (:k {} b)                   ;; => nil (should be b, see todo)
    (:k "does not eval to map") ;; => nil
    (:x {:k "v"})               ;; => nil
@phrohdoh
Copy link
Contributor Author

Current HEAD of this PR's branch at time of writing this comment (65484688) prepares keyword application for IPersistentMap::get support of a default value.

wait until IPersistentMap::get supports a default Value ... to match ... the Clojure reference.

So will return nil (Value::Nil) when the default value ought to be returned.

let map = evaled_arg_values.get(0).unwrap().as_ref();
// @TODO IPersistentMap::get to support optional default value
let _default = evaled_arg_values.get(1).map(|x| x.as_ref());

Some(match map {
    Value::PersistentListMap(map) => map.get(&keyword.to_rc_value()),
    _ => Rc::new(Value::Nil),
})

The following (which is the map.get(...) above) remains unchanged.

impl IPersistentMap for PersistentListMap {
    fn get(&self, key: &Rc<Value>) -> Rc<Value> {
        match self {
            PersistentListMap::Map(parent, entry) => {
                if entry.key == *key {
                    return Rc::clone(&entry.val);
                }
                parent.get(key) // @TODO if nil return default
            }
            PersistentListMap::Empty => Rc::new(Value::Nil),
        }
    }
}

@phrohdoh
Copy link
Contributor Author

phrohdoh commented Jul 14, 2021

#87 proposes an implementation of clojure.core/get arity 3, which would allow keyword application to return a default value when applicable:

(:k {} :not-found)      ;; => :not-found
(:k {:x :v} :not-found) ;; => :not-found

Instead of the above-documented, as-of-65484688:

(:k {} b) ;; => nil (should be b, see todo)
(:k {} :not-found)      ;; => nil
(:k {:k :v} :not-found) ;; => nil

If #87 is merged (or some other implementation!) prior to this (assuming this is), I will update this PR to incorporate those changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant