IDEA-147530 ModifiableModuleModel#newModule should always create a new module from scratch

platform/configuration-store-impl/src/FileBasedStorage.kt

@@ -58,6 +58,8 @@ open class FileBasedStorage(file: Path,
   private var lineSeparator: LineSeparator? = null
   private var blockSavingTheContent = false
 
+  var resolveVirtualFileOnlyOnWrite = false
+
   @Volatile var file = file
     private set
 
@@ -120,7 +122,7 @@ open class FileBasedStorage(file: Path,
         return loadLocalDataUsingIo()
       }
 
-      val file = virtualFile
+      val file = if (resolveVirtualFileOnlyOnWrite) cachedVirtualFile else virtualFile
       if (file == null || file.isDirectory || !file.isValid) {
         LOG.debug { "Document was not loaded for $fileSpec, not a file" }
       }

platform/configuration-store-impl/src/ModuleStoreImpl.kt

@@ -71,6 +71,11 @@ abstract class ModuleStoreBase : ComponentStoreImpl() {
       return
     }
 
-    (storageManager.getOrCreateStorage(StoragePathMacros.MODULE_FILE) as FileBasedStorage).setFile(file, null)
+    storageManager.getOrCreateStorage(StoragePathMacros.MODULE_FILE, storageCustomizer = {
+      (this as FileBasedStorage).setFile(file, null)
+      // ModifiableModuleModel#newModule should always create a new module from scratch
+      // https://youtrack.jetbrains.com/issue/IDEA-147530
+      resolveVirtualFileOnlyOnWrite = true
+    })
   }
 }

platform/configuration-store-impl/src/StateStorageManagerImpl.kt

@@ -148,11 +148,13 @@ open class StateStorageManagerImpl(private val rootTagName: String,
     return if (path.endsWith('/')) path.substring(0, path.length - 1) else path
   }
 
+  // storageCustomizer - to ensure that other threads will use fully constructed and configured storage (invoked under the same lock as created)
   fun getOrCreateStorage(collapsedPath: String,
                          roamingType: RoamingType = RoamingType.DEFAULT,
                          storageClass: Class<out StateStorage> = StateStorage::class.java,
                          @Suppress("DEPRECATION") stateSplitter: Class<out StateSplitter> = StateSplitterEx::class.java,
-                         exclusive: Boolean = false): StateStorage {
+                         exclusive: Boolean = false,
+                         storageCustomizer: (StateStorage.() -> Unit)? = null): StateStorage {
     val normalizedCollapsedPath = normalizeFileSpec(collapsedPath)
     val key: String
     if (storageClass == StateStorage::class.java) {
@@ -166,7 +168,11 @@ open class StateStorageManagerImpl(private val rootTagName: String,
     }
 
     return storageLock.read { storages.get(key) } ?: storageLock.write {
-      storages.getOrPut(key, { createStateStorage(storageClass, normalizedCollapsedPath, roamingType, stateSplitter, exclusive) })
+      storages.getOrPut(key) {
+        val storage = createStateStorage(storageClass, normalizedCollapsedPath, roamingType, stateSplitter, exclusive)
+        storageCustomizer?.let { storage.it() }
+        storage
+      }
     }
   }
 

platform/configuration-store-impl/testSrc/ModuleStoreTest.kt

@@ -77,6 +77,16 @@ class ModuleStoreTest {
     }
   }
 
+  @Test fun `newModule should always create a new module from scratch`() {
+    val moduleFile = runWriteAction {
+      VfsTestUtil.createFile(tempDirManager.newVirtualDirectory("module"), "test.iml", "<module type=\"JAVA_MODULE\" foo=\"bar\" version=\"4\" />")
+    }
+
+    Paths.get(moduleFile.path).createModule().useAndDispose {
+      assertThat(getOptionValue("foo")).isNull()
+    }
+  }
+
   @Test fun `must be empty if classpath storage`() {
     // we must not use VFS here, file must not be created
     val moduleFile = tempDirManager.newPath("module", refreshVfs = true).resolve("test.iml")