Support for Kotlin and Setters/Getters for Private Members

[anthonycr]

Ticket

• Stag-java does not support Kotlin #99 - Kotlin support

Ticket Summary

We would like to support Kotlin. Due to the way Kotlin compiles, we cannot assign fields in a Kotlin model at runtime, but instead must rely on getters and setters to access those fields. Currently, stag has no way to use getters and setters. A side affect of this support is that Java models will no longer require members to be non-private.

Implementation Summary

In order to support this, stag had to be refactored so that it did not have direct knowledge about how to assign a field. The approach I took was to abstract the field access behind an abstract class called FieldAccessor. This class provides a skeleton framework for code that is common to all fields that will be assigned, but it does not specify HOW the field should be assigned. Rather, concrete implementations of the field DirectFieldAccessor and MethodFieldAccessor provide the functionality to access the fields. DirectFieldAccessor as the name implies uses direct field access, and MethodFieldAccessor searches for getters and setters in order to use those to access the fields.

Now, instead of throwing an error when a private field is encountered, stag will try to construct a MethodFieldAccessor instead of accessing the field directly. If getters and setters cannot be found, an exception is thrown and stag then communicates to the consumer that their field is private and doesn't have both setters and getters.

Setters and getters currently require the following format:

• set[NAME_OF_FIELD]([VALUE_TO_SET])
• get[NAME_OF_FIELD](void)

So in Java...

@UseStag
public class PrivateMembers {

    @NotNull private String testString;

    @NotNull
    public String getTestString() {
        return testString;
    }

    public void setTestString(@NotNull String testString) {
        this.testString = testString;
    }
}

In Kotlin (setters/getters implied)...

@UseStag
class KotlinSamples {

    // nullable field
    var stringField: String? = null

    // non null field
    var nonNullStringField: String = "default"

    // field with serialized name
    @SerializedName("boolean_field")
    var booleanField: Boolean = false
}

Under the hood, Kotlin will generate setters and getters for each of these fields along the naming format of setFieldName/getFieldName. The setters and getters prepend the appropriate action and capitalize the first letter of the variable name.

If you are using Hungarian notation in your Java models, you will likely need to provide an additional annotation processor option to Stag to enable different naming scheme for setters/getters:

apt {
    arguments {
        stagAssumeHungarianNotation true
    }
}

Then in Java, the above code can become...

@UseStag
public class PrivateMembers {

    @NotNull private String mTestString;

    @NotNull
    public String getTestString() {
        return mTestString;
    }

    public void setTestString(@NotNull String testString) {
        this.mTestString = testString;
    }
}

In order to support this accessor style functionality, a lot of refactoring was done including eliminating most usage of Element in preference of VariableElement or TypeElement. This allows the code to be much more clear in its purpose.

How to Test

Build and run tests.

• TODO: More Kotlin test cases
• TODO: Figure out build problem when Kotlin module is included as a part of :sample
• TODO: Add test cases for java classes with private members and setters/getters
• TODO: Update documentation to reflect ability to have private members
• TODO: Naming options for getters/setters (Hungarian notation not supported yet)

[kvenn]

I haven't dug in as deeply as I'd like - but I did a high level review of the code and agree with the approach you outlined in your ReadMe.

If I have some more time I'll dig further into the logic. But as far as a first pass goes, this PR looks GTG.

[kvenn]

🔥

[kvenn]

Are these getters/setters required?

[anthonycr]

They are not required, but I meant to make all these members private so they should be required. Good catch

[kvenn]

How does field visibility work in Kotlin? Is it fine to have these public? Should they be private with internal getters/setters?

Obviously since this is a sample it doesn't really matter - but I'm curious what type of visibility people usually use with Kotlin. Are setters exposed? I can imagine you'd want to make most classes externally immutable. Unless they're immutable by default?

[anthonycr]

For reference in this discussion: https://kotlinlang.org/docs/reference/visibility-modifiers.html

