Check to enforce accessor usage on nested record classes

[dodgex]

Hey there,

in some places, we use nested records to e.g. return multiple values from a method, or to encapsulate them for passing them as single argument instead of multiples. Most of those records are private nested classes as they are in almost all cases very specific.

The problem is, that Java allows to use the fields of nested records directly e.g r.name instead of the accessor r.name().

I have not found a check for this case, only RecordAccessorInCompactConstructor which ironically does kind of the opposite.

Depending on how common our use case is in the outside world, I wonder if it would make sense to have such a check in errorprone directly.

Currently we are considering to build that check for our internal plugin, but if there is interest in having it as a core check we could contribute it.

[cpovirk]

I remember some discussion around this from almost a couple years ago now. (Googlers: cl/699102575)

I agree with the irony of RecordAccessorInCompactConstructor. (The long-term good news there is that Java is likely to ban the pattern that causes trouble there entirely.)

Aside from that, I'm sympathetic to the idea of using the accessor method "by default," since the accessor method can be implemented manually, perhaps to clone an array on the way out. It's also the approach that's more often applicable, since you don't have access to the fields of most objects that you interact with. I'm additionally sympathetic to the idea of just pick a pattern and stick with it so that users don't stress over whether one approach is more efficient or otherwise superior to the other.

That said, my array example offers a reason that you might not want to do this: Maybe your accessor method clones the array so that arbitrary users don't mutate it, but maybe you trust yourself to access it directly to avoid the cost of a copy. To address that, we could imagine that our check could make an exception for "What if the accessor is overridden (possibly with a particular kind of implementation)?" Or we could tell you to expose a separate method for accessing it directly. (Presumably methods in the class itself will be allowed to access the field directly, both for performance (like in an equals implementation that wants to skip the clone step) and for correctness (so that a custom getter implementation can access the value instead of recursively calling itself!).)

Now, to go the other direction: We could also consider always suggesting direct field access instead of method calls whenever we detect that it would be equivalent. (That would mean checking for overridability and perhaps other things.)

Overall, though, the feeling from a couple years ago was mostly that this wasn't necessarily worth pushing people on. Do you see harm arising from the direct field accesses (like a failure to use custom accessor methods when people should have?), or are your concerns mainly that it looks odd and might be used inconsistently?

[dodgex]

Thank you for your quick answer.

When you have your record as top-level class, no other code can access the field directly, as they are considered private to the record. So the accessor method is required in those cases anyway, that would include custom overrides as in your array example.

The methods in a record can access fields directly and should be allowed to do so, even with the proposed check. Only outside-access to the fields should be checked; and this would only apply to classes where the record is nested into.

record OuterRecord(String name, int age) {

	@Override
	public int age() {
		return age + name.length(); // fine as it is inside the record
	}
}

class Parent {

	void doSomething() {
		InnerRecord inner = new InnerRecord("John Doe", 21);
		System.out.println(inner.name); // fine for javac; should be prohibited by this check
		System.out.println(inner.name()); // OK
		System.out.println(inner.age()); // OK

		OuterRecord outer = new OuterRecord("Jane Doe", 21);
		System.out.println(outer.name); // javac error: 'name' has private access in 'OuterRecord'
		System.out.println(outer.name()); // OK
		System.out.println(outer.age()); // OK
	}

	private record InnerRecord(String name, int age) {

		@Override
		public int age() {
			return age + name.length(); // fine as it is inside the record
		}
	}
}

Other than inconsistency between usage in nested and top-level classes i see close to no harm. Possibly only if one opts to always use the fields when possible (due to the record beeing nested) but the record is expected to be used with accessors, e.g. for array cloning or whatever else might be implemented in a custom accessor.

For your array exaple, one could override the default accessor with clone, and have/need a extra accessor for unsafe access. (ignoring the fact that arrays should not be used in records and that this not prevents any other code to access the array using the unsafe accessor)

record ArrayRecord(String[] arr) {

	@Override
	public String[] arr() {
		return arr.clone();
	}

	public String[] arrUnsafe() {
		return arr;
	}
}

I can totally see, that this kind of usage might indeed be an edge case and that it might not necessarily be worth a check in error-prone itself.