basilisp-lang / basilisp

A Clojure-compatible(-ish) Lisp dialect targeting Python 3.8+
https://basilisp.readthedocs.io
Eclipse Public License 1.0
260 stars 7 forks source link

grafeceful handling of vector destructuring to map keys #1090

Closed ikappaki closed 4 days ago

ikappaki commented 5 days ago

Hi,

attempting to desctructure a vector to map keys produces an error message/stack trace that does not reflect the cause of the error

TypeError: pvector indices must be integers, not Keyword

To reproduce, Open up the REPL and try to destructure a vector to map keys. A stacktrace/error message is produced "pvector indices must be integers" that is hard to link to the actual cause

> basilisp repl
basilisp.user=> (let [{:keys [a]} [:a 5]]
                  a)
Traceback (most recent call last):
  File "C:\src\basilisp\src\basilisp\cli.py", line 582, in repl
    result = eval_str(lsrc, ctx, ns, eof)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\src\basilisp\src\basilisp\cli.py", line 53, in eval_str
    last = compiler.compile_and_exec_form(form, ctx, ns)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\src\basilisp\src\basilisp\lang\compiler\__init__.py", line 211, in compile_and_exec_form
    exec(bytecode, ns.module.__dict__)  # pylint: disable=exec-used
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<REPL Input>", line 2, in <module>
  File "C:\src\basilisp\src\basilisp\core.lpy", line 2228, in get
    (defn get
  File "C:\src\basilisp\src\basilisp\core.lpy", line 2236, in get__arity2
    (basilisp.lang.runtime/get m k))
                           ^^^^^^^^^
  File "C:\local\Python311\Lib\functools.py", line 909, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\src\basilisp\src\basilisp\lang\runtime.py", line 1446, in _get_ilookup
    return m.val_at(k, default)
           ^^^^^^^^^^^^^^^^^^^^
  File "C:\src\basilisp\src\basilisp\lang\vector.py", line 204, in val_at
    return self._inner[k]
           ~~~~~~~~~~~^^^
TypeError: pvector indices must be integers, not Keyword

Although this is a simple example, the error message does not directly reflect the root cause which might take some time for a user to decipher. Even more, the error is reported at "line 2," where the variable a is referenced, rather than at "line 1," where the destructuring actually happens

In a large program where the destructuring happens over a variable that we can't really tell what its value is by just looking at it, the cause of the error is hard to trace.

I expect that, from a user's experience point of view, either a better error message should be thrown indicating that the destructuring is the issue, or nil should be used for the destructured key bindings.

I couldn't find something in Clojure documentation to indicate what the correct behavior should be for trying to destructuring non-map values. I believe is undefined behavior. However I did a survey of

what Clojure does

(let [{:keys [a]} nil]
  a)
;; => nil

(let [{:keys [a]} [:a 5]]
  a)
;; => nil

(let [{:keys [a]} '(:a 5)]
  a)
;; => 5

(let [{:keys [a]} "abc"]
  a)
;; => nil

(let [{:keys [a]} #{:a}]
  a)
;; => :a

(let [{:keys [a]} 5]
  a)
;; => Syntax error (UnsupportedOperationException) compiling at (src\destr.clj:20:1).
;;    Can't type hint a primitive local with a different type

(let [{:keys [a]} 5.3]
  a)
;; => Syntax error (UnsupportedOperationException) compiling at (src\destr.clj:25:1).
;;    Can't type hint a primitive local with a different type

and what Basilisp currently does

(let [{:keys [a]} nil]
  a)
;; => nil

(let [{:keys [a]} [:a 5]]
  a)
;; => TypeError: pvector indices must be integers, not Keyword

(let [{:keys [a]} '(:a 5)]
  a)
;; => nil

(let [{:keys [a]} "abc"]
  a)
;; => TypeError: string indices must be integers, not 'Keyword'

(let [{:keys [a]} #{:a }]
  a)
;; => :a

(let [{:keys [a]} 5]
  a)
;; => nil

(let [{:keys [a]} 5.3]
  a)
;; => nil

Notice in the above how collection are gracefully handled in Clojure, even though it throws an example on numerical values.

Thanks

chrisrink10 commented 4 days ago

Thanks for filing and for providing the examples above. I'm working on a PR to fix this.

I did notice something interesting from your examples:

(let [{:keys [a]} 5]
  a)
;; => Syntax error (UnsupportedOperationException) compiling at (src\destr.clj:20:1).
;;    Can't type hint a primitive local with a different type

(let [{:keys [a]} 5.3]
  a)
;; => Syntax error (UnsupportedOperationException) compiling at (src\destr.clj:25:1).
;;    Can't type hint a primitive local with a different type

(let [{:keys [a]} (Integer. 5)]
  a)
;; => nil

(let [{:keys [a]} 5N]
  a)
;; => nil

Notice how when we coerce the value to boxed and/or non-primitive values in the last examples Clojure returns nil rather than throwing the syntax exception. Those exceptions you showed may just be a peculiarity of the JVM and primitive types.

ikappaki commented 4 days ago

Notice how when we coerce the value to boxed and/or non-primitive values in the last examples Clojure returns nil rather than throwing the syntax exception. Those exceptions you showed may just be a peculiarity of the JVM and primitive types.

Thanks for looking into it. Good catch regarding primitive types. I couldn’t find a pattern, so I assumed the it was undefined behavior.