CacheBuilder newBuilder method should be parameterised #738

Closed
gissuebot opened this Issue Oct 31, 2014 · 10 comments

Projects

None yet

1 participant

@gissuebot
Collaborator

Original issue created by samuel.lb on 2011-10-04 at 07:22 PM


The current (10.0) implementation is:

public static CacheBuilder<Object, Object> newBuilder() {
    return new CacheBuilder<Object, Object>();
}

my take is that it was meant to be:

public static <K, V> CacheBuilder<K, V> newBuilder() {
  return new CacheBuilder<K, V>();
}

@gissuebot
Collaborator

Original comment posted by cgdecker on 2011-10-04 at 09:44 PM


No, it was meant to be CacheBuilder<Object, Object>. It doesn't need to have specific key/value types until you configure it with something that requires specific key/value types such as a RemovalListener. Otherwise, it can remain a CacheBuilder<Object, Object>, allowing you to build multiple Caches (with different key/value types) based on the same configuration.

@gissuebot
Collaborator

Original comment posted by kevinb@google.com on 2011-10-05 at 04:47 AM


I apologize if this is counter-intuitive, but yes, cgdecker is correct. There should be @param descriptions of those type parameters that should help explain. It also helps to think about how javac type inference works; putting <K, V> on newBuilder wouldn't even help much because javac doesn't know how to infer them from inside the middle of a chain.


Status: WorkingAsIntended
Labels: Type-Defect

@gissuebot
Collaborator

Original comment posted by samuel.lb on 2011-10-05 at 06:57 AM


Thanks for the quick response, I understand better now and it all makes sense. I think a bit of Javadoc here would probably help. What tricked me is that I didn't build my cache using the builder in a single chain, but was trying to configure it given some parameters, hence splitting the call chain in multiple calls. I wanted a cache of with specific types and assumed I needed the builder with the same types (from the first call).

@gissuebot
Collaborator

Original comment posted by kohanyi.robert on 2011-10-06 at 05:54 AM


Relating to "semi-parameterizedness" of CacheBuilder I've encountered some strange behavior. Basically the following code doesn't compile under using JDK 1.6 (I've attached a simple Maven project which produces this issue).

  public static void main(String[] args) {
    fromBuilder(CacheBuilder.newBuilder());
  }

  public static void fromBuilder(CacheBuilder<Object, Object> builder) {
    fromCache(builder.build(new CacheLoader<String, String>() {

  @Override
  public String load(String key) throws Exception {
    return null;
  }
}));

  }

  public static void fromCache(Cache<String, String> cache) {}

I get the following compilation error (using mvn clean compile):

\Work\guava\src\main\java\CacheBuilderTest.java:[12,4] fromCache(com.google.common.cache.Cache<java.lang.String,java.lang.String>) in CacheBuilderTest cannot be applied to (com.google.common.cache.Cache<java.lang.Object,java.lang.String>)

(On a side-note: Eclipse doesn't bother to tell that there is an error with type inference, the code just compiles inside the IDE without any warnings or errors.)

Note the part where it says "cannot be applied to (com.google.common.cache.Cache<java.lang.Object,java.lang.String>)"; somehow the compiler infers <Object, String> type parameters instead of <String, String>. I could understand <Object, Object>, but <Object, String>? :)

The workaround I've found is that instead of

   fromCache(builder.build(new CacheLoader<String, String>() {

  @Override
  public String load(String key) throws Exception {
    return null;
  }

   }));

one has to use

   Cache<String, String> cache = builder.build(new CacheLoader<String, String>() {

  @Override
  public String load(String key) throws Exception {
    return null;
  }

   }));
   fromCache(cache);

which works as intended.

I know that sample code isn't too sophisticated, however my intention with taking a CacheBuilder as an argument is that this way my method holds control over the CacheLoader implementation the Cache being built will use.

I know also, that this isn't an error from your (the Guava developers) behalf, but I think someone might come across this same issue, maybe this post will help them.

@gissuebot
Collaborator

Original comment posted by cgdecker on 2011-10-15 at 01:49 PM


Issue #759 has been merged into this issue.

@gissuebot
Collaborator

Original comment posted by cody.a.ray on 2012-03-28 at 10:41 PM


There seems to be a bug around here that just bit me. For example, to cache compile regex patterns:

  public SomeClass() {
    this(CacheBuilder.newBuilder()
        .expireAfterAccess(60, TimeUnit.MINUTES)
        .build(new CacheLoader<String, Pattern>() {
          @Override
          public Pattern load(String pattern) throws Exception {
            return Pattern.compile(pattern);
          }
        }));
  }

  SomeClass(LoadingCache<String,Pattern> patternCache) {
    this.patternCache = patternCache;
  }

Get the same odd error as he does with LoadingCache<Object,Pattern>. Eclipse is showing that build() returns a LoadingCache<String,Pattern> but the compiler isn't interpreting it that way.

cannot find symbol
  symbol : constructor SomeClass(com.google.common.cache.LoadingCache<java.lang.Object,java.util.regex.Pattern>)

And in this instance, storing it to a variable first for type inference to correct itself isn't working.

@gissuebot
Collaborator

Original comment posted by wasserman.louis on 2012-03-29 at 12:45 AM


You probably need to explicitly specify .<String, Pattern> build(new CacheLoader<String, Pattern>() {...

@gissuebot
Collaborator

Original comment posted by kevinb@google.com on 2012-03-29 at 07:06 AM


Yeah, type inference is a real drag whenever you're not assigning directly to a variable. :-(

@gissuebot
Collaborator

Original comment posted by cody.a.ray on 2012-03-29 at 03:52 PM


For others stumbling on this thread, I solved this issue by declaring the generics on build(), like

CacheBuilder.newBuilder()
        .expireAfterAccess(60, TimeUnit.MINUTES)
        .<String,Pattern>build(new CacheLoader<String, Pattern>() {
          @Override
          public Pattern load(String pattern) throws Exception {
            return Pattern.compile(pattern);
          }
        });

@gissuebot
Collaborator

Original comment posted by lowasser@google.com on 2013-08-26 at 11:17 PM


Issue #1514 has been merged into this issue.

@gissuebot gissuebot closed this Nov 1, 2014
@RainWarrior RainWarrior added a commit to MinecraftForge/MinecraftForge that referenced this issue Dec 9, 2015
@RainWarrior RainWarrior Workaround for google/guava#738 724405d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment