Import syntax including namespace #1806

kustosz · 2021-06-18T15:03:12Z

Pull Request Description

Modifies the import syntax to include the author/namespace prefix in project names. Also introduces a special project keyword for imports of modules from the current project.

Important Notes

This introduces a namespace field to package.yaml. This is a temporary solution, as this is supposed to be mediated by the libraries repository in the future.
It's a very breaking change.
Also introduces a breaking change to the refactoring/renameProject method of the language server, requiring the namespace to be provided for project identification.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the Scala, Java, and Rust style guides.
All documentation and configuration conforms to the markdown and YAML style guides.
All code has been tested where possible.

iamrecursion

Don't include the image at distribution/std-lib/Examples/data/image.png. It was ignored in .gitignore but its path has changed. Please adapt .gitignore.

Additionally, please test and confirm that we still get the suggestions in the IDE as expected.

Finally, you're missing a changelog entry. Please put one in RELEASES.md.

iamrecursion · 2021-06-18T15:26:40Z

engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/DocumentationComments.scala

@@ -169,7 +169,7 @@ case object DocumentationComments extends IRPass {
      case expr: IR.Expression            => resolveExpression(expr)
      case df: IR.Module.Scope.Definition => resolveDefinition(df)
      case imp: IR.Module.Scope.Import    => imp
-      case exp: IR.Module.Scope.Export    => exp
+      case exp: IR.Module.Scope.Export.Module    => exp


Formatting.

iamrecursion · 2021-06-18T15:27:23Z

engine/runtime/src/main/scala/org/enso/compiler/phase/ImportResolver.scala

+                val exp = ir.exports
+                  .collect { case ex: IR.Module.Scope.Export.Module => ex }
+                  .find(_.name.name == impName)


Given you're touching the imports resolver, how much work would it be to fix #1569?

More than I can spend :P Probably ~2 days.

iamrecursion · 2021-06-18T15:28:00Z

engine/runtime/src/test/resources/Cycle_Test/src/Main.enso

@@ -1,3 +1,4 @@
-import Cycle_Test.Sub.Imp
+import project.Sub.Imp


Can we still import by the project name itself?

I would think so, because the desugar process converts to exactly that, so there should be no reason why that would be disallowed.

Project name + name space. So the original import here is invalid, but you could write import local.Cycle_Test.Sub.Imp

iamrecursion · 2021-06-18T15:31:34Z

lib/scala/syntax/definition/src/main/scala/org/enso/syntax/text/ast/meta/Builtin.scala

+              .collect {
+                case AST.Ident.Cons.any(n) => n
+                case AST.Ident.Var.any(n)  => n
+              }


Do you mean to implicitly ignore anything that somehow slips through to here? .collect will discard things that don't match.

I think given how the qualNamePat is defined, nothing else could show up here, right?
But then if it is ever changed, indeed an error could be introduced that will go unnoticed here, so I think I'd +1 that it would be good to somehow handle the unexpected (currently, but not necessarily forever, impossible) case.

It also contains the Opr('.') segments, this is a way of filtering them.

iamrecursion · 2021-06-18T15:33:46Z

tools/ci/Test.enso

@@ -1,3 +1,3 @@
-from Builtins import all
+from Standard.Builtins import all

 main = IO.println "Test successful."


Missing line ending.

radeusgd

Looks good overall

Below are some comments

radeusgd · 2021-06-19T13:14:24Z

distribution/std-lib/Base/src/Meta/Enso_Project.enso

+
+## Returns the namespace of the project.
+
+   > Example
+     Get the name of the current project.
+
+         Enso_Project.namespace
+Builtins.Project_Description.namespace : Text
+Builtins.Project_Description.namespace = this.prim_config.namespace


This doc is unclear to me about what is the behaviour if I do the following:
Library Foo.Bar defines a method baz = Builtins.Project_Description.namespace and then in a project radeusgd.Hmm I do import Foo.Bar and then IO.println Foo.Bar.baz. Will it return Hmm or Foo?

Project_Descritption is just an atom, so you call namespace on its instance, obtained through calling the enso_project method of some module. The wording is unclear indeed, I'll fix this (and the method above, as this one is a copy-paste :P )

radeusgd · 2021-06-19T13:24:18Z

engine/language-server/src/main/scala/org/enso/languageserver/refactoring/RefactoringApi.scala

@@ -10,7 +10,7 @@ object RefactoringApi {

  case object RenameProject extends Method("refactoring/renameProject") {

-    case class Params(oldName: String, newName: String)
+    case class Params(namespace: String, oldName: String, newName: String)


I think it may be worth noting somewhere explicitly that the rename can (currently) only change the project name, but not the namespace.

radeusgd · 2021-06-19T13:25:06Z

engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala

    /** Signals that project has been renamed.
      *
      * @param newName the new project name
      */
-    case class ProjectRenamed(newName: String) extends ApiResponse
+    case class ProjectRenamed(namespace: String, newName: String)
+        extends ApiResponse


Missing doc for namespace

radeusgd · 2021-06-19T13:28:41Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/Module.java

   * @param name the qualified name of this module.
   * @param sourceFile the module's source file.
   */
-  public Module(QualifiedName name, TruffleFile sourceFile) {
+  public Module(QualifiedName name, Package<TruffleFile> pkg, TruffleFile sourceFile) {


Missing doc for the added parameter

radeusgd · 2021-06-19T13:35:57Z

engine/runtime/src/main/scala/org/enso/compiler/core/IR.scala

+          def copy(
+            name: IR.Name.Qualified                    = name,
+            rename: Option[IR.Name.Literal]            = rename,
+            isAll: Boolean                             = isAll,


I know that this is not part of this PR, but this name isAll seems a bit confusing to me, especially as below there are special cases where isAll does not really mean all. Maybe it could be renamed to something more appropriate, like isQualified?

yes, it maybe could, when someone works on this

radeusgd · 2021-06-19T14:23:19Z

lib/scala/pkg/src/main/scala/org/enso/pkg/Config.scala

  * @param dependencies a list of package dependencies
  * @param originalJson a Json object holding the original values that this
  *                     Config was created from, used to preserve configuration
  *                     keys that are not known
  */
 case class Config(
  name: String,
+  namespace: String,


Namespace is not documented.

I think it's important to note here that this is just a temporary workaround that will be removed once it is possible.
Also that it falls back to local if not provided.

radeusgd · 2021-06-19T14:25:30Z

engine/runtime/src/test/scala/org/enso/compiler/test/codegen/AstToIrTest.scala

+        "from Foo.Bar.Quux import Baz",
+        "from Foo.Bar.Test import Baz, Spam",


Why these two are not prefixed by username? Or is Foo meant to be the username here? It's a bit confusing as in other examples Foo is used as project/module name. If this is meant to test capitalised usernames, I'd suggest making this Username instead of Foo.

radeusgd · 2021-06-19T14:29:40Z

engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/ExpressionIdTest.scala

-      val id1  = meta.addItem(33, 36)
-      val id2  = meta.addItem(58, 8)
+      val id1  = meta.addItem(42, 36)
+      val id2  = meta.addItem(67, 8)


It's worrying to me that so many of our tests rely on these locations.
Because, if the tests are being updated it's so extremely tempting to just run them, see what are the new locations and paste them here without checking if they correspond to the right entities.
It's certainly not within scope of this PR to fix that, but just wanted to mention that maybe at some point we could consider taking some other approach, which could maybe be based on identifying the locations by some infix or something more readable than just an integer location.

cc: @iamrecursion

Because, if the tests are being updated it's so extremely tempting to just run them, see what are the new locations and paste them here without checking if they correspond to the right entities.

Would be very tempting, if that was possible... But the only feedback these give you is "the node you want doesn't exist". Nonetheless, the idea of having special markers for metadata is awesome (and I'm pretty sure @mwu-tow already has something like that in the IDE?)

radeusgd · 2021-06-19T14:31:07Z

lib/scala/pkg/src/main/scala/org/enso/pkg/QualifiedName.scala

-    this.copy(path = newName :: path.tail)
+    val namespace = path.head
+    this.copy(path = namespace :: newName :: path.drop(2))


I understand that we can assume that the project sugar is already not present here, correct?

radeusgd · 2021-06-19T14:33:44Z

lib/scala/syntax/definition/src/main/scala/org/enso/syntax/text/ast/meta/Builtin.scala

+              .collect {
+                case AST.Ident.Cons.any(n) => n
+                case AST.Ident.Var.any(n)  => n
+              }


I think given how the qualNamePat is defined, nothing else could show up here, right?
But then if it is ever changed, indeed an error could be introduced that will go unnoticed here, so I think I'd +1 that it would be good to somehow handle the unexpected (currently, but not necessarily forever, impossible) case.

builtins -> standard.builtins progress towards tests fixing more tests more tests more tests (almost) all tests untangle std untangle std all e2e tests error handling licenses CR feedback remove image.png

iamrecursion · 2021-06-21T11:51:46Z

RELEASES.md

+- Implemented changes to the import and export syntax, requiring to provide the
+  project namespace, or use the new `project` keyword to import from the current
+  project.


Please put this under Interpreter/Runtime and add the PR number.

iamrecursion · 2021-06-21T11:52:59Z

engine/runtime/src/test/scala/org/enso/compiler/test/codegen/AstToIrTest.scala

+        "from Username.Bar export all",
+        "from Username.Bar as Eggs export all hiding Spam",
+        "from Username.Bar export all hiding Spam, eggs"


Probably should test with project and lowercase username.

iamrecursion · 2021-06-21T11:53:13Z

lib/rust/lexer/generation/tests/enso_lexer_combined.rs

@@ -324,7 +324,9 @@ polyglot java import org.enso.base.Text_Utils
                Token::line(
                    vec![
                        Token::variable("from", 0),
-                        Token::referent("Builtins", 1),
+                        Token::referent("Standard", 1),
+                        Token::operator(".",0),


Suggested change

Token::operator(".",0),

Token::operator(".", 0),

farmaazon · 2021-06-23T08:25:27Z

@kustosz I think the change in the import syntax should be reflected in docs: https://enso.org/docs/developer/enso/syntax/imports.html#imports-and-exports or https://enso.org/docs/developer/enso/semantics/modules.html#referring-to-modules

What namespace will the stdlib have?

Imports and Exports · Enso Developer Documentation
The developer documentation for the Enso compiler, runtime, and IDE..

Modules · Enso Developer Documentation
The developer documentation for the Enso compiler, runtime, and IDE..

kustosz added Type: Enhancement --breaking Important: a change that will break a public API or user-facing behaviour p-highest Should be completed ASAP labels Jun 18, 2021

kustosz self-assigned this Jun 18, 2021

kustosz requested review from 4e6, iamrecursion, radeusgd and wdanilo as code owners June 18, 2021 15:03

iamrecursion added the Difficulty: Core Contributor label Jun 18, 2021

iamrecursion reviewed Jun 18, 2021

View reviewed changes

radeusgd approved these changes Jun 19, 2021

View reviewed changes

half baked

6422ef2

builtins -> standard.builtins progress towards tests fixing more tests more tests more tests (almost) all tests untangle std untangle std all e2e tests error handling licenses CR feedback remove image.png

kustosz force-pushed the wip/mk/namespace-import branch from c27a234 to 6422ef2 Compare June 21, 2021 11:09

kustosz added 2 commits June 21, 2021 13:14

Merge branch 'main' into wip/mk/namespace-import

82a4607

fix rust test

cdfc180

iamrecursion approved these changes Jun 21, 2021

View reviewed changes

kustosz and others added 7 commits June 21, 2021 13:54

fixes

41b36ba

Fix rust tests

ade6189

fix poly tests

be46bd3

Merge branch 'main' into wip/mk/namespace-import

2f37677

fix pkg tests

1d19a6c

Merge branch 'main' into wip/mk/namespace-import

4ec51fb

Merge branch 'main' into wip/mk/namespace-import

6d47686

kustosz and others added 4 commits June 23, 2021 14:24

fix project manager

8792811

save

58d178f

Merge branch 'main' into wip/mk/namespace-import

052f7d3

changleog

f9e3394

kustosz merged commit 334a022 into main Jun 24, 2021

kustosz deleted the wip/mk/namespace-import branch June 24, 2021 10:42

iamrecursion pushed a commit that referenced this pull request Jul 9, 2021

Import syntax including namespace (#1806)

6322a94

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import syntax including namespace #1806

Import syntax including namespace #1806

kustosz commented Jun 18, 2021

iamrecursion left a comment

iamrecursion Jun 18, 2021

iamrecursion Jun 18, 2021

kustosz Jun 21, 2021

iamrecursion Jun 18, 2021

radeusgd Jun 19, 2021

kustosz Jun 21, 2021

iamrecursion Jun 18, 2021

radeusgd Jun 19, 2021

kustosz Jun 21, 2021

iamrecursion Jun 18, 2021

radeusgd left a comment

radeusgd Jun 19, 2021

kustosz Jun 21, 2021

radeusgd Jun 19, 2021

radeusgd Jun 19, 2021

radeusgd Jun 19, 2021

radeusgd Jun 19, 2021

kustosz Jun 21, 2021

radeusgd Jun 19, 2021

radeusgd Jun 19, 2021

radeusgd Jun 19, 2021

kustosz Jun 21, 2021

radeusgd Jun 19, 2021

kustosz Jun 21, 2021

radeusgd Jun 19, 2021

iamrecursion Jun 21, 2021

iamrecursion Jun 21, 2021

iamrecursion Jun 21, 2021

farmaazon commented Jun 23, 2021 •

edited by unfurl-links bot

Loading

		@@ -1,3 +1,4 @@
		import Cycle_Test.Sub.Imp
		import project.Sub.Imp

		"from Foo.Bar.Quux import Baz",
		"from Foo.Bar.Test import Baz, Spam",

Import syntax including namespace #1806

Import syntax including namespace #1806

Conversation

kustosz commented Jun 18, 2021

Pull Request Description

Important Notes

Checklist

iamrecursion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon commented Jun 23, 2021 • edited by unfurl-links bot Loading

farmaazon commented Jun 23, 2021 •

edited by unfurl-links bot

Loading