The way Kotlin member variables work is that they can only be accessed through getters and setters. Setters and getters are automatic and are left out of the source code unless you have different logic (see https://kotlinlang.org/docs/reference/properties.html). Looking at the corresponding generated Java code, when you specify public visibility on a field in Kotlin, you are giving public visibility to the getters and setters, but the field itself is always declared private.

For json model objects, you could make your class immutable by declaring all the fields in the constructor, but gson functions by just assigning directly to fields rather than through a constructor, so stag functions the same (except using methods rather than fields in this case).

You can change the visibility of setters and getters individually (refer to the properties link above). Right now stag only supports public setters in kotlin. Maybe it should support internal, but this current code can't because internal behaves different from package local and therefore needs some special logic to access. Maybe we should file an issue about internal setters and getters for a future PR, or maybe it should be part of this PR.

[anthonycr]

I tested out using internal and it's not viable to support until stag is generating Kotlin output for kotlin modules instead of java. I tested by modifying the myField field and making only the setter internal. See the below code for the temporary java code generated by the kotlin plugin. Note the different naming for the setter that contains the module name in it... something that we don't have access to within the context of an annotation processor:

@com.vimeo.stag.UseStag()
public final class KotlinSamples {
    @org.jetbrains.annotations.Nullable()
    private java.lang.String stringField;
    @org.jetbrains.annotations.Nullable()
    private java.lang.Object testField;
    
    @org.jetbrains.annotations.Nullable()
    public final java.lang.String getStringField() {
        return null;
    }
    
    public final void setStringField(@org.jetbrains.annotations.Nullable()
    java.lang.String p0) {
    }
    
    @org.jetbrains.annotations.Nullable()
    public final java.lang.Object getTestField() {
        return null;
    }
    
    public final void setTestField$sample_kotlin_model_debug(@org.jetbrains.annotations.Nullable()
    java.lang.Object p0) {
    }
    
    public KotlinSamples() {
        super();
    }
}

[kvenn]

It sounds like this would be an additional feature. And also something to look out for (or ask Brent about) if we start using Kotlin in production. It sounds like you'd need a lot of boilerplate code (generating private setters for every model) to make a model immutable.

[anthonycr]

I'm going to ticket this after merge, but what I would like to do is have a separate stag-library-compiler-kotlin module that generates kotlin code instead of java. This would allow us to support the module level visibility without needing to jump through hoops to access the fields. It isn't straight forward though as it will require major refactoring because code gen is baked into the stag-library-compiler module and we need code gen to be interchangeable in order to support kotlin generation.

[kvenn]

This is pretty sweet.

[kvenn]

Nitpick - this sounds declarative for returning a boolean. requiresNotNull() sounds a little more like a question if FieldAccess.requiresNotNull(), do something.

[kvenn]

This actually brings up another code style thing. Because it could alternatively be doesRequireNotNull() - which is more consistent with how we usually name booleans (and goes along with this SO post).

But this and this are also interesting answers.

We should add the result to code style regardless.

[kvenn]

I don't think you need variable.length() as the second arg to substring.

[kvenn]

Annotation?

[kvenn]

I personally don't really like the use of annotation processor arguments in this instance. Up until now they're pretty much optional (and only necessary for the power user), but they'll be required for a person who uses Hungarian notation after this.

I don't really know of a way to pass arguments to an AP other than this, but I know Gson solves the naming convention problem by having it as an argument to their builder. Stag is super lightweight and doesn't require a builder, and I'm not sure if the way Gson works for annotation processing is fundamentally different from stag. I guess I'm calling this out to see if there's a friendlier way to add it into the Stag API.

The only two things I can think of are passing to a param to the StagFactory and not requiring the naming convention knowledge until Runtime (which I'm assuming is how Gson does it?) or by adding it as an argument at the class level @UseStag annotation which seems a little more appropriate but would be a pain to have to copy over to every annotation if you're using Hungarian.

Currently my two proposed solutions aren't better than the AP arg, but I'm just calling it out to see if you can think of anything.

[anthonycr]

Because Gson isn't generating any code but just accessing fields at compiler time, we can't rely on something like a constructor or method parameter to determine whether to accept hungarian notation or not. We need to know at compile time what the name of the methods are so we can create code.

That leaves only two concrete solutions in my mind: using an annotation, or using a processor argument.

I do like the idea of an annotation level parameter, but my concern with that is that it could become a hassle to add the naming scheme used to every class declaration, especially when any reasonable codebase uses consistent naming.

A third solution that I've thought of is to remove the option to choose, and have stag choose for you. So, given a field mField, it would look for two different naming schemes, one with the first letter intact, and one without (for setters setMField and setField). This would mean no intervention from the consumer. I didn't implement it this way though because it's non deterministic and there's no way to know which method to choose if both setMField and setField exist in a class. In a small subset of scenarios, this might even generate the wrong code. Given two fields mode and ode, with an ambiguous naming scheme, setOde could justifiably be used to set either field. We could try to enforce consistent naming per class, but determining this seems problematic. We could remove some ambiguity by checking whether the first letter of a field starts with m, but idk.

[kvenn]

Good points. Looks like processor args is the best bet - maybe with class level overrides as a future feature. And auto detection would be a pretty sweet reach goal (if there was a way to guarantee it's correct).

[anthonycr]

I will ticket so that we are aware of it in the roadmap

[anthonycr]

@kvenn responded to feedback and replied to comments a66a6ea 5a917d9

[kvenn]

Thanks for the thorough responses - this looks good to merge 👍

I think my biggest takeaway here is Kotlin has some weird scoping (and it's up to you if you want to add a ticket to add support for varying scopes).

[anthonycr]

@kvenn
Ticketed issues here:
#105
#106
#107