PrivateModuleAnalysis ensures that module subtree have same visibility

enso-org · Oct 2, 2023 · 0fee9cd · 0fee9cd
1 parent abadf42
commit 0fee9cd
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 33 deletions.
diff --git a/...time-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/ImportExport.scala b/...time-parser/src/main/scala/org/enso/compiler/core/ir/expression/errors/ImportExport.scala
@@ -192,6 +192,17 @@ object ImportExport {
       s"Cannot import private module '$moduleName'"
   }
 
+  case class SubmoduleVisibilityMismatch(
+    moduleName: String,
+    submoduleName: String,
+    moduleVisibility: String,
+    submoduleVisibility: String
+  ) extends Reason {
+    override def message: String =
+      s"Cannot export submodule '$submoduleName' of module '$moduleName': " +
+      s"the submodule is $submoduleVisibility, but the module is $moduleVisibility"
+  }
+
   /** Represents an ambiguous import resolution error, where the same symbol is imported more than once refereing
     * to different objects. The objects are represented by their physical path in the project.
     *

diff --git a/engine/runtime/src/main/java/org/enso/compiler/pass/analyse/PrivateModuleAnalysis.java b/engine/runtime/src/main/java/org/enso/compiler/pass/analyse/PrivateModuleAnalysis.java
@@ -64,16 +64,13 @@ public Seq<IRPass> invalidatedPasses() {
 
   @Override
   public Module runModule(Module moduleIr, ModuleContext moduleContext) {
-    if (moduleContext.isSynthetic()) {
-      return moduleIr;
-    }
-
     var bindingsMap = (BindingsMap) moduleIr.passData().get(BindingAnalysis$.MODULE$).get();
     var currentPackage = moduleContext.getPackage();
     List<Import> importErrors = new ArrayList<>();
     List<Export> exportErrors = new ArrayList<>();
+    var isCurrentModulePrivate = moduleIr.isPrivate();
 
-    // Check if imported modules are not private
+    // Ensure that imported modules from a different project are not private.
     bindingsMap.resolvedImports().foreach(resolvedImp -> {
       var importedModule = resolvedImp.target().module().unsafeAsModule("should succeed");
       var importedModuleName = importedModule.getName().toString();
@@ -91,8 +88,8 @@ public Module runModule(Module moduleIr, ModuleContext moduleContext) {
       return null;
     });
 
-    // If this module is private, we cannot export anything from it.
-    if (moduleIr.isPrivate() && containsExport(moduleIr)) {
+    // Ensure that no symbols are exported from a private module.
+    if (isCurrentModulePrivate && containsExport(moduleIr)) {
       exportErrors.add(ImportExport.apply(
           moduleIr.exports().apply(0),
           new ImportExport.ExportSymbolsFromPrivateModule(moduleContext.getName().toString()),
@@ -101,6 +98,7 @@ public Module runModule(Module moduleIr, ModuleContext moduleContext) {
       ));
     }
 
+
     // Check if we try to export some other private module.
     bindingsMap
         .getDirectlyExportedModules()
@@ -109,14 +107,33 @@ public Module runModule(Module moduleIr, ModuleContext moduleContext) {
           if (expModuleRef.isPrivate()) {
             var associatedExportIR = findExportIRByName(moduleIr, expModuleRef.getName());
             assert associatedExportIR.isDefined();
-            exportErrors.add(
-                ImportExport.apply(
-                    associatedExportIR.get(),
-                    new ImportExport.ExportPrivateModule(expModuleRef.getName().toString()),
-                    ImportExport.apply$default$3(),
-                    ImportExport.apply$default$4()
-                )
-            );
+            if (isSubmoduleName(moduleContext.getName(), expModuleRef.getName())) {
+              var haveSameVisibility = isCurrentModulePrivate == expModuleRef.isPrivate();
+              if (!haveSameVisibility) {
+                exportErrors.add(
+                    ImportExport.apply(
+                        associatedExportIR.get(),
+                        new ImportExport.SubmoduleVisibilityMismatch(
+                            moduleContext.getName().toString(),
+                            expModuleRef.getName().toString(),
+                            isCurrentModulePrivate ? "private" : "public",
+                            expModuleRef.isPrivate() ? "private" : "public"
+                        ),
+                        ImportExport.apply$default$3(),
+                        ImportExport.apply$default$4()
+                    )
+                );
+              }
+            } else {
+              exportErrors.add(
+                  ImportExport.apply(
+                      associatedExportIR.get(),
+                      new ImportExport.ExportPrivateModule(expModuleRef.getName().toString()),
+                      ImportExport.apply$default$3(),
+                      ImportExport.apply$default$4()
+                  )
+              );
+            }
           }
           return null;
         });
@@ -137,6 +154,16 @@ public Module runModule(Module moduleIr, ModuleContext moduleContext) {
     );
   }
 
+  private boolean isSubmoduleName(QualifiedName parentModName, QualifiedName subModName) {
+    if (subModName.getParent().isDefined()) {
+      return parentModName.item().equals(
+          subModName.getParent().get().item()
+      );
+    } else {
+      return false;
+    }
+  }
+
   @Override
   public Expression runExpression(Expression ir, InlineContext inlineContext) {
     return ir;

diff --git a/engine/runtime/src/test/resources/Test_Private_Modules_4/src/Main.enso b/engine/runtime/src/test/resources/Test_Private_Modules_4/src/Main.enso
@@ -1,6 +1,7 @@
 import project.Sub
 export project.Sub
 
+# Fails at compile time - cannot mix private and public modules in a module subtree.
+
 main =
-    # OK to use private submodule within the same project
-    Sub.Priv_SubMod.foo
+    42
diff --git a/engine/runtime/src/test/scala/org/enso/compiler/test/PassesTest.scala b/engine/runtime/src/test/scala/org/enso/compiler/test/PassesTest.scala
@@ -55,7 +55,7 @@ class PassesTest extends CompilerTest {
           LambdaShorthandToLambda,
           ImportSymbolAnalysis,
           AmbiguousImportsAnalysis,
-          PrivateModuleAnalysis.MODULE$,
+          PrivateModuleAnalysis.getInstance(),
           ShadowedPatternFields,
           UnreachableMatchBranches,
           NestedPatternMatch,

diff --git a/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala b/engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ImportsTest.scala
@@ -242,26 +242,16 @@ class ImportsTest extends PackageTest {
     )
   }
 
-  "Private modules" should "be able to use private submodules via FQN in the same project" in {
-    evalTestProject(
+  "Private modules" should "not be able to mix private and public submodules" in {
+    val e = the[InterpreterException] thrownBy evalTestProject(
       "Test_Private_Modules_4"
-    ) shouldEqual 23
+    )
+    e.getMessage() should include("Cannot export submodule 'local.Test_Private_Modules_4.Sub.Priv_SubMod' of module 'local.Test_Private_Modules_4.Sub'")
   }
 
-  "Private modules" should "be able to use public submodules via FQN" in {
+  "Private module" should "be able to have only private submodules" in {
     evalTestProject(
       "Test_Private_Modules_5"
     ) shouldEqual 42
   }
-
-  "Private modules" should "not be able to use private submodules via FQN" in {
-    the[InterpreterException] thrownBy evalTestProject(
-      "Test_Private_Modules_6"
-    ) should have message "No_Such_Method.Error"
-    val outLines = consumeOut.filterNot(isDiagnosticLine)
-    outLines should have length 1
-    outLines.head should include(
-      "???"
-    )
-  }
 }