From 5b8e58fe361af4c0d1e96495c152684398f12f4d Mon Sep 17 00:00:00 2001 From: Alexander Udalov Date: Fri, 7 Jul 2017 12:45:29 +0300 Subject: [PATCH] Implement module accessibility checks differently in compiler and IDE Essentially, the logic that was previously in JvmModuleAccessibilityChecker.diagnosticFor, is moved into a new abstract method JavaModuleResolver.checkAccessibility, which is implemented differently in the compiler and in the IDE. In the compiler, we use our JavaModuleInfo and JavaModuleGraph, as previously. In the IDE, we use intellij's PsiJavaModule and JavaModuleGraphUtil. This fixes strange behavior in IDE where some modules could be observed in an invalid state. The cause of that was the JavaModuleGraph instance caching modules in IdeJavaModuleResolver, which is a project component. Moreover, this will allow to report an error "named module does not read unnamed module" in the compiler, and avoid reporting it in the IDE (see the comment in IdeJavaModuleResolver about that) --- .../cli/jvm/modules/CliJavaModuleResolver.kt | 29 +++++++- .../checkers/JvmModuleAccessibilityChecker.kt | 41 ++++------- .../diagnostics/DefaultErrorMessagesJvm.java | 1 + .../resolve/jvm/diagnostics/ErrorsJvm.java | 1 + .../resolve/jvm/modules/JavaModuleResolver.kt | 10 ++- .../idea/modules/IdeJavaModuleResolver.kt | 68 +++++++++++++------ 6 files changed, 97 insertions(+), 53 deletions(-) diff --git a/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/modules/CliJavaModuleResolver.kt b/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/modules/CliJavaModuleResolver.kt index 7b4d0c52ef1..f3789373ec8 100644 --- a/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/modules/CliJavaModuleResolver.kt +++ b/compiler/cli/src/org/jetbrains/kotlin/cli/jvm/modules/CliJavaModuleResolver.kt @@ -21,12 +21,13 @@ import com.intellij.ide.highlighter.JavaFileType import com.intellij.openapi.vfs.VfsUtilCore import com.intellij.openapi.vfs.VirtualFile import org.jetbrains.kotlin.idea.KotlinFileType +import org.jetbrains.kotlin.name.FqName import org.jetbrains.kotlin.resolve.jvm.modules.JavaModule import org.jetbrains.kotlin.resolve.jvm.modules.JavaModuleGraph import org.jetbrains.kotlin.resolve.jvm.modules.JavaModuleResolver class CliJavaModuleResolver( - override val moduleGraph: JavaModuleGraph, + private val moduleGraph: JavaModuleGraph, private val javaModules: List ) : JavaModuleResolver { init { @@ -37,7 +38,7 @@ class CliJavaModuleResolver( private val sourceModule = javaModules.firstOrNull { !it.isBinary } - override fun findJavaModule(file: VirtualFile): JavaModule? = + private fun findJavaModule(file: VirtualFile): JavaModule? = when (file.fileType) { KotlinFileType.INSTANCE, JavaFileType.INSTANCE -> sourceModule JavaClassFileType.INSTANCE -> javaModules.firstOrNull { module -> @@ -45,4 +46,28 @@ class CliJavaModuleResolver( } else -> null } + + override fun checkAccessibility( + fileFromOurModule: VirtualFile?, referencedFile: VirtualFile, referencedPackage: FqName? + ): JavaModuleResolver.AccessError? { + val ourModule = fileFromOurModule?.let(this::findJavaModule) + val theirModule = this.findJavaModule(referencedFile) + + if (ourModule?.name == theirModule?.name) return null + + if (theirModule == null) { + return JavaModuleResolver.AccessError.ModuleDoesNotReadUnnamedModule + } + + if (ourModule != null && !moduleGraph.reads(ourModule.name, theirModule.name)) { + return JavaModuleResolver.AccessError.ModuleDoesNotReadModule(theirModule.name) + } + + val fqName = referencedPackage ?: return null + if (!theirModule.exports(fqName) && (ourModule == null || !theirModule.exportsTo(fqName, ourModule.name))) { + return JavaModuleResolver.AccessError.ModuleDoesNotExportPackage(theirModule.name) + } + + return null + } } diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmModuleAccessibilityChecker.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmModuleAccessibilityChecker.kt index e67480d9c1a..80f563f83f1 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmModuleAccessibilityChecker.kt +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/checkers/JvmModuleAccessibilityChecker.kt @@ -34,8 +34,9 @@ import org.jetbrains.kotlin.resolve.calls.checkers.CallChecker import org.jetbrains.kotlin.resolve.calls.checkers.CallCheckerContext import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall import org.jetbrains.kotlin.resolve.checkers.ClassifierUsageChecker -import org.jetbrains.kotlin.resolve.jvm.diagnostics.ErrorsJvm +import org.jetbrains.kotlin.resolve.jvm.diagnostics.ErrorsJvm.* import org.jetbrains.kotlin.resolve.jvm.modules.JavaModuleResolver +import org.jetbrains.kotlin.resolve.jvm.modules.JavaModuleResolver.AccessError.* import org.jetbrains.kotlin.resolve.source.getPsi import org.jetbrains.kotlin.serialization.deserialization.descriptors.DeserializedMemberDescriptor @@ -61,33 +62,21 @@ class JvmModuleAccessibilityChecker(project: Project) : CallChecker { ): Diagnostic? { val referencedFile = findVirtualFile(targetClassOrPackage, originalDescriptor) ?: return null - // TODO: do not resolve our JavaModule every time, invent a way to obtain it from ModuleDescriptor and do it once in constructor - val ourModule = fileFromOurModule?.let(moduleResolver::findJavaModule) - val theirModule = moduleResolver.findJavaModule(referencedFile) + val referencedPackageFqName = + DescriptorUtils.getParentOfType(targetClassOrPackage, PackageFragmentDescriptor::class.java, false)?.fqName + val diagnostic = moduleResolver.checkAccessibility(fileFromOurModule, referencedFile, referencedPackageFqName) - // If we're both in the unnamed module, it's OK, no error should be reported - if (ourModule == null && theirModule == null) return null - - if (theirModule == null) { - // We should probably prohibit this usage according to JPMS (named module cannot use types from unnamed module), - // but we cannot be sure that a module without module-info.java is going to be actually used as an unnamed module. - // It could also be an automatic module, in which case it would be read by every module. - return null + return when (diagnostic) { + is ModuleDoesNotReadUnnamedModule -> + // TODO: report this error as soon as module path is used instead of class path for modular compilation + // JAVA_MODULE_DOES_NOT_READ_UNNAMED_MODULE.on(reportOn) + null + is ModuleDoesNotReadModule -> + JAVA_MODULE_DOES_NOT_DEPEND_ON_MODULE.on(reportOn, diagnostic.dependencyModuleName) + is ModuleDoesNotExportPackage -> + JAVA_MODULE_DOES_NOT_EXPORT_PACKAGE.on(reportOn, diagnostic.dependencyModuleName, referencedPackageFqName!!.asString()) + else -> null } - - if (ourModule?.name == theirModule.name) return null - - if (ourModule != null && !moduleResolver.moduleGraph.reads(ourModule.name, theirModule.name)) { - return ErrorsJvm.JAVA_MODULE_DOES_NOT_DEPEND_ON_MODULE.on(reportOn, theirModule.name) - } - - val containingPackage = DescriptorUtils.getParentOfType(targetClassOrPackage, PackageFragmentDescriptor::class.java, false) - val fqName = containingPackage?.fqName ?: return null - if (!theirModule.exports(fqName) && (ourModule == null || !theirModule.exportsTo(fqName, ourModule.name))) { - return ErrorsJvm.JAVA_MODULE_DOES_NOT_EXPORT_PACKAGE.on(reportOn, theirModule.name, fqName.asString()) - } - - return null } private fun findVirtualFile( diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java index fcf105aba40..ce69bc3ac01 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/DefaultErrorMessagesJvm.java @@ -115,6 +115,7 @@ public class DefaultErrorMessagesJvm implements DefaultErrorMessages.Extension { MAP.put(OBSOLETE_SUSPEND_INLINE_FUNCTIONS_ABI, "Cannot inline suspend function built with compiler version less than 1.1.4/1.2-M1"); MAP.put(JAVA_MODULE_DOES_NOT_DEPEND_ON_MODULE, "Symbol is declared in module ''{0}'' which current module does not depend on", STRING); + MAP.put(JAVA_MODULE_DOES_NOT_READ_UNNAMED_MODULE, "Symbol is declared in unnamed module which is not read by current module"); MAP.put(JAVA_MODULE_DOES_NOT_EXPORT_PACKAGE, "Symbol is declared in module ''{0}'' which does not export package ''{1}''", STRING, STRING); } diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java index d55c1ec9999..030bb7beae5 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/diagnostics/ErrorsJvm.java @@ -96,6 +96,7 @@ public interface ErrorsJvm { DiagnosticFactory0 OBSOLETE_SUSPEND_INLINE_FUNCTIONS_ABI = DiagnosticFactory0.create(ERROR); DiagnosticFactory1 JAVA_MODULE_DOES_NOT_DEPEND_ON_MODULE = DiagnosticFactory1.create(ERROR); + DiagnosticFactory0 JAVA_MODULE_DOES_NOT_READ_UNNAMED_MODULE = DiagnosticFactory0.create(ERROR); DiagnosticFactory2 JAVA_MODULE_DOES_NOT_EXPORT_PACKAGE = DiagnosticFactory2.create(ERROR); @SuppressWarnings("UnusedDeclaration") diff --git a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleResolver.kt b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleResolver.kt index d4d9f9d505b..44dcb8c7605 100644 --- a/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleResolver.kt +++ b/compiler/frontend.java/src/org/jetbrains/kotlin/resolve/jvm/modules/JavaModuleResolver.kt @@ -19,12 +19,16 @@ package org.jetbrains.kotlin.resolve.jvm.modules import com.intellij.openapi.components.ServiceManager import com.intellij.openapi.project.Project import com.intellij.openapi.vfs.VirtualFile +import org.jetbrains.kotlin.name.FqName interface JavaModuleResolver { - val moduleGraph: JavaModuleGraph + fun checkAccessibility(fileFromOurModule: VirtualFile?, referencedFile: VirtualFile, referencedPackage: FqName?): AccessError? - // For any .java, .kt or .class file in the project, returns the corresponding module or null if there's none - fun findJavaModule(file: VirtualFile): JavaModule? + sealed class AccessError { + object ModuleDoesNotReadUnnamedModule : AccessError() + data class ModuleDoesNotReadModule(val dependencyModuleName: String) : AccessError() + data class ModuleDoesNotExportPackage(val dependencyModuleName: String) : AccessError() + } companion object SERVICE { fun getInstance(project: Project): JavaModuleResolver = diff --git a/idea/idea-analysis/src/org/jetbrains/kotlin/idea/modules/IdeJavaModuleResolver.kt b/idea/idea-analysis/src/org/jetbrains/kotlin/idea/modules/IdeJavaModuleResolver.kt index 3a1658ed574..6c6e7173264 100644 --- a/idea/idea-analysis/src/org/jetbrains/kotlin/idea/modules/IdeJavaModuleResolver.kt +++ b/idea/idea-analysis/src/org/jetbrains/kotlin/idea/modules/IdeJavaModuleResolver.kt @@ -19,38 +19,62 @@ package org.jetbrains.kotlin.idea.modules import com.intellij.codeInsight.daemon.impl.analysis.JavaModuleGraphUtil import com.intellij.openapi.project.Project import com.intellij.openapi.vfs.VirtualFile -import com.intellij.psi.PsiCompiledElement import com.intellij.psi.PsiJavaModule import com.intellij.psi.PsiManager -import com.intellij.psi.impl.file.impl.JavaFileManager import com.intellij.psi.impl.light.LightJavaModule -import com.intellij.psi.search.GlobalSearchScope -import org.jetbrains.kotlin.resolve.jvm.modules.* +import org.jetbrains.annotations.NotNull +import org.jetbrains.kotlin.name.FqName +import org.jetbrains.kotlin.resolve.jvm.modules.JavaModuleResolver class IdeJavaModuleResolver(project: Project) : JavaModuleResolver { private val psiManager = PsiManager.getInstance(project) - private val fileManager = JavaFileManager.SERVICE.getInstance(project) - private val allScope = GlobalSearchScope.allScope(project) - override val moduleGraph: JavaModuleGraph = JavaModuleGraph( - object : JavaModuleFinder { - override fun findModule(name: String): JavaModule? { - return fileManager.findModules(name, allScope).singleOrNull()?.toJavaModule() - } - } - ) - - override fun findJavaModule(file: VirtualFile): JavaModule? { - val psiFile = psiManager.findFile(file) ?: return null - return JavaModuleGraphUtil.findDescriptorByElement(psiFile)?.toJavaModule() + private fun findJavaModule(file: VirtualFile): PsiJavaModule? { + return psiManager.findFile(file)?.let(JavaModuleGraphUtil::findDescriptorByElement) } - private fun PsiJavaModule.toJavaModule(): JavaModule { - if (this is LightJavaModule) { - return JavaModule.Automatic(name, rootVirtualFile) + override fun checkAccessibility( + fileFromOurModule: VirtualFile?, referencedFile: VirtualFile, referencedPackage: FqName? + ): JavaModuleResolver.AccessError? { + val ourModule = fileFromOurModule?.let(this::findJavaModule) + val theirModule = this.findJavaModule(referencedFile) + + // If we're both in the unnamed module, it's OK, no error should be reported + if (ourModule == null && theirModule == null) return null + + if (theirModule == null) { + // We should probably prohibit this usage according to JPMS (named module cannot use types from unnamed module), + // but we cannot be sure that a module without module-info.java is going to be actually used as an unnamed module. + // It could also be an automatic module, in which case it would be read by every module. + return null } - val virtualFile = containingFile?.virtualFile ?: error("No VirtualFile found for module $this ($javaClass)") - return JavaModule.Explicit(JavaModuleInfo.create(this), virtualFile.parent, virtualFile, this is PsiCompiledElement) + if (ourModule?.name == theirModule.name) return null + + if (ourModule != null && !JavaModuleGraphUtil.reads(ourModule, theirModule)) { + return JavaModuleResolver.AccessError.ModuleDoesNotReadModule(theirModule.name) + } + + val fqName = referencedPackage?.asString() ?: return null + if (!exports(theirModule, fqName, ourModule)) { + return JavaModuleResolver.AccessError.ModuleDoesNotExportPackage(theirModule.name) + } + + return null + } + + // Returns whether or not [source] exports [packageName] to [target] + private fun exports(source: PsiJavaModule, packageName: String, target: PsiJavaModule?): Boolean { + if (source is LightJavaModule) { + return true + } + + // TODO: simply call JavaModuleGraphUtil.exports as soon as its 'target' parameter is nullable + if (target != null) { + return JavaModuleGraphUtil.exports(source, packageName, target) + } + return source.exports.any { statement -> + statement.moduleNames.isEmpty() && statement.packageName == packageName + } } }