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

ClassPath.getTopLevelClasses() does not return a top-level class with $ character #3349

Open
suztomo opened this issue Dec 21, 2018 · 3 comments

Comments

@suztomo
Copy link
Member

suztomo commented Dec 21, 2018

@netdpb and I found that ClassPath.getTopLevelClasses() does not return top-level class with $ character in its name. Example class: com.google.cloud.bigquery.$AutoValue_Labels in google-cloud-bigquery-1.56.0.jar.

Test case

  @Test
  public void testGuavaTopLevelClass() throws Exception {
    // google-cloud-bigquery-1.56.0.jar contains com.google.cloud.bigquery.$AutoValue_Labels
    URL jarFileUrl =
        new URL(
            "file:///home/suztomo/.m2/repository/com/google/cloud/google-cloud-bigquery/1.56.0/google-cloud-bigquery-1.56.0.jar");
    URL[] jarFileUrls = new URL[] {jarFileUrl};

    URLClassLoader classLoaderFromJar = new URLClassLoader(jarFileUrls, null);

    com.google.common.reflect.ClassPath classPath =
        com.google.common.reflect.ClassPath.from(classLoaderFromJar);

    ImmutableList<String> allClassNames =
        classPath.getAllClasses().stream()
            .map(classInfo -> classInfo.getName())
            .collect(toImmutableList());

    // This code does not return class com.google.cloud.bigquery.$AutoValue_Labels
    // Guava only checks the existence of '$'
    // https://github.com/google/guava/blob/master/guava/src/com/google/common/reflect/ClassPath.java#L85
    ImmutableList<String> topLevelClassNames =
        classPath.getTopLevelClasses().stream()
            .map(classInfo -> classInfo.getName())
            .collect(toImmutableList());

    String class$AutoValue_Labels = "com.google.cloud.bigquery.$AutoValue_Labels";
    System.out.println("In all classes? " + allClassNames.contains(class$AutoValue_Labels)); // true
    System.out.println(
        "In top-level classes? " + topLevelClassNames.contains(class$AutoValue_Labels)); // false. It should be true.
  }

JLS 3.8 states that dollar sign ('$') is a valid Java letter.

AutoValue has @Memoized annotation, which creates class $AutoValue_XXXX and AutoValue_XXXX extends $AutoValue_XXXX in the same package.

@eamonnmcmanus
Copy link
Member

The issue of course is that ClassPath.getTopLevelClasses() assumes that a class is nested if it has $ in its name. If a class Bar is nested inside com.example.Foo then its full name will be com.example.Foo$Bar, but it is perfectly legal to define a class whose actual name is Foo$Bar. So the heuristic that getTopLevelClasses() uses cannot work in general. I believe the only way to be sure would be to look for the InnerClasses attribute inside the .class file, and that implies that this method would become quite a bit slower and more complicated. Alternatively, we could disregard sequences of $ characters at the beginning and end of a class name when determining nestedness. That would cover the case at hand, while still not being a general solution.

@suztomo
Copy link
Member Author

suztomo commented Jan 2, 2019

Thank you for response. Our project (cloud-opensource-java) will use getAllClasses instead. So I don't expect Guava to implement something specific to Autovalue.

@Maaartinus
Copy link

@eamonnmcmanus

Alternatively, we could disregard sequences of $ characters at the beginning and end of a class name when determining nestedness.

I guess, doing this would be a clear improvement for a minimum cost.

You could also use a heuristics like "when there's no Foo, then Foo$Bar is not a nested class" (which also takes care of the leading dollar). This feels pretty hacky, but solves cases like "sometool" generating sometool$SomeClass (I can't recall where I saw it and if it really looked like this).

@netdpb netdpb added P3 and removed P2 labels May 29, 2020
nick-someone pushed a commit that referenced this issue Oct 24, 2020
Seems like a useful method? I can't remember why I didn't add it in the first place. Perhaps just an oversight.

Partially addresses #3349

RELNOTES=`reflect`: Added `ClassInfo.isTopLevel()`.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=338738055
nick-someone pushed a commit that referenced this issue Oct 26, 2020
Seems like a useful method? I can't remember why I didn't add it in the first place. Perhaps just an oversight.

Partially addresses #3349

RELNOTES=`reflect`: Added `ClassInfo.isTopLevel()`.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=338738055
copybara-service bot pushed a commit that referenced this issue Apr 16, 2021
And add some documentation relevant to #3349

RELNOTES=n/a
PiperOrigin-RevId: 368551044
copybara-service bot pushed a commit that referenced this issue Apr 16, 2021
And add some documentation relevant to #3349

RELNOTES=n/a
PiperOrigin-RevId: 368916206
@cpovirk cpovirk self-assigned this Apr 19, 2021
copybara-service bot pushed a commit that referenced this issue Apr 19, 2021
Relevant to #2130 and #3349.

RELNOTES=n/a
PiperOrigin-RevId: 369233331
copybara-service bot pushed a commit that referenced this issue Apr 19, 2021
Relevant to #2130 and #3349.

RELNOTES=n/a
PiperOrigin-RevId: 369241326
@cpovirk cpovirk removed their assignment Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants