clj-kondo support? - Githubissues

sogaiu commented 3 years ago

I'm noticing that clj-kondo is giving warnings about certain situations, e.g.

import-python makes a number of things available (e.g. python.dict)
require-python can make a new namespace

for these clj-kondo gives me "unresolved ..." types of messages.

IIUC, clj-kondo sprouted a hooks api recently: https://github.com/borkdude/clj-kondo/blob/master/doc/config.md#hooks (https://blog.michielborkent.nl/2020/06/21/clj-kondo-hooks/)

Hooks are a way to enhance linting via user provided code.

If some of the constructs in libpython-clj are relatively stable, may be it would make sense to try using the hooks api toward improved clj-kondo support.

I've tried my hand at this once and there are examples here: https://github.com/borkdude/clj-kondo/tree/master/examples

If the constructs are still relatively in flux, may be it is too early for this, OTOH, if we can start making a list of relevant constructs, we might be able to get a sense of what's involved.

So, here is the start of a candidate list with some details:

def-pylib-fn
- similar to defn
- draft: https://gist.github.com/sogaiu/b6aa01efbd4006bbcab8decf2d5889aa https://github.com/clj-python/libpython-clj/blob/69c4c0c11098e7352aed9d48d606ab402525af11/src/libpython_clj/jna/base.clj#L88-L106
import-python
- introduces python.list
- introduces python.dict
- introduces python.set
- introduces python.tuple
- introduces python.frozenset
- introduces python.str
- introduces python https://github.com/clj-python/libpython-clj/blob/69c4c0c11098e7352aed9d48d606ab402525af11/src/libpython_clj/require.clj#L289-L302
require-python
- can introduce namespace(s) https://github.com/clj-python/libpython-clj/blob/69c4c0c11098e7352aed9d48d606ab402525af11/src/libpython_clj/require.clj#L179-L286

In my experiments with def-pylib-fn I found that though the warnings went away with the hook in place, the actual usages did not seem to be recorded appropriately. I'm not quite sure why this is, but hope to investigate more. (One reason the usage being recorded is of some relevance is if one makes use of clj-kondo's analysis results in tooling, it can affect the outcome -- here is an example tool that does this: https://github.com/sogaiu/alc.index-defs and also some info on the analysis mode: https://github.com/borkdude/clj-kondo/tree/master/analysis)

Also, though not part of libpython-clj, the following is used by it and possibly it would be worth considering:

tech.parallel.utils/export-symbols

similar in flavor to potemkin's import-vars https://github.com/techascent/tech.parallel/blob/3d25ebe0e7260a4ebcc9ae48167c6a0937212ddd/src/tech/parallel/utils.clj#L4-L27
import-vars https://github.com/aleph-io/potemkin/blob/729f3bcf1cbc22e70e8d574bc2b6ebf3af6f5208/src/potemkin/namespaces.clj#L77-L103
clj-kondo hook for import-vars-with-mods https://github.com/lread/rewrite-cljc-playground/blob/d40abc7b43ba8fa748b20003d374d3b029c128ee/.clj-kondo/hooks/rewrite_cljc/potemkin/import_vars_with_mods.clj)

sogaiu commented 3 years ago

I have a draft for export-symbols: https://gist.github.com/sogaiu/ad05cde3fd2529c11949903ca5993301

To test it I rooted the files and directories in the project directory of libpython-clj. The config.edn content can be merged with whatever is already there in a straight-forward fashion. It seems to work along with the draft for def-pylib-fn: https://gist.github.com/sogaiu/b6aa01efbd4006bbcab8decf2d5889aa

sogaiu commented 3 years ago

One technique I learned from borkdude [1] is to not necessarily create a representation with rewrite-clj nodes that exactly matches what a macro might expand to but rather to construct something that gives clj-kondo sufficient information to do its job.

For example, in def-pylib-fn, rettype and docstring are both ignored. docstring could be used I suppose but that shouldn't be hard to add later if necessary.

[1] Also mentioned in docs:

It is not important that the code is rewritten exactly according to the macroexpansion. What counts is that the transformation rewrites into code that clj-kondo can understand.

jjtolton commented 3 years ago

@cnuernber I'll tackle this if you're interested in supporting clj-kondo

cnuernber commented 3 years ago

Oh for sure, let's do it. Kondo is a great tool, it usually makes the code better :-).

sogaiu commented 3 years ago

IIUC, it's not currently possible to support what import-python does in clj-kondo. However, I've talked a bit with borkdude about it so perhaps before long it may become possible. For reference, here are 2 related issues I filed after discussion:

sogaiu commented 3 years ago

