add haveOnlyFinalNonSyntheticFields() to ignore synthetic fields

[sharanggupta]

Some compilers create non-final synthetic fields, e.g. the Eclipse compiler generates a $SWITCH_TABLE$... field for every switch over an enum. classes().should().haveOnlyFinalFields() reported such fields as violations although they do not appear in the source code and cannot be declared final by the developer (see the discussion in the issue, where ignoring synthetic members in predefined predicates was suggested).

Keep haveOnlyFinalFields() exactly as it is and add haveOnlyFinalNonSyntheticFields() to ArchConditions and ClassesShould as an explicit alternative that ignores fields with modifier SYNTHETIC.
Link the alternative from the Javadoc of haveOnlyFinalFields().

Initial plan (discarded)

This PR excludes fields with modifier SYNTHETIC from haveOnlyFinalFields() only, leaving haveOnlyPrivateConstructors() and all other conditions untouched, since excluding synthetic members globally has proven problematic in the past. The Javadoc of ClassesShould.haveOnlyFinalFields() and ArchConditions.haveOnlyFinalFields() documents the behaviour. If you would rather have the filter inside the shared HaveOnlyModifiersCondition, I am happy to move it.

As javac never emits a non-final synthetic field (the $SWITCH_TABLE$ field is compiler specific), the regression test generates a class file with such a field via ASM and imports it with the ClassFileImporter, asserting that only the regular non-final field is reported.

Verified with :archunit:test (15804 tests), spotlessCheck, architectureTest and spotbugsMain.

Resolves #1313

[hankem]

First of all:

Thank you for your contribution! 💙

My thoughts about adapting haveOnlyFinalFields

I'm always a bit sceptical about implicit behavior designed to anticipate users' expectations.

In my opinion, a variant of ArchUnit that does exactly what it says ("should().haveOnlyFinalFields() checks for final fields and nothing else") is much easier to understand than one that has inconsistent little side effects here and there. (Why does haveOnlyFinalFields ignore synthetic fields, but other predicates and conditions do not? What can users intuitively rely on without validating every assumption in the documentation, which could be incomplete, or even the source code?)

Furthermore, our assumptions about users' expectations could just be wrong. I agree that many users may indeed not know about synthetic members generated by their Java compilers, but what if some other (future) language that compiles to JVM bytecode has constructions where users suddenly want to distinguish final synthetic fields (Your ClassWriter test can show that it's technically possible.) from non-final synthetic fields? (This example may be far-fetched; it is more meant to make the general point.)

Explicit is better than implicit.

In my opinion, the behavior is not a real problem: Users discover the existence of synthetic fields, and will eventually find out that they can replace classes().should().haveOnlyFinalFields() with some kind of classes().should(haveOnlyFinalOrSyntheticFields()).

haveOnlyFinalOrSyntheticFields could be defined by users themselves, but ArchUnit could of course also support it, e.g. in ArchConditions and ClassesShould, and with links in the documentation of haveOnlyFinalFields.
I don't think that adding some more specialized APIs would jeopardise long-term maintainability.

What do you (or others) think?

[sharanggupta]

@hankem — Thanks for the feedback! You make a great point about explicit vs implicit behavior. I actually agree that having inconsistent little side effects scattered through the API isn't great for maintainability.

I've updated the PR to take your suggested approach:

haveOnlyFinalFields() now does exactly what it says — checks for final fields, no hidden filtering
Added haveOnlyFinalFieldsOrSynthetic() as an explicit opt-in for users who want to ignore compiler-generated fields

Users who hit synthetic field violations can now just switch to the new method if they need that behavior. Makes it clear in the code what's going on instead of burying it in the docs.

The tests still cover both cases (that haveOnlyFinalFields reports synthetic violations, and that the new method ignores them), so the regression coverage is there.

All 15k+ tests pass. Ready for another look whenever you get a chance!

[hankem]

Did you pick the name haveOnlyFinalFieldsOrSynthetic to have it right next to haveOnlyFinalFields?
I'd think the natural language pattern would otherwise be "final or synthetic fields".

[sharanggupta]

Good point, I agree: haveOnlyFinalOrSyntheticFields() follows the natural language order and is also the name you suggested in your first comment. I renamed it in ArchConditions, ClassesShould and ClassesShouldInternal, so the rule text now reads "classes should have only final or synthetic fields" (asserted in the test). I also added a note with a link to the new method in the Javadoc of haveOnlyFinalFields(), as you proposed. The full :archunit:test, spotlessCheck, architectureTest and spotbugsMain run is green.

[hankem]

Or another thought, since I realized that FINAL and SYNTHETIC are not handled symmetrically
(synthetic fields aren't considered; they don't end up in a violation event's correspondingObjects):
Should the method be called haveOnlyFinalNonSyntheticFields?

Sorry for the back and forth... 🙈

[hankem]

Wouldn't we expect

Suggested change

	.hasOnlyViolations(String.format("Field <%s.%s> is not final in (%s.java:0)",
	.hasOnlyViolations(String.format("Field <%s.%s> is not final and not synthetic in (%s.java:0)",

in this case?

[hankem]

Or maybe

Suggested change

	.hasOnlyViolations(String.format("Field <%s.%s> is not final in (%s.java:0)",
	.hasOnlyViolations(String.format("Non-synthetic field <%s.%s> is not final in (%s.java:0)",

[hankem]

Ah, I see:
It's tricky because we're still using the HaveOnlyModifiersCondition effectively using be(modifier(FINAL)).
Maybe it's not worth to mention synthetic fields in the violation message.
I'm just thinking out loud...

[hankem]

A most minor suggeestion. just because I saw it (i.e. feel free to ignore 😅):
If you wanted to have this more concise, you could rearrange it to

Suggested change

	assertThatRule(rule).hasDescriptionContaining("classes should have only final or synthetic fields");
	JavaClasses classes = GeneratedClassWithNonFinalSyntheticField.importIntoArchUnit();
	assertThatRule(rule).checking(classes)
	assertThatRule(rule)
	.hasDescriptionContaining("classes should have only final or synthetic fields")
	.checking(GeneratedClassWithNonFinalSyntheticField.importIntoArchUnit())

[hankem]

We could keep the imports in alphabetical order. 😅

Suggested change

	import static com.tngtech.archunit.core.domain.JavaClass.Functions.GET_FIELDS;
	import static com.tngtech.archunit.core.domain.JavaClass.Functions.GET_DIRECT_DEPENDENCIES_FROM_SELF;
	import static com.tngtech.archunit.core.domain.JavaClass.Functions.GET_DIRECT_DEPENDENCIES_TO_SELF;
	import static com.tngtech.archunit.core.domain.JavaClass.Functions.GET_FIELDS;

[hankem]

The DCO check complains about the second commit's missing sign-off, but I'd anyway ask you to squash them together. We don't have to preserve the back and forth (partly due to my undecisiveness; sorry!) in the history...

[sharanggupta]

Thanks @hankem, I incorporated all of it and squashed everything into a single signed-off commit, rebased on current main:

• renamed to haveOnlyFinalNonSyntheticFields() (ArchConditions, ClassesShould, ClassesShouldInternal); the rule text follows the method name, as RandomClassesSyntaxTest expects: "classes should have only final non synthetic fields";
• haveOnlyFinalFields() links to the new method in its Javadoc;
• imports in alphabetical order and the assertions chained as you suggested;
• violation message left as it is, as you concluded.

The full :archunit:test (15,894 tests), spotlessCheck, architectureTest and spotbugsMain pass locally. The CI workflow needs an approval to run on the new commit whenever you have a moment.

[hankem]

Thank you for your contribution to ArchUnit!