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

improve Index[View].getKnownUsers() #354

Merged
merged 2 commits into from
May 13, 2024

Conversation

Ladicek
Copy link
Collaborator

@Ladicek Ladicek commented Mar 4, 2024

@Ladicek Ladicek added this to the 3.1.7 milestone Mar 4, 2024
return new Index(masterAnnotations, subclasses, subinterfaces, implementors, classes, modules, users);
Map<DotName, List<ClassInfo>> userLists = new HashMap<>();
for (Map.Entry<DotName, Set<ClassInfo>> entry : users.entrySet()) {
userLists.put(entry.getKey(), new ArrayList<>(entry.getValue()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if List#copyOf() could be a little bit more efficient here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly, but Jandex is still on Java 8 :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry. I forgot about it 🤦

Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Looks good but I'm not 100% sure it's a good idea to target a bugfix release...

@Ladicek
Copy link
Collaborator Author

Ladicek commented Mar 4, 2024

I often add miniature features in micro releases, but you're probably right -- this should go into 3.2.0.

I'll change the milestone and make this a draft for now.

Copy link
Contributor

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

I suppose this is more general, and more useful than just appearing in the constant pool, although I suppose that in the first two use-cases we've had (calling static methods on Log and calling any method/field on Panache entities) it's now going to be possible to get "users" that "only" declare our types (Log or Panache entities) somewhere in their signatures without calling any method on them, which may be "false positives" in these cases.

In the case of Panache, we'll register a class visitor/transformer too many, but it won't do anything since it won't find any field access to transform. The chances of classes having signature references to Panache entities without using their fields is plausible.

In the case of Quarkus Log it may be that we define an extra static logger for a class that will go unused, but the chances of that happening is pretty unlikely.

@Ladicek
Copy link
Collaborator Author

Ladicek commented May 3, 2024

Agree. Thanks!

Previously, the `getKnownUsers()` method only considered a class
as used in another class when it occured in the list of class
references in the other class's constant pool.

With this commit, a class is also considered as used in another class
when it occurs in the other class's signature (superclass type,
superinterface types, type parameters, permitted subclasses),
in the signatures of the other class's methods (return type,
parameter types, exception types, type parameters), or in the types
of the other class's fields and record components.
@Ladicek Ladicek marked this pull request as ready for review May 13, 2024 12:50
@Ladicek Ladicek merged commit ddba5d4 into smallrye:main May 13, 2024
39 checks passed
@Ladicek Ladicek deleted the improve-known-users branch May 13, 2024 13:01
This pull request was closed.
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.

3 participants