Add ImmutablesInterfaceDefaultValue

[schlosna]

Before this PR

In code reviews, noticed some uses of @Value.Immutable on default methods in interfaces without corresponding @Value.Default, @Value.Lazy or @Value.Derived

After this PR

==COMMIT_MSG==
Add ImmutablesInterfaceDefaultValue

@Value.Immutable interface default methods should be annotated @Value.Default
==COMMIT_MSG==

Possible downsides?

Refactoring suggestion assumes @Value.Default was intended if not already annotated @Value.Lazy or @Value.Derived

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Add ImmutablesInterfaceDefaultValue

@Value.Immutable interface default methods should be annotated @Value.Default

Check the box to generate changelog(s)

• Generate changelog entry

[jkozlowski]

LGTM! Just a comment, but don't think it's blocking

@CRogers / @tpetracca for SA, I think you were both mentioning adding some more rules for immutables usage.

[jkozlowski]

Perhaps this would read slightly better as a series of matchers?

gradle-baseline/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java

Line 48 in 0f716d5

private static final Matcher<MethodTree> methodReturnsAutoCloseable = Matchers.allOf(

But unclear what is more idiomatic.

[jkozlowski]

I think if would look like this:

private static final Matcher<Tree> MATCHER = Matchers.allOf(
            Matchers.enclosingClass(Matchers.hasAnnotation("org.immutables.value.Value.Immutable")),
            Matchers.hasModifier(Modifier.DEFAULT),
            Matchers.not(Matchers.anyOf(
                    Matchers.hasAnnotation("org.immutables.value.Value.Default"),
                    Matchers.hasAnnotation("org.immutables.value.Value.Derived"),
                    Matchers.hasAnnotation("org.immutables.value.Value.Lazy"))));

Meaning you can collapse the if-statement to just:

if (MATCHER.matches(tree, state)) {
  ...
}

[schlosna]

yeah, will pull these matchers out

[carterkozak]

Can we detect if an immutables style annotation is used with defaultAsDefault = true? Example: https://immutables.github.io/style.html#custom-immutable-annotation

[schlosna]

Good question, haven't test that yet.

I've seen varying .*Immutable(s)?Style annotations used across projects, and might be useful to encourage consistent usage.

[CRogers]

I don't think this should be an error? Default methods are regularly used as helper instance methods rather than anything an @Value.Default, and this is a totally valid use of it.

[jkozlowski]

Ah I guess you need to check for no args?

[CRogers]

Even no args is a perfectly valid use of the default methods in @Value.Immutable interfaces, to use as helper methods. I'm trying to an example externally, this is used a lot internally.

[jkozlowski]

:sigh:, I guess we can't really do anything here then, because this will suddenly start failing everyone? I think annotating with one of the annotations is not a change? And then I thought we have some automation for baseline bumps to upgrade people's code if we can.

So I guess question is are there cases where you'd legitimately not want any annotations, and adding an annotation would break the world?

[CRogers]

A few valid uses I found I can link to:

• https://github.com/palantir/atlasdb/blob/5dec063955e6efe10bdf24dbd368abf19a6b9e67/atlasdb-cassandra/src/main/java/com/palantir/atlasdb/keyvalue/cassandra/thrift/QueryWeight.java#L30
• https://github.com/palantir/conjure-python/blob/eba5f91fd2ce1fcfe52f74f4aca217c75cdf5c94/conjure-python-core/src/main/java/com/palantir/conjure/python/poet/PythonLine.java#L25
• https://github.com/palantir/cassandra-manager/blob/8bcdfed111e905d03a5a51f11782aa0d1f353592/cassandra-manager-core/src/main/java/com/palantir/cassandra/manager/core/state/ClusterStateSnapshot.java#L40-L50
• https://github.com/palantir/conjure-python/blob/eba5f91fd2ce1fcfe52f74f4aca217c75cdf5c94/conjure-python-core/src/main/java/com/palantir/conjure/python/poet/NamedImport.java#L39

[CRogers]

Adding an annotation is 100% a breaking change (and in all currently working cases not what you want). If you serialise the immutable, suddenly you end up saving more data. It adds new methods to the builder which you may not want.

We can't tell whether someone forgot to add a @Value.Default or they're using a default method as a helper function that should not be persisted/in the builder - that's up the dev to decide. If they have forgotten to add a @Value.Default then it should be obvious elsewhere when they can't use it in a builder.

I don't think this check is possible to implement statically.

[tpetracca]

for the 4 examples you gave, are they all not validly annotated as @Value.Derived? or is the whole idea someone might directly set them as well in the builder.

[JacekLach]

We can't tell whether someone forgot to add a @Value.Default or they're using a default method as a helper function that should not be persisted/in the builder - that's up the dev to decide. If they have forgotten to add a @Value.Default then it should be obvious elsewhere when they can't use it in a builder.

This is true except for the most important case, of interfaces declaring configuration options. A forgotten Value.Default will only be exposed there once someone tries to override a default config and the override will not take - which can be difficult to notice in the first place.

I think this check applied only to things referenced in the service configuration classes/interfaces would be a great benefit

[pkoenig10]

I don't see why configuration definitions are any different. We regularly use default methods in configuration definitions for values that are trivially derived.

Sounds like we have a problem

[pkoenig10]

I'm not sure I agree with this check. We have a number of legitimate uses of default methods that do not need to be memoized because they simply return constants or perform a trivial calculation (like a lookup in a map field). I don't want to bloat the size of my immutables objects with these fields that are practically free to compute on-demand. Some of these objects are used in caches where memory is at a premium and we'd like to avoid increasing the size of our objects unnecessarily.

For an example, see AtlasUser in our internal authentication service.

[schlosna]

Going to close this out for now given discussion