Skip to content

JBRes-8425: Add memory for renaming transformations#41

Open
dragoi75 wants to merge 8 commits intomainfrom
adragoi/feature/rename-transformation-memory
Open

JBRes-8425: Add memory for renaming transformations#41
dragoi75 wants to merge 8 commits intomainfrom
adragoi/feature/rename-transformation-memory

Conversation

@dragoi75
Copy link
Collaborator

@dragoi75 dragoi75 commented Mar 4, 2026

Changes

  • Addition of RenameMemory
    • Responsible for loading / writing the memory file.
    • Creates one memory file per project.
  • Addition of MemoryAwareTransformation:
    • Extends SelfManagedTransformation.
    • Acts as middleware between the transformations and RenameMemory
  • Addition of PsiSignatureGenerator`
    • Generates a unique signature for (a subset of) PSI elements.
  • Modifications to the three RenameTransformations to use memory.

Design questions

  1. For now, the useMemory is an individual config under the transformation. Should we make it a global flag?

@dragoi75 dragoi75 requested a review from Vladislav0Art March 4, 2026 13:34
@github-actions
Copy link

github-actions bot commented Mar 4, 2026

Qodana Community for JVM

1 new problem were found

Inspection name Severity Problems
Leaking 'this' in constructor 🔶 Warning 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/qodana-action@v2025.1.1
        with:
          upload-result: true
Contact Qodana team

Contact us at qodana-support@jetbrains.com

Copy link
Collaborator

@Vladislav0Art Vladislav0Art left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a major comment about the selected architecture and a better alternative: here.

// Use the project's base path to derive a meaningful name
val projectName = project.basePath?.let { File(it).name } ?: project.name

RenameMemory(projectName).also { memory ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can reduce the control-flow complexity:

val memory =  RenameMemory(projectName)
// all operations you do in `also` block.

imho, let is semantically more suitable here.

Comment on lines +69 to +78
protected fun getOrCreateMemory(project: Project): RenameMemory? {
// Return the cached instance if already initialized
if (cachedMemory != null) {
return cachedMemory
}

// Initialize memory for the first time
cachedMemory = try {
// Use the project's base path to derive a meaningful name
val projectName = project.basePath?.let { File(it).name } ?: project.name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NOTE, no modifications requested]

This entire method can be reduced to a lazy block over the cachedMemory instance.

private val cachedMemory: RenameMemory = lazy {
   // TODO: need access to a `project` instance.
}

Comment on lines +128 to +129
return emptyList()
}
Copy link
Collaborator

@Vladislav0Art Vladislav0Art Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all places where you return emptyList in this getNameSuggestions function, you should return generateNewClassNames(psiClass).

Otherwise, this solution doesn't map all valid inputs into a successful rename (although the previous solution without any memory would).

Comment on lines +129 to +137
private suspend fun getNameSuggestions(method: PsiMethod, memory: RenameMemory?): List<String> {
if (useMemory) {
val signature = IntelliJAwareTransformation.withReadAction {
PsiSignatureGenerator.generateSignature(method)
}
if (signature == null) {
logger.warn("Could not generate signature for method ${IntelliJAwareTransformation.withReadAction { method.name }}")
return emptyList()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, also. Not emptyList but the implementation generateNewMethodNames.

Comment on lines +129 to +135
private suspend fun getNameSuggestions(method: PsiMethod, memory: RenameMemory?): List<String> {
if (useMemory) {
val signature = IntelliJAwareTransformation.withReadAction {
PsiSignatureGenerator.generateSignature(method)
}
if (signature == null) {
logger.warn("Could not generate signature for method ${IntelliJAwareTransformation.withReadAction { method.name }}")
Copy link
Collaborator

@Vladislav0Art Vladislav0Art Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at these getNameSuggestions(psiComponent, memory), it's apparent that you can reduce it into a "cached-or-call-method" semantic:

[suspend] fun <R> Memory.cachedOrCall(signature: String?, callback: [suspend] () -> R): R {
    val memory = this
    if (signature != null && memory.has(signature)) return memory.getValue(signature)
    
    // otherwise, when missing, execute a callback
    val result = callback()
    if (signature != null) memory.store(signature, result)
    return result
}

// How To Use:
// inside your transformation
val methodSignature = readAction { SignatureGenerator.signatureOf(psiMethod) }

val suggestions = memory.cachedOrCall(methodSignature) {
    suggestNewMethodNamesByAI(/* whatever */)
}

return suggestions

This way, your memory can be used not only for renames, but for everything; which is needed by my PR as well.

Comment on lines +162 to +163
@Serializable
data class RenameMemoryFile(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. private data class?

  2. It's not really a file but rather a state of the memory: A file (i.e., the fle system itself) is an implementation detail used for persistent saving (kinda one of many options where/how to save a memory state); it could have been a database also). I'd rename to MemoryState or alike.

Comment on lines +130 to +147
companion object {
private const val MEMORY_DIR = ".codecocoon-memory"

/**
* The directory where memory files are stored.
* Defaults to the CodeCocoon-Plugin root directory.
*/
private val memoryBaseDir: File by lazy {
// Get the codecocoon.config system property path and resolve the memory directory relative to it
val configPath = System.getProperty("codecocoon.config")
val baseDir = if (configPath != null) {
File(configPath).parentFile
} else {
// Fallback to current working directory if property not set
File(System.getProperty("user.dir"))
}
File(baseDir, MEMORY_DIR)
}
Copy link
Collaborator

@Vladislav0Art Vladislav0Art Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ This hard-coded setup of memoryBaseDir makes the save location very rigid.

It should be a caller's responsibility to define where this memory instance saves its data.

I'd place this folder selection logic somewhere to the upper layer of the project (HeadlessStarter or ExecutionService), and make the memoryBaseDir a constructor parameter of a memory instance.


I'd make it optionally parameterizable from the CLI: Useful for the eval pipeline to decide where to store the memory (i.e., creating and parsing this filepath somewhere in the HeadlessStarter).

Comment on lines +10 to +19
/**
* Manages persistent storage of rename operations to enable deterministic transformations.
*
* Memory files are stored in the CodeCocoon-Plugin directory under `.codecocoon-memory/`
* and are organized by project name to allow tracking multiple projects independently.
*/
class RenameMemory(private val projectName: String) {

private val logger = thisLogger().withStdout()

Copy link
Collaborator

@Vladislav0Art Vladislav0Art Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗️MAJOR API Concern about RenameMemory and MemoryAwareTransformation ❗️

This comment addresses some architectural caveats of how memory API is current integrated with transformations.


You don't need this RenameMemory class be rename-specific; in fact, it doesn't need to know what type of information it stores whatsoever.

What you have is a general-purpose interface of any persistent storage, like Redis/Database or even a simple in-memory Map<String, String>.

I suggest turning this API into an interface Memory with a single implementation that stores entries under a filepath given by the caller:

// you only need these 4 methods (maybe you don't even need `size` method)
interface Memory<K, V> {
    fun get(key: K): V?
    // returns the previous value if present, otherwise `null`
    fun put(key: K, value: V): V?
    fun dump(): Unit // or `save`
    fun size(): Int/Long
}

Even better approach is to make it AutoCloseable, since you always need to "save" your memory once you're done with it. Do it as:

interface Memory<K, V> : AutoCloseable {
    // ... as above ...

    fun close() {
     // we kinda know how to close it already:
     this.save() // or this.dump()
    }
}

Given this interface, you write an implementation that stores data on disk:

class PersistentProjectMemory(
   filepath: String, // base directory where to store/from where to load the JSON file with cached entries
   projectName: String,
) : Memory<String, String> {
     // ...
}

This gives you a high level of freedom of how to integrate this memory with transformations; you can:

  1. Create a single globally defined Memory instance shared by all transformations (somewhere in the TransformationService or HeadlessStarter).
  2. Create a memory instance for every transformation applied (I think this is the approach you currently have with MemoryAwareTransformation).

Next, you can create a memory instance either project-wide or per-transformation in the TransformationService:

// inside `TransformationService`

class TransformationService {

   fun applyTransformations() {

         // ...
         // (1): define a memory instance here -> you get a single memory for an entire project under transformation
         val globalMemory = PersistentProjectMemory(baseDir, projectName)

         for (tr in transformations) {
             // (2): define memory instance here -> you get a per-transformation memory instance
             val memory = PersistentProjectMemory(baseDir, projectName)
             try {
                 executor.execute(tr, context, memory)
             } finally {
                  // IMPORTANT: we successfully "close" the resource, i.e. dump data on disk/save in the db or Redis, etc.
                  memory.close() // aka `memory.save()/memory.dump()`
             }
         }

    }
}

As for the transformations (namely, IntelliJAwareTransformation interface), I'd simply add another parameter memory: Memory into apply method. The transformations that can benefit from memory, will do so; otherwise, it'll be a no-op parameter, which is fine for us.


Problems with MemoryAwareTransformation:

  1. It removes the freedom of selecting a scope for your memory (i.e., you always have it per-transformation; you cannot make it project-scoped, or place anywhere else). This is because MemoryAwareTransformation uses inheritance, which in this case is inferior to the compositional approach.
  2. It tightly couples transformation and persistent memory.
  3. Besides, we spawned quite a lot of transformation-related classes; making it even larger hinders maintainability.

In other words, the main problem with MemoryAwareTransformation is that it uses inheritance and couples memory and transformations way too strongly.

Comment on lines +20 to +22
private val memoryFile: File
private var memoryData: RenameMemoryFile

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you can have both as val:

private val memoryFile: File = run {
        // Sanitize project name for use in filename
        val sanitizedName = sanitizeProjectName(projectName)

        // Ensure memory directory exists
        if (!memoryBaseDir.exists()) {
            memoryBaseDir.mkdirs()
        }

        memoryFile = File(memoryBaseDir, "$sanitizedName.json")
}

private val memoryData: RenameMemoryFile = loadFileFromDisk(memoryFile)



// where
companion object {
     fun loadFileFromDisk(from: File) { ... }
}

Comment on lines +120 to +129
/**
* Returns the number of entries currently in memory.
*/
fun size(): Int = memoryData.entries.size

/**
* Returns the path to the memory file.
*/
fun getMemoryFilePath(): String = memoryFile.absolutePath

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use these methods once for logging only. Do you need them in public API then? you can log in the init block as well.

Especially getMemoryFilePath: it is an implementation detail; callers should not know how the Memory class manages its storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants