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

[analysis] Missing locals analysis of deftype args #1612

Closed
ericdallo opened this issue Mar 13, 2022 · 6 comments
Closed

[analysis] Missing locals analysis of deftype args #1612

ericdallo opened this issue Mar 13, 2022 · 6 comments

Comments

@ericdallo
Copy link
Member

version

2022.03.09

platform

JVM

editor

clojure-lsp

problem

there are missing locals analysis for bar in the following code:

(deftype Foo [bar] ;; missing analysis for bar
  Object
  (toString [_this]
    bar))

repro

clj -Sdeps '{:deps {clj-kondo/clj-kondo {:mvn/version "2022.03.09"}}}' -m clj-kondo.main --lint - --config '{:output {:analysis {:locals true} :format :json}}' <<< '(deftype Foo [bar] Object (toString [_this] bar))' | jq .analysis.local

Output

[
  {
    "end-row": 1,
    "scope-end-row": 1,
    "name": "_this",
    "scope-end-col": 49,
    "filename": "<stdin>",
    "str": "_this",
    "col": 38,
    "id": 3,
    "end-col": 43,
    "row": 1
  }
]

expected behavior

Expect bar to be available on locals

Note that bar local-usage is already available, missing only the locals one

@borkdude
Copy link
Member

I found the issue: the bindings are analyzed, but are not "registered" so they won't be flagged as unused. But this has the negative side effect that they also don't end up in analysis. We clearly need to separate these concerns.

@ericdallo
Copy link
Member Author

ericdallo commented Mar 14, 2022

any reason to not flag them as unused like all other function arities?

@borkdude
Copy link
Member

Yes, unused bindings must be renamed to be ignored: x -> _x. But renaming fields of a deftype or defrecord has the undesired effect that their APIs change.

borkdude added a commit that referenced this issue Mar 16, 2022
@borkdude
Copy link
Member

@ericdallo Fixed on master. Plz test :)

@ericdallo
Copy link
Member Author

Thanks! Will test

@ericdallo
Copy link
Member Author

Confimed this one fixed for clojure-lsp OOTB, thank you!

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

No branches or pull requests

2 participants