review: feat: support to parse `CtType`, `CtClass` and `CtField` from JDK element CtPath.toString() by kaml8 · Pull Request #4989 · INRIA/spoon

kaml8 · 2022-10-30T08:56:45Z

feat(CtType,CtTypeImpl): add method getMethodBySignature(String signature).
feat(CtClass,CtClassImpl): add method getConstructorBySignature(String signature).
fix(CtPathStringBuilder): delete redundant ')' when get arguments for default constructor.
feat(CtPathImpl): add method getJdkClass.
feat(CtPathImpl): rewrite method evaluateOn, search for jdk elements when no root parameter is given.
docs(path.md): update doc

sonatype-lift · 2022-10-30T09:07:51Z

src/main/java/spoon/reflect/path/impl/CtPathImpl.java

+			}
+			if (filtered.isEmpty() && ctType != null) filtered.add(ctType);
+		}
 		return (List<T>) filtered;


💬 2 similar findings have been found in this PR

unchecked: unchecked cast

🔎 Expand here to view all instances of this finding

File Path Line Number

src/main/java/spoon/reflect/path/impl/CtPathImpl.java 38

src/main/java/spoon/reflect/path/impl/CtPathImpl.java 38

Visit the Lift Web Console to find more details in your report.

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

I-Al-Istannen

Thanks, I have a few remarks :)

src/test/java/spoon/test/path/PathTest.java

I-Al-Istannen · 2022-10-30T10:17:42Z

src/main/java/spoon/reflect/declaration/CtClass.java

+	 */
+	@DerivedProperty
+	@PropertyGetter(role = CONSTRUCTOR)
+	CtConstructor<T> getConstructorBySignature(String signature);


I don't quite see how this relates to the title and scope of this PR. This should be done in a separate PR IMHO.

As a CtConstructor has a CtPath ends with #constructor[signature=(int)], I need this function to get the CtElement.

I-Al-Istannen · 2022-10-30T10:18:06Z

src/main/java/spoon/reflect/declaration/CtType.java

+	 * @return null if does not exit
+	 */
+	@PropertyGetter(role = METHOD)
+	<R> CtMethod<R> getMethodBySignature(String signature);


I don't quite see how this relates to the title and scope of this PR. This should be done in a separate PR IMHO.

Same to constructor, a CtMethod has a CtPath ends with #method[signature=(...)], I need this function to get the CtElement.

I-Al-Istannen · 2022-10-30T10:21:25Z

src/main/java/spoon/reflect/path/impl/CtPathImpl.java

 		return (List<T>) filtered;
 	}

+	private Class<?> getJdkClass(String name) {


Note: This is duplicated in a few places in spoon

Yes, I found similar one: TypeFactory.get(final String qualifiedName).
However, this method doesn't support shadow elements while another reload method TypeFactory.get(Class<?> cl) does.

So the question is that we need a function that transform String to Class<?>, as the method above did,
or add extra logic in TypeFactory.get(final String qualifiedName) to process shadow elements intendly.

doc/path.md

src/main/java/spoon/reflect/path/impl/CtPathImpl.java

I-Al-Istannen · 2022-10-30T10:30:56Z

src/main/java/spoon/reflect/path/impl/CtPathImpl.java

+		if (filtered.isEmpty()) {
+			List<String> cls_name_list = new LinkedList<>();
+			CtType<?> ctType = null;
+			for (CtPathElement element : elements) {


I am not very well versed with the CtPath API, but this seems like it re-implements existing functionality and we should instead adjust the matching logic of the path elements?

Seeing issue #4984 , yeah I want to reuse the case that with no parameter for matching shadow elements.
The matching logic do differ, so it may also be a good idea to extract a new method evaluateOnShadowModel as you suggested.

I-Al-Istannen · 2022-10-30T10:32:16Z

src/main/java/spoon/reflect/path/CtPathStringBuilder.java

 			} else if ("]".equals(token) || ";".equals(token)) {
 				//finished reading of argument value
-				pathElement.addArgument(argName, argValue.toString());
+				//[fix bug]:AbstractPathElement.getArguments([constructor with no parameters])


I am not sure what this is doing here, I will need to have a look at it later. This also feels like it should be a different PR and have a test case :)

Yes it's a small bug in method AbstractPathElement.getArguments(), when a default constructor in, it produces redundant ).

sonatype-lift · 2022-10-31T01:51:30Z

src/main/java/spoon/support/reflect/declaration/CtClassImpl.java

+			if (!(typeMember instanceof CtConstructor)) {
+				continue;
+			}
+			CtConstructor<T> c = (CtConstructor<T>) typeMember;


unchecked: unchecked cast

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

kaml8 · 2022-10-31T02:40:10Z

@MartinWitt
Thanks for testing, Should I modify my code as suggested below?
Because I did't see any kinds of comment in origin branch either.

monperrus · 2025-11-08T07:04:52Z

@kaml8 what's the status here? thanks!

when calling `AbstractPathElement.getArguments([constructor with no parameters])` will produce redundant ')'

Test cases have passed. Now it supports to get JDK-element from CtPath(As String).

sonatype-lift bot reviewed Oct 30, 2022

View reviewed changes

I-Al-Istannen requested changes Oct 30, 2022

View reviewed changes

kaml8 force-pushed the fix-issue#4984 branch from 08d43a9 to 02adcd8 Compare October 31, 2022 01:18

sonatype-lift bot reviewed Oct 31, 2022

View reviewed changes

kaml8 requested a review from I-Al-Istannen October 31, 2022 02:01

kaml8 added 12 commits November 12, 2025 14:12

feat(CtType): add method getMethodBySignature

f749dad

feat(CtClass): add method getConstructorBySignature

d3c901b

fix(CtPathStringBuilder): delete redundant ')'

2636329

when calling `AbstractPathElement.getArguments([constructor with no parameters])` will produce redundant ')'

feat(CtPathImpl): add method getJdkClass

6e239d1

feat(CtPathImpl): rewrite method evaluateOn

905f897

Test cases have passed. Now it supports to get JDK-element from CtPath(As String).

docs(path.md): update doc

add1e0c

refactor(CtPathImpl): optimization code

7638075

refactor(PathTest): revert imports

cf37d52

docs(path.md): update doc as suggested

f8d17c2

refactor(CtPath): extract method evaluateOnShadowModel

e5d8e1f

refactor: adjust code style

e909f37

refactor: adjust codeStyle

a2e2090

monperrus force-pushed the fix-issue#4984 branch from 30c4b84 to a2e2090 Compare November 12, 2025 15:19

up

4a0e8ee

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Uh oh!

Conversation

kaml8 commented Oct 30, 2022

Uh oh!

sonatype-lift bot Oct 30, 2022

Choose a reason for hiding this comment

Uh oh!

I-Al-Istannen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonatype-lift bot Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

kaml8 commented Oct 31, 2022

Uh oh!

monperrus commented Nov 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants