Tracking Issue for Attributes being Experimental.

[carl-mastrangelo]

In #1700 it was decided that Attributes is still experimental. This issue is acts the the reference from the source code.

[dapengzhang0]

should we un-public Attributes#keys()?

[herbyderby]

Attributes.get(Key key) current has @SuppressWarnings("unchecked"). It is actually not safe--Key.of will return a key for any and it is never enforced. (It is generally a bad practice for a method signature to use a type parameter a single time).

I suggest:

• adding Class argument to Key.of
• changing Attributes.get to return key.type.cast(data.get(key); then no suppression will be necessary

(This may be insufficient if there are any keys that use generic types)

[ejona86]

@herbyderby, is there a code snippet that would show how it breaks? Attributes.Key is the key used for setting/lookup and it uses reference equality. So there's really no way I see to break the Generics.

[herbyderby]

For example:

Key<Integer> ikey = Key.of("mykey");
Key<String> skey = (Key) ikey;

Attributes attr = Attributes.newBuilder()
    .set(skey, "foo")
    .build();

System.err.println("result: " + attr.get(ikey));

Prints "result: foo", even though get here is supposed to return an Integer.

It would be better for attr.get to fail. Currently it is delayed until you actually do something which causes a cast, if at all--methods on Object like equals and toString won't complain.

Admittedly this example requires a raw cast of the key. But given that it is not much work to make Attributes & Key actually typesafe, I think it is worth being a bit more defensive.

[ejona86]

This seems like an obvious, "you're going to have a bad time":

Key<String> skey = (Key) ikey;

That seems no different than, say:

List<Integer> ints = ...;
List<String> l = (List) ints;

[herbyderby]

List is a good example, because Collections.checkedList does exist, for reasons that also apply here:

http://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#checkedCollection(java.util.Collection,%20java.lang.Class)

[ejona86]

But nobody uses checkedCollection. If you're casting, you're already going to have issues. Passing the Class is possible, but class literals are completely broken with generics. So that'd make everyone suffer for a case most people simply don't care about and won't hurt them.

[herbyderby]

I doubt there are good use cases for generic values here. But I've also realized that I'm just repeating the advice already in Effective Java 2nd ed. Item 29, so I'll give up now.

[carl-mastrangelo]

@herbyderby: I think your suggestion is just passing the buck. The problem still would exist:

Attributes attr = ...;
List<String> list = new ArrayList<>();
Key<List<String>> k1 = Key.of("yay", List.class);
Key<List<Integer>> k2 = (Key) k1;

List<Integer> uhoh = attr.get(k2);

The class cast check you add would succeed incorrectly. If you wanted a real match, you would need to pass in a TypeLiteral.

[herbyderby]

Yes there is no good solution if you want to support generic values--they have a bunch of problems. But as I mentioned, I doubt there is a real use case for that. If you really need a complex value you can always wrap it up into an AutoValue or whatever.

[ejona86]

API review:

• Key.of(debugString) is actually a bit weird; we were originally doing Key.of(name) which isn't as weird. Across all similar key types (Context, Attributes, and this one), we want to swap to the form Key.create(debugString) and Key.createWithDefault(debugString, T default). So that would mean adding create here and deprecating of().
• Consider removing keys(); need to investigate what it is being used for. We're fine with it, but it is a bit weird; we're not sure what it's useful for. We are okay with keys() here because Attributes was intended to be an immutable (and type-safe) map and is only really passed between the producer and consumer. Unlike Context and CallOptions, there doesn't seem much point to visibility-restricting certain keys (so you can't access the value if you can't get at the instance), which is the main thing having keys() would defeat. But because each key is semantically quite distinct, it is hard to determine what someone is using it for.
• Replace static newBuilder(Attributes) with non-static toBuilder(). Deprecate (and later remove) newBuilder(Attributes)

With those tweaks, looks good to stabilize.