Changes to address the issues mentioned in the previous comment:

have been made to the master branch of clj-kondo:

I intend to take a look at whether import-python can now be supported, but if anyone else feels like making an attempt that's fine by me :)

sogaiu commented 3 years ago

Here is preliminary support for import-python:

Assuming the following tree under libpython-clj's project directory:

.clj-kondo/
├── config.edn
└── hooks
    └── libpython_clj
         └── require
             └── import_python.clj

config.edn

{
 :hooks {:analyze-call {libpython-clj.require/import-python
                        hooks.libpython-clj.require.import-python/import-python}}
}

hooks/libpython_clj/require/import_python.clj

(ns hooks.libpython-clj.require.import-python
  "The import-python macro from libpython-clj/require.clj"
  (:require [clj-kondo.hooks-api :as api]))

(defn make-require
  [ns-sym alias-sym]
  (api/list-node
   [(api/token-node 'require)
    (api/list-node
     [(api/token-node 'quote)
      (api/vector-node
       [(api/token-node ns-sym)
        (api/keyword-node :as)
        (api/token-node alias-sym)])])]))

(defn import-python
  "Macro in libpython-clj/require.clj.

  Example call:

    (import-python)

  May be treating it as:

   (do
     (require (quote [builtins.list :as python.list]))
     (require (quote [builtins.dict :as python.dict]))
     ,,,
     )

  "
  [{:keys [:node]}]
  (let [new-node
        (with-meta (api/list-node
                    [(api/token-node 'do)
                     (make-require 'builtins.list 'python.list)
                     (make-require 'builtins.dict 'python.dict)
                     (make-require 'builtins.set 'python.set)
                     (make-require 'builtins.tuple 'python.tuple)
                     (make-require 'builtins.frozenset 'python.frozenset)
                     (make-require 'builtins.str 'python.str)
                     (make-require 'builtins 'python)])
                   (meta node))]
    ;; XXX: uncomment following and run clj-kondo on cl_format.clj to debug
    ;;(prn (api/sexpr node))
    ;;(prn (api/sexpr new-node))
    {:node new-node}))

To test, run clj-kondo --lint path.clj where path.clj has import-python available in it and uses it.

At the moment, there will likely be warnings about unused namespaces, but this may be something we can add a configuration for in config.edn.

Probably need a build based on the latest from the master branch of clj-kondo -- at least https://github.com/borkdude/clj-kondo/commit/6360b3cf5a3165d6f611a32eeb32fbec341e5e71 or later.

sogaiu commented 3 years ago

Adding:

 :linters {:unused-namespace {:exclude [builtins.list
                                        builtins.dict
                                        builtins.set
                                        builtins.tuple
                                        builtins.frozenset
                                        builtins.str
                                        builtins]}}

to config.edn seems to address the warnings issue.

sogaiu commented 3 years ago

FWIW, I believe the latest release of clj-kondo now has the necessary features to try things out.

cnuernber commented 3 years ago

@sogaiu - I think we need to update clj-kondo support for v2.

sogaiu commented 3 years ago

@cnuernber I've looked into this a bit and have started work here: https://github.com/sogaiu/libpython-clj/commit/0d84359b9986007d017275ec56bad9aa6e4a9ad0

That seems to work on simple code. May be @jjtolton could give it a try at some point?

Is it correct that there is now no longer anything akin to libpython-clj/jna/base.clj? Specifically, I don't see something likedef-pylib-fn, so may be that portion of the hooks can just be removed?

cnuernber commented 3 years ago

Yes, there is no def-pylib-fn. The way it works now is that this datastructure is used to dynamically generate bindings by the FFI driver (either jna direct mapping or jdk-16's ffi pathway) and that is loaded. A library instance, when dereferenced, produces map of fn-name to clojure.lang.IFn . Then we generate the low level binding functions directly from the initial function definitions taking care of converting/releasing strings when necessary.

I didn't know if perhaps that pathway would cause issues with clj-kondo but apparently not.

sogaiu commented 3 years ago

Thanks for the explanation.

It may already be clear, but just to spell it out, the work-in-progress branch (https://github.com/sogaiu/libpython-clj/commit/0d84359b9986007d017275ec56bad9aa6e4a9ad0), no longer references def-pylib-fn and it seems to work in simple testing.

cnuernber commented 2 years ago

Closing this issue as it has stalled. The best solution to a lot of this is to wrap any namespace with macros with export-symbols/write-api! to produce a new namespace and use that which will work not only with clj-kondo but with Calva and lots of other systems.

clj-python / libpython-clj

clj-kondo support? #124