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

Support finding descriptors for java statics #171

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

yigit
Copy link
Collaborator

@yigit yigit commented Nov 25, 2020

No description provided.

This PR fixes a bug where static java declarations were not being resolved
properly because we would only look at the main scope of the class declaration
but we also need to check the static scope.

I've also refactored resolveJavaDeclaration to move common code to helpers.

Fixes: google#170
@yigit
Copy link
Collaborator Author

yigit commented Nov 25, 2020

ugh i was passing jdk 11 path for jdk 9 and seems like it made the function visible after this fix.
with jdk 9 it does not show up, will revert the test change

Copy link
Contributor

@neetopia neetopia left a comment

Choose a reason for hiding this comment

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

LGTM, only have 2 minor comments.

.resolveClass(JavaMethodImpl(psi).containingClass)
?.findEnclosedDescriptor(
kindFilter = DescriptorKindFilter.FUNCTIONS,
psi = psi
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks no difference from just pass the filter lambda at this call site.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea you are right, i first had the psi version then added the filter version hence this unnecessary duplication.
removed the psi version to always require the lambda.

kindFilter: DescriptorKindFilter,
psi: PsiElement
): DeclarationDescriptor? {
return this.unsubstitutedMemberScope.findEnclosedDescriptor(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are using firstOrNull it should make no performance impact if we just combine 2 lists together and call the filter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this method is going away (and it was wrong by not delegating to the ClassDescriptor filter version) so i'm assuming this comment applies to the other one as well?
in that case though i have member scopes that i cannot combine.
I can combine the results of getContributedDescriptors but that would have two unnecessary things:

  • it would query staticScope regardless even when we find the result in member scope
  • it would create another list to combine the two.

hence, i think it is better to keep the ClassDescriptor.findEnclosedDescriptor above as is? (this function is going away)

@neetopia neetopia merged commit 638d9a1 into google:master Nov 25, 2020
neetopia pushed a commit that referenced this pull request Dec 23, 2020
* Handle statics in resolveJavaDeclaration

This PR fixes a bug where static java declarations were not being resolved
properly because we would only look at the main scope of the class declaration
but we also need to check the static scope.
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.

2 participants