Annotations are at wrong abstraction levels

[Ambeco]

The idea behind the annotations is neat, but they are much stricter than makes sense, which requires even correct usage to add a lot of suppressions.

1. @Cache is not a valid abstraction in a purity checker. Instead, this concept should be replaced with @GuardedBy or some sort of @ThreadSafeObject annotation for properties or parameters.
2. I should be able to mark a class as @Immutable as well.
3. @InternalState does not make sense in a multi-threaded context. It needs to be replaced with @GuardedBy or some sort of @ThreadSafeObject.
4. @LocalState should not be needed. Any qualification should be enforced by properties that locals are assigned to, or by parameters that it's passed as. This is especially problematic for values created in a more complex expression, like builders. Right now, I have to create a builder, assign it to a @LocalState, and THEN use it to build. This is also especially problematic for caught exceptions, which appear impossible to annotate with any of these annotations, or even suppressed, without suppressing the entire method.
5. There are a many methods in the Java library, and a LOT of methods in Kotlin, that are not marked as @Pure or @Readonly that ought to be. Especially in the area of collections/sequences/streams/iterators:
   "java.util.stream.StreamSupport.longStream",
   "java.util.stream.LongStream.parallel",
   "java.util.LinkedHashMap.forEach",
   "java.util.LinkedHashMap.computeIfAbsent",
   "kotlin.LongArray.get",
   "kotlin.LongArray.iterator",
   "kotlin.collections.copyOf", *
   "kotlin.collections.copyInto", *
   "kotlin.collections.firstNotNullOfOrNull",
   "kotlin.collections.slice",
   "kotlin.collections.IntIterator.hasNext",
   "kotlin.collections.IntIterator.next",
   "kotlin.sequences.firstNotNullOfOrNull",
   "kotlin.sequences.minWithOrNull",
   "kotlin.collections.copyInto", *
6. Note that three of those modify the parameter. Which reveals that the library needs an annotation for function parameters that signifies that the method mutates the parameter, but does not expose it outside of the method, so it doesn't violate purity when called on local or @GuardedBy variables, or Thead-Safe properties. Very similar to the current@LocalState actually. Suggested name: @Mutates.
7. My experience is that when devs think of a class as thread-safe, they tend to stop thinking about thread safety, and write code that assumes the data does not mutate in parallel. (The iconic example is ConcurrentHashMap) So this is why ThreadSafe annotations like @Cache and @InternalState have been problematic. I think this might be what to aim for:
   • Methods can be marked as either @ThreadSafeUnconditionalWrite (ex: AtomicInt#clear) or @ThreadSafeCas (ex: AtomicInt#compareAndSwap) or @Readonly (ex: AtomicInt#get).
   • A method that calls a @ThreadSafeX method should itself be marked @ThreadSafeRead("property_or_parameter_name") or @ThreadSafeWrite("property_or_parameter_name").
   • A method that calls a @ThreadSafeUnconditionalWrite method may not call any other methods on that object BEFORE the call, even if those methods are @ThreadSafeX.
   • (Longer term) A method that calls a @ThreadSafeX method may not call any other @ThreadSafeX methods for that property, nor on this.
   • These rules do not guarantee correctness, but I believe they'll make it more obvious to developers how thread safe code should look. And discourage ignoring thread safety simply because something is a @Cache or @InternalState.

Examples:

class WorkerAutomation {
    @ThreadSafeObject private val tileRankings = HashMap<Tile, TileImprovementRank>()

@ThreadSafeRead("tileRankings")
private fun startWorkOnCurrentTile(unit: MapUnit) {
        val currentTile = unit.currentTile
        val tileRanking = tileRankings[currentTile]!! // allowed because it only calls @Readonly
}

    @Readonly
    @ThreadSafeWrite("tileRankings")
    fun getBasePriority(tile: Tile, unit: MapUnit): UnitPriority {
        val unitSpecificPriority = 2 - (tile.aerialDistanceTo(unit.getTile()) / 2.0f).coerceIn(0f, 2f) * 3
       // if (tileRankings.containsKey(tile))
        //    return tileRankings[tile]!!.tilePriority + unitSpecificPriority // error: multiple operations on ThreadSafe
       val ranking= tileRankings.compute(tile, {tile2,ranking->  // instead: replace with one @CompareAndSwap call
           if (ranking != null) return ranking
        ... // most of existing body here
       }
       return UnitPriority(ranking, unitSpecificPriority)
}
    @Readonly
    private fun getImprovementPriority(tile: Tile, unit: MapUnit, localUniqueCache: LocalUniqueCache): ImprovementPriotity {
        val unitPriority = getBasePriority(tile, unit)
   //  @LocalState val rank = tileRankings[tile] // error: multiple operations on ThreadSafe
        val rank = unitPriority.ranking
        ... // reset of existing body here
}

    @Readonly
    private fun tileHasWorkToDo(tile: Tile, unit: MapUnit, localUniqueCache: LocalUniqueCache): Boolean {
        // if (getImprovementPriority(tile, unit, localUniqueCache) <= 0) return false
        // if (!(tileRankings[tile]!!.bestImprovement != null || tileRankings[tile]!!.repairImprovment!!))
        // error: multiple operations on ThreadSafe
        val improvementPriority= getImprovementPriority(tile, unit, localUniqueCache)
        if (improvementPriority.priority  <= 0) return false
        val rank =improvementPriority.ranking 
        val bestImprovement = improvementPriority.
        if (!(rank.bestImprovement != null || rank.repairImprovment!!))
            throw IllegalStateException("There was an improvementPriority > 0 and nothing to do")
        return true
    }
}

[Ambeco]

It just occured to me that if the parameter/property was marked AppendOnly in some way, to indicate that entries once set are never unset, which would allow read-before-CAS.
And if the entries were also marked Immutable in some way, that would allow read-after-write.

But this indicates I'm also thinking at the wrong abstraction level.
Apparently the right abstraction level indicates if values can be updated or deleted, and restricts reads and writes accordingly.

[yairm210]

1. @cache comes to solve a specific problem, to wit: When you call a readonly/pure function you sometimes want to cache the value locally. Externally you won't be able to tell that this internal caching exists, but it does technically alter state, hence it needs a special escape hatch. GuardedBy and ThreadSafeObject are not solutions for "this is supposed to not alter state but does for technical caching reasons" 🤔

2. "Immutable" is not the correct term for a class, since a classes immutability does not necessarily guarantee that all of its functions are pure or indeed readonly. An immutable class can 100% have a function that gets a list and mutates it, so the proper place to mark things is on the function level.

3. This goes against our principle of allowing annotations to be added to code without changing the code

4. In theory, sure. In practice, I have not found a way to propagate state-safetly across the AST in any meaningful way to allow me to get rid of it. Definitely accepting changes that would allow us expand autorecognition of local state :)

5. Definitely, these are only what I managed to catch so far, will add what you mentioned and am accepting PRs :)

6. I'm not sure what validation we're taking about here. If a function mutates an input parameter, then it's neither pure nor readonly, so what exactly are we validating? "Inputs marked with mutates have non-readonly functions called"?

7. I'm not sure what problem you're trying to solve here, to be honest - Readonly means that it reads mutatable state, so it's liable to thread changes - that's the main thing that makes it differ from Pure which reads non-mutatable state. Are you saying that Readonly isn't explicit enough in its thread-unsafety-ness?