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: parse java files via javaparser to produce java-member-definitions #2053

Merged

Conversation

ericdallo
Copy link
Member

Please answer the following questions and leave the below in as part of your PR.

Fixes #1983

@ericdallo ericdallo force-pushed the java-member-definitions-for-source branch from bc44a56 to b7e0bcd Compare April 16, 2023 19:49
:uri #"file:.*/corpus/java/sources/foo/bar/AwesomeClass.java"
:flags #{:public :static :field :final}
:name "bar3"
:type "Double"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this type be fully qualified, as it can refer to any class?

java.lang.Double

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure if java parser does that only for java.langs, but I couldn't think on how to get the full package there, any suggestions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that to work, javaparser need to resolve the symbol which is possible via its javaparser-symbol-solver lib, now it's properly working!

(condp = (type node)
FieldDeclaration :field
MethodDeclaration :method
ConstructorDeclaration :method))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this :method and not :constructor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the same rule we had at ASM, call it method and know it by the name, should we change it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From ASM eyes, a constructor is a method as well, we could infer that and make javaparser returns constructor as well, but maybe do in another PR, the way it's is consistent always returning method for both asm and javaparser

@borkdude
Copy link
Member

@ericdallo I'm afraid the resolver isn't very useful because it relies on runtime analysis.

You can see this when you add something like:

import dude.foobar.FooClazz;

    public static FooClazz dude() {
        return null;
    }

to AwesomeClass. This will fail with:

Unsolved symbol : FooClazz

The symbol solver stuff is not particularly "static analysis" but uses reflection / javassist and stuff like that, that probably won't be very useful to clj-kondo / clojure-lsp.

As this is a bigger change I think it's best to test-drive this locally for a while, before merging this into clj-kondo, so we can discover issues like the above.

@borkdude
Copy link
Member

Btw, adding the symbol solver stuff added another 3mb to the image size (increase in total is then 6mb).

@ericdallo
Copy link
Member Author

I see, I was afraid something like that could happen, there is the JavaTypeResolver class which you can pass a File to lookup for Java classes, maybe we could pass the root of the jar being analyzed or something, but I agree this make things complex.

I'm wondering how bad this is for clojure-lsp having only the simple class name instead of full package for :type and :return-type, maybe it won't be even necessary as we care about location and available method names, I 'll test locally and confirm

@ericdallo
Copy link
Member Author

ericdallo commented Apr 20, 2023

Good news @borkdude, in the end clojure-lsp won't care if :type and :return-type are full qualified and this info will be mostly used for hover and completion so having it as simple names is quite enough :)
I made one commit adding the :method-name to :java-class-usages so now we officially and finally can check docs of methods with clojure-lsp's improve-java-analysis branch 🙌🏻

With that, we could fix clojure-lsp/clojure-lsp#204 for static classes, for other classes I think we would need to do the same for instance-invocations, but provide the class-name, not sure how hard would be tho

image

@borkdude borkdude merged commit 6ccd4f1 into clj-kondo:master May 17, 2023
4 of 9 checks passed
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.

analysis: java-method-definitions support
2 participants