@FieldNameConstants Fields classes should use inheritance

[brianlenz]

Describe the feature

We are upgrading to the 0.18.4 and switching over @FieldNameConstants to the new approach. One downside to the new Fields class approach is that you lose references of fields in the superclass hierarchy. Previously, you could use the FIELD_ constants from the current class and all of its superclasses. I didn't need to know or care where the fields were defined, which as a consumer of the class, I shouldn't really need to know anyway. With the new Fields approach, you have to explicitly reference the specific superclass that contains the field in order to reference the proper Fields class that the constant lives on.

Current Approach

lomboked:

@FieldNameConstants
public class FieldNameConstantsBase {
  private final String iAmAField;
}

@FieldNameConstants
public class FieldNameConstantsExample extends FieldNameConstantsBase {
  private final int andSoAmI;
}

delomboked:

public class FieldNameConstantsBase {
  private final String iAmAField;
  public static final class Fields {
    public static final String iAmAField = "iAmAField";
  }
}

public class FieldNameConstantsExample extends FieldNameConstantsBase {
  private final int andSoAmI;
  public static final class Fields {
    public static final String andSoAmI = "andSoAmI";
  }
}

If I'm a consumer of FieldNameConstantsExample, I know that it has two fields: iAmAField and andSoAmI. I don't care where those fields are defined, nor should I need to know. But, if I want to reference the field name constant for iAmAField, I have to dig into the class hierarchy to see that it is defined on FieldNameConstantsBase and therefore must reference FieldNameConstantsBase.Fields.iAmAField.

Worse, the FieldNameConstantsBase could have limited access and may not even be exposed publicly (e.g. protected or package-protected). In this case, I have access to the class properties, but not the field name constants 😩

Suggested Approach

To solve this problem, the Fields class should not be final and subclasses should extend the nearest superclass' Fields class.

delomboked:

public class FieldNameConstantsBase {
  private final String iAmAField;
  public static class Fields {
    public static final String iAmAField = "iAmAField";
  }
}

public class FieldNameConstantsExample extends FieldNameConstantsBase {
  private final int andSoAmI;
  public static class Fields extends FieldNameConstantsBase.Fields {
    public static final String andSoAmI = "andSoAmI";
  }
}

With this approach, as a consumer of FieldNameConstantsExample, I don't need to know or care about the class structure. I know that the FieldNameConstantsExample object I am consuming contains iAmAField, so it references the field name constant through FieldNameConstantsExample.Fields.iAmAField.

Describe the target audience

Anybody using object-oriented design w/ inheritance + @FieldNameConstants 😉

Additional context

• Original design in New features for @FieldNameConstants #1774. The conversation there doesn't seem to have covered the inheritance issues introduced in the new approach.
• The Enum-based approach is incompatible with this design due to the fact that Enums do not support inheritance. I personally don't see the need/value to use Enums for these types of constants, and I think this is a much bigger problem than supporting Enums is. As a result, the Enum approach would probably have to be abandoned in order to solve this problem.
• I realize that there is some complexity with the approach due to the unknown of whether @FieldNameConstants is defined on superclasses or not. The proposed solution here is definitely the ideal. If it's unfeasible, a couple of alternative solutions might be:
  • Use a @SuperBuilder-like approach where all superclasses must have @FieldNameConstants if the current class wants to use it. This could make some good sense when you control the entire class hierarchy, but it is limiting if you are using third-party libraries that you don't control.
  • On the current class' Fields class, define all constants from not only the current class, but also all superclasses. This would result in a lot of duplication in the Fields classes, but it at least ensures that you can access all of the field name constants for all fields of the class. It would also allow you to use @FieldNameConstants on classes that extend third-party libraries (and would include those fields).

This is my first interaction with the Lombok team, so just wanted to let you know I appreciate all of your efforts on this great library 👍

[brianlenz]

Note that I see you can define your own Fields classes to work around this issue, but that seems like adding the type of boilerplate code that Lombok aims to solve 😁

lomboked

@FieldNameConstants
public class FieldNameConstantsBase {
  private final String iAmAField;

  @NoArgsConstructor(access = AccessLevel.PROTECTED)
  public static class Fields {}
}

@FieldNameConstants
public class FieldNameConstantsExample extends FieldNameConstantsBase {
  private final int andSoAmI;

  public static class Fields extends FieldNameConstantsBase.Fields {}
}

[Maaartinus]

Disclaimer: Not a lombok team member here.

I personally don't see the need/value to use Enums for these types of constants

I find things like Enum#values() pretty useful (e.g., when I converted the object to a Map). Sure, in the end I need reflection somewhere...

Note that I see you can define your own Fields classes to work around this issue, but that seems like adding the type of boilerplate code that Lombok aims to solve

IMHO your workaround is fine. It is some boilerplate and I can imagine Lombok supporting it in a simpler way, but the cost is just one line, while you get all the fields of the superclass. Moreover, while you have to write this single line, you don't have to change it, when you add fields or whatever.

Concerning the simpler way, you'd have to specify if the class should be final and if it should inherit from super Fields. Together, it isn't much shorter than your line and it's harder to understand (sure, it's trivial, but your line is much simpler, nonetheless).

@rzwitserloot IMHO, it'd be nice to officially support this workaround (assuming, you don't want to support inheritance more), i.e., to add a sentence like "You can also use extends, e.g., to include fields of the parent class." and a test.

[eximius313]

Original implementation of @FieldNameConstants (#46) was one of the most voted issues in Lombok and it was just fine - simple but very helpful feature.
Then - by some reasons - it changed to what it is now, which causes many problems (because of that I must stick to Lombok 1.18.2 in all of my projects).
I hope that thanks to this feature, @FieldNameConstants "will be great again" :D

[rzwitserloot]

@eximius313 we changed it on request. Seems a bit daft to change it on request again, no? What's bad about the current version? Note that the old way @FNC worked didn't address the concern in this issue at all. Please don't make drive-by unrelated comments in issues, it clogs up the flow of conversation.

[rzwitserloot]

@brianlenz This is a well-written feature request; thank you!

Unfortunately, the problem with the request is that we can't do it without resolution which is probably a bit too heavy for this feature. We could introduce something like @FieldNameConstants(callSuper=true) which would make lombok generate it precisely as stated: class Fields extends SuperClassName.Fields – but that's a lot uglier and is so close to just writing your own (empty) class which lombok will then fill, why bother, right?

Top of the list if we ever tackle resolution in a way that it's not so pricey in performance and support.

[eximius313]

@rzwitserloot

we changed it on request. Seems a bit daft to change it on request again, no?

No, because current version is bad. Why didn't you respect the original version that community wanted? It had 29 votes (#46)! How many votes had current version? Only it's authors.

What's bad about the current version?

Just look how many issues it generated on github and how many people have problem with that. Sometimes "better is the enemy of good". @brianlenz or I described it in details in our issues.

Note that the old way @fnc worked didn't address the concern in this issue at all

Because it worked just fine before! That's why it didn't have to address this issue explicitly! Current version is much worse!

[nealeu]

@rzwitserloot Would it be possible to do `Bar.Fields._Super.Fields?

If I understand the resolution wiki page correctly, then this would generate the right references without need for resolution.

Looking at HandleFieldNameConstants.java, I don't think a PR would be too difficult to do here. It might need @FieldNameConstants(generateSuper=true) for backwards compatibility, but I think it'd be performant.

[nealeu]

Another workaround could be to allow us to add (or to generate):

  public static class Fields extends TheSuperClass.Fields {}

buy making the generated Fields class non-final