basilisp-lang / basilisp

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

error defining symbols with no metadata in macros #850

Closed ikappaki closed 8 months ago

ikappaki commented 9 months ago

basilisp throws an error when trying to define a symbol in a macro using syntax quoting: AttributeError: 'NoneType' object has no attribute 'update'

To reproduce

  1. Open up the REPl and create a simple macro that returns a def whose symbol comes from evaluation rather than being constant
    basilisp.user=> (defmacro abc [] `(def ~(symbol "xyz") 3))
    #'basilisp.user/abc
  2. Call the macro, it throws an error
    basilisp.user=> (abc)
    Traceback (most recent call last):
    File "C:\src\basilisp\src\basilisp\lang\compiler\analyzer.py", line 2510, in _invoke_ast
    return __handle_macroexpanded_ast(form, expanded, ctx)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "C:\src\basilisp\src\basilisp\lang\compiler\analyzer.py", line 2455, in __handle_macroexpanded_ast
    expanded_ast = _analyze_form(expanded, ctx)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "D:\local\Python312\Lib\functools.py", line 909, in wrapper
    return dispatch(args[0].__class__)(*args, **kw)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "C:\src\basilisp\src\basilisp\lang\compiler\analyzer.py", line 712, in _analyze_form
    return cast(T_node, f(form, ctx).fix_missing_locations(form_loc))
                        ^^^^^^^^^^^^
    File "C:\src\basilisp\src\basilisp\lang\compiler\analyzer.py", line 3250, in _list_node
    return handle_special_form(form, ctx)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "C:\src\basilisp\src\basilisp\lang\compiler\analyzer.py", line 959, in _def_ast
    name.meta.update(  # type: ignore [union-attr]
    ^^^^^^^^^^^^^^^^
    AttributeError: 'NoneType' object has no attribute 'update'

The same works in Clojure

user> (defmacro abc [] `(def ~(symbol "xyz") 3))
#'user/abc
user> (abc)
#'user/xyz
user> xyz
3

Looking at the analyzer code where the exception is thrown, I am not sure what the correct solution is (should there be a check if name comes with metadata before trying to update it? should the symbol have some metadata attached to it to begin with and thus the issue lies elsewhere?).

    # Attach metadata relevant for the current process below.
    #
    # The reader line/col metadata will be attached to the form itself in the
    # happy path case (top-level bare def or top-level def returned from a
    # macro). Less commonly, we may have to rely on metadata attached to the
    # def symbol if, for example, the def form is returned by a macro wrapped
    # in a do or let form. In that case, the macroexpansion process won't have
    # any metadata to pass along to the expanded form, but the symbol itself
    # is likely to have metadata. In rare cases, we may not be able to get
    # any metadata. This may happen if the form and name were both generated
    # programmatically.
    def_loc = _loc(form) or _loc(name) or (None, None)
    if def_loc == (None, None):
        logger.warning(f"def line and column metadata not provided for Var {name}")
    def_node_env = ctx.get_node_env(pos=ctx.syntax_position)
    def_meta = _clean_meta(
        name.meta.update(  # type: ignore [union-attr]
            lmap.map(
                {
                    COL_KW: def_loc[1],
                    FILE_KW: def_node_env.file,
                    LINE_KW: def_loc[0],
                    NAME_KW: name,
                    NS_KW: current_ns,
                }
            )
        )
    )
    assert def_meta is not None, "def metadata must be defined at this point"

Thanks

chrisrink10 commented 8 months ago

The fix would likely be this:

if name.meta is None:
    name = name.with_meta(lmap.EMPTY)

...though I am not convinced even if Clojure allows this anyone should be doing this. def names should come from code given by users (whose meta should already exist or can be copied via with-meta in the macro definition).