Allow specifying visibility as string

[nastra]

In apache/iceberg#8099 we would like to use a custom style annotation to modify the name of the generated enclosing class (mainly to keep API compatibility).

@Value.Enclosing
@Value.Style(typeImmutableEnclosing = "ImmutableSnapshotTable")
interface BaseSnapshotTable extends SnapshotTable {

  @Value.Immutable
  interface Result extends SnapshotTable.Result {}
}

Now we'd also like to configure visibility = Value.Style.ImplementationVisibility.PUBLIC so that the generated class is publicly visible.

However, doing that would mean that we'd be forcing a compile-time dependency to consumers due to the issue described in #291 and the currently suggested workaround for this is to have something like

@Target({ElementType.PACKAGE, ElementType.TYPE})
@Retention(RetentionPolicy.SOURCE)
@Value.Style(visibility = Value.Style.ImplementationVisibility.PUBLIC)
@interface ImmutablesPublic {}

But applying both @Value.Style(typeImmutableEnclosing = "ImmutableSnapshotTable") and @ImmutablesPublic doesn't work due to applying two Style definitions on the class.

One could argue to move @Value.Style(typeImmutableEnclosing = "ImmutableSnapshotTable") into @ImmutablesPublic but it's not feasible to create X custom meta annotations.

Hence my suggestion would be to allow specifying the visibility as a string in order to avoid the issue described in #291 and to not have to introduce custom meta annotations.

@elucash could you take a look at this please?

[elucash]

Thank you for the PR! One one hand, all-in-all-in-the-end we might merge smth like this. On the other hand, the change is very questionable, and I want to make sure we understand context of a problem. I've reread linked tickets, so I've started to recall some of that, but anyway. First, we need to answer the following question:

• Given someone uses annotation, and that annotation is annotated with another annotation which also uses enum, coming from the same lib (annotation class and enum class nested). It is a compile time dependency, why it is somehow bad to have this dependency?

However, doing that would mean that we'd be forcing a compile-time dependency to consumers due to the issue described

This quoted statement is not something I can agree on. This might be true and I might be missing something or just plain wrong. My current take is that, If you use some annotations to some effect you should have these annotation on the classpath. But I want to put it into context, some explanation, which is important:

1. In some cases, annotations don't have to be on the classpath, but that for the cases where you don't do anything with them. Example: you use library XYZ, this library used compile-only annotation library ABC for compile time checking of something. You don't use ABC, so don't have it your classpath and XYZ still works for you without ABC.
2. In case of Style as meta annotation, compile-time processor would read that metadata, why deny it (the processor) access to the classes (Style, and Style$ImplementationVisibility in this case etc) which actually describe the types? Yes, annotation processor can work on compiled metadata sometimes on the basis on just classnames/symbol-names, but that might give Javac warning.
3. We're providing (and that was from the very beginning) annotation-only jar. There were some discussions about that it should have been the default, but because of the backward compatibility we will not change this in this major version anyway. So there's org.immutables:value-annotations (artifact value-annotations) and org.immutables:value:annotations (artifact value, classifier annotations), these 2 jars have the same classes. If someone uses your annotation (@ImmutablesPublic etc) and it's meta-annotated with annotations from Immutables, it's absolutely nothing wrong with having Immutables annotations and enums being transitive dependency of your jar which supplies custom annotation. It's actually a right thing to do. Because this jar should be compilation only, it should not propagate to any dependency of jar which is compiled with the help of @ImmutablesPublic/@Immutables.Value{Style}
4. Be mindful of which classpath you use. (If we for a moment forget about Java Module Systems' new processor-module-path) you might need to have annotation-only jar on either processor-path or regular compile classpath, depending how you setup your annotation processor.

[nastra]

@elucash thanks for your detailed description.

However, doing that would mean that we'd be forcing a compile-time dependency to consumers due to the issue described...

Sorry in case this wasn't precise. What I meant here is based on the below error prone warning, which refers to #291.

warning: [ImmutablesStyle] Using an inline Immutables @Value.Style annotation or meta-annotation with non-SOURCE retention forces consumers to add a Immutables annotations to their compile classpath.Instead use a meta-annotation with SOURCE retention.See https://github.com/immutables/immutables/issues/291.
interface BaseSnapshotTable extends SnapshotTable {
^
    (see https://github.com/palantir/gradle-baseline#baseline-error-prone-checks)

Now using a meta-annotation as I described above isn't feasible for us.
My ideal solution in apache/iceberg#8099 would be to have the below class, where the class itself has package-private access, but we'd like to make the generated ImmutableSnapshotTable public.

@Value.Enclosing
@Value.Style(
    typeImmutableEnclosing = "ImmutableSnapshotTable",
    visibility = Value.Style.ImplementationVisibility.PUBLIC)
interface BaseSnapshotTable extends SnapshotTable {

  @Value.Immutable
  interface Result extends SnapshotTable.Result {}
}

Now when I publish these changes and consume this in another project (let's call it ProjectX), where I'd like to use ImmutableSnapshotTable.builder()...build(), we're seeing the below javac warning:

warning: unknown enum constant ImplementationVisibility.PUBLIC
  reason: class file for org.immutables.value.Value$Style$ImplementationVisibility not found

This is the part that I don't understand about this warning. ImmutableSnapshotTable itself doesn't contain any usage of ImplementationVisibility and ImmutableSnapshotTable is correctly generated as public.

In order to get rid of the warning in ProjectX, one can add a dependency to org.immutables:value-annotations in ProjectX, but this isn't really a viable option, because consuming projects should not have to add anything immutable-specific to make that warning go away.

[elucash]

This got me to seriously think about, this puzzle. Maybe I'm fooling myself, but I think I'm starting to understand what's going on.
So here's the train of thoughts, before it's gone

1. We've made (or left) Immutable.Style annotations CLASS-retained because we want to load these across compiled libraries, so it's possible to have precompiled jar dependency of styles (meta-annotated etc). And because of that and anyone else depending on that retention for any other reason, we cannot change that (i.e. cannot make those SOURCE)
2. You would be ok to make your style-annotation as SOURCE retained, but you need those to be customized per class, so you would use custom name and style-annotation. But because we don't have any merging of styles (not in this version, potential future, Immutables 3 will probably allow this, as necessity for more extensible design/plugins). So this is the reason to resort to inline style, which is flagged by error-prone.
3. The Javac warning is raised for the following reason in the ProjectX which uses generated class: When loading generated class it also loads its supertypes as bytecode (during compilation) and examines annotations. Because style annotation sitting on the superinterface is class retained and it's there in the bytecode, it tries to examine it and fails (non-critically, as a warning) to load attribute with enum value. For some reason it's ok that annotation itself is not available on the classpath, but not ok if its attribute value's enum class is not. This warning, apparently, appeared somewhere after Java 9.
4. If I say that the solution in this PR is still on the table, then I would also like to explore if there are other solutions to what you want to achieve. As I've mentioned, style merging can be implemented, but for this major version it would be quite an undertaking. Separate, small, isolated solution like a new annotation for such custom naming is also something I'm thinking of. I've looked at the patterns in the naming, like shown in API, Core: Move @Value.Immutable usage from iceberg-api to iceberg-core apache/iceberg#8099 It seems like what you want is to have BaseViewHistoryEntry -> ImmutableViewHistoryEntry renaming. Now, there's seems to be a way to automate that. Tell me if this will not work for your case. So you have style attribute typeAbstract, it works in the opposite way of renaming. It detects the logical name of the value type by trimming suffix or prefix. It was specifically designed for users who wish to have *Base, Abstract*, and similar suffixes or prefixes in the abstract interfaces/classes. In this case, your shared style annotation might have typeAbstract="Base*", typeImmutable="Immutable*" , even skip typeImmutable="Immutable*" as it's the default.

[nastra]

@elucash thanks for filling the gap with the explanation in Point 3, that's exactly the part that I didn't fully understand.

We went ahead and merged apache/iceberg#8099 using

@Value.Enclosing
@Value.Style(typeImmutableEnclosing = "ImmutableDeleteReachableFiles", visibility = ImplementationVisibility.PUBLIC, builderVisibility = BuilderVisibility.PUBLIC)
interface BaseDeleteReachableFiles extends DeleteReachableFiles { ... }

so we're not blocked by this PR here. I have not tried the suggestions you provided in Point 4 but I think those would work as well.

To deal with the javac warning, I'm absolutely ok exploring other solutions, and I was also thinking about Style merging, but as you mentioned, that sounds like a lot of work for this particular issue.
So the easiest approach that came to my mind in solving this javac warning was to not use enums in the Annotations.

Also I didn't just want to solve this particular problem for our project, but for other users that might see this javac warning when using Style annotations.

Let me know your thoughts about what kind of possible approach you have in mind and I can make the required changes as part of this PR.

[elucash]

Need to expand documentation with the note about Javac warning (and maybe error-prone's one) at least briefly

It is a good idea to not specify such attributes inline with immutable values, but rather create style annotation (@see Style)

This sorta tries to explain this, but without context (of warnings) it's not explaining it well why these *String() exist

[nastra]

I have mentioned the javac warning and added a link to #291 but I'm not sure we'd want to mention that error-prone flags this (because it's only being flagged in Palantir's error-prone implementation)

[elucash]

I like and use tabs for indents, but this repo uses 2 spaces indentation. (I've seen some other code accidentally has tabs, but that was sloppiness and exception to the rule)

[nastra]

I have changed this to have 2-space indentation, but the diff now looks weirder than before (most likely because this file seems to be using Tabs for indentation)

[elucash]

Yep, this file is in need for reformat, but thank you for getting your lines correctly

[elucash]

I think after thinking and realizing the "exceptional" role of what seems to be strange quirk (if not a bug) in Javac wrt class retained annotations, well I think we should merge this PR and provide this solution. This would be definitely a place where Immutables 3 (and I hope there will be such) should improve and just avoid getting into such cases. I've asked for some cosmetic changes and I'll merge the PR.
I still recommend checking if name detection will work so you can actually use reusable style annotation instead of inline styles, it will add consistency/reduce any typo errors in names put as string literals on top of each abstract value type.
Thank you!

[elucash]

Thank you @nastra for the PR and refinements, merged!

[nastra]

Thanks @elucash for getting this in!