There is a widespread practice of avoiding unnecessarily exposing your synchronization locks, i.e. using synchronized(myPrivateField) instead of public synchronized void myMethod(). (For background, see e.g. this SO question or the rationale for using a related Lombok feature.)
I think ArchUnit could be used nicely to write rules around this, for example:
regarding "don't expose your locks":
"no method should have the synchronized modifier" (already possible in ArchUnit today)
"no synchronized block should use this"
"fields used for synchronized blocks should neither be public nor have public getters"
regarding "don't lock on the wrong stuff":
"no synchronized block should use a field of another class or a result returned from a method of another (library) class"
As far as I can see, however, ArchUnit does not model the existence of synchronized blocks or which locks they use.
Now of course there are static code analyzers that do some of this (e.g. Error Prone with its @GuardedBy annotation). Still, I believe it's worthwhile to support this concept in ArchUnit for the following reasons:
Some locks are shared intentionally, e.g. as described in the javadoc for Collections.synchronizedMap(). With ArchUnit, a team could easily write their own annotation, e.g. @ReturnValueSuitableForLocking, and have their rules make an exception for those of their methods that have it.
Other tools don't have a notion of architecture layers, whereas in ArchUnit it would be possible to implement a rule like "shared locks can only be used within the layer they are defined in, not in other/upper layers".
"Stupid" reason: introducing a static code analyzer (or any new dependency, for that matter) can be cumbersome, controversial, or both. If ArchUnit is already in place, it could be used as a) a permanent workaround for the lack of specialized tooling or b) as a proof of concept that checking for those code smells is valuable and specialized tooling should be introduced.
So, could this be added?
Note: in theory, it would also be possible to model what accesses are done inside a synchronized block, enabling rules similar to what ErrorProne does (see link above). IMO this is not required. However, when refactoring the model now in order to add the minimal feature, one should probably take care to not create any obstacles for a future addition of the larger feature.
There is a widespread practice of avoiding unnecessarily exposing your synchronization locks, i.e. using
synchronized(myPrivateField)instead ofpublic synchronized void myMethod(). (For background, see e.g. this SO question or the rationale for using a related Lombok feature.)I think ArchUnit could be used nicely to write rules around this, for example:
synchronizedmodifier" (already possible in ArchUnit today)this"As far as I can see, however, ArchUnit does not model the existence of
synchronizedblocks or which locks they use.Now of course there are static code analyzers that do some of this (e.g. Error Prone with its
@GuardedByannotation). Still, I believe it's worthwhile to support this concept in ArchUnit for the following reasons:Collections.synchronizedMap(). With ArchUnit, a team could easily write their own annotation, e.g.@ReturnValueSuitableForLocking, and have their rules make an exception for those of their methods that have it.So, could this be added?
Note: in theory, it would also be possible to model what accesses are done inside a synchronized block, enabling rules similar to what ErrorProne does (see link above). IMO this is not required. However, when refactoring the model now in order to add the minimal feature, one should probably take care to not create any obstacles for a future addition of the larger feature.