Import resolution throws exception for non-existing symbols #6457

Akirathan · 2023-04-27T15:18:17Z

Pull Request Description

Adds a new step to the Compiler's imports/exports resolution, implemented by the SymbolsImportResolution class, that resolves all the symbols from from ... import symbol_1, symbol_2, ... statements and adds these additional information to the appropriate BindingMap.

Important Notes

Fills in additional resolved import targets to the BindingMap.
SymbolsImportResolution needs to be implemented as a separate step from ImportResolver, as checking for some symbols requires information provided by ExportsResolution that is scheduled by the Compiler after ImportResolver.
Adds ImportExportTest that synthetically creates modules with various import/export statements.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

- import all symbols from module - import all symbols from a type - recognize that a type does not exist - import just some symbols from module/type

CLAassistant · 2023-05-11T16:10:07Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Pavel Marek seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

hubertp · 2023-05-12T10:09:13Z

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

+      "Resolving import symbols for module {0}",
+      Array[Object](module.getName.toString)
+    )
+    module.unsafeSetIr(


This feels a bit dangerous and against the typical pass design. If there are any errors then they should potentially returned as part of this method and it shouldn't be Unit?

Exactly - this is not a typical pass design on purpose. It has similar design to ImportResolver that also does module.unsafeSetIr. Both ImportResolver and SymbolsImportResolution are kind of compiler-internal passes that operate directly on BindingMap and are allowed to set the IR of a module with unsafeSetIR, if I correctly understand the design. The same is true also for ExportResolution.

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

hubertp · 2023-05-12T10:26:29Z

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

+    val exportedSymbols: ListBuffer[BindingsMap.ImportTarget] = ListBuffer.empty
+    importedModuleBindingMap.exportedSymbols.foreach {


Again flatMap

I don't know how to do this for these two nested foreach loops. I am bumping into type errors. Would you provide me suggestion here pls?

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

hubertp · 2023-05-12T10:52:51Z

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

+    val moduleName = if (shouldImportFromType) {
+      importedName.parts.dropRight(1).map(_.name).mkString(".")
+    } else {
+      importedName.name
+    }
+    val typeName: Option[String] = if (shouldImportFromType) {
+      Some(importedName.parts.last.name)
+    } else {
+      None
+    }
+    logger.log(
+      Level.FINEST,
+      "moduleName = {0}, typeName = {1}",
+      Array[Object](moduleName, typeName)
+    )
+
+    val importedModule = compiler.getModule(moduleName) match {
+      case Some(module) => module
+      case None =>
+        throw new CompilerError(
+          s"Module ${moduleName} should already be imported"
+        )
+    }


could be combined into one condition/match

hubertp · 2023-05-12T10:57:09Z

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

+      "Trying to resolve symbol {0} from type {1} from module {2} defined entities",
+      Array[Object](symbolToImport, typeName, module.getName)
+    )
+    if (typeName.isDefined) {


use pattern match instead of conditionals

Really? I use a conditional here on purpose. The block that follows this conditional is rather complicated and long, and uses a lot of pattern matching. Should I introduce yet another level of pattern match?

hubertp · 2023-05-12T10:59:33Z

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

+    val bindingMap = module.getIr.unsafeGetMetadata(
+      BindingAnalysis, "Should be here"


should be done once and I think this is attempted in other place as well

hubertp · 2023-05-12T11:01:54Z

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

+              case resolvedModule: BindingsMap.ResolvedModule => resolvedModule
+              case resolvedType: BindingsMap.ResolvedType => resolvedType
+              case resolvedMethod: BindingsMap.ResolvedMethod => resolvedMethod
+              case resolvedCtor: BindingsMap.ResolvedConstructor => resolvedCtor


is there anything else? Because it feels like you are just copying the list

hubertp · 2023-05-12T11:02:53Z

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

+                case Some(ctor) => Left(BindingsMap.ResolvedConstructor(resolvedType, ctor))
+                case None =>
+                  Right(
+                    IR.Error.ImportExport.NoSuchConstructor(
+                      resolvedType.tp.name,
+                      symbolToImport.name
+                    )
+                  )
+              }


by convention Left is an error value, Right is not

It no longer find also instance methods.

…es in IR

JaroslavTulach · 2023-05-13T05:04:28Z

This is an interesting error:

The name Integer is ambiguous. Possible candidates are:
      Type Integer defined in module Standard.Base.Data.Numbers
      Constructor Report_Error defined in module Standard.Base.Errors.Problem_Behavior;
      The method uncurry defined in module Standard.Base.Function
      The method curry defined in module Standard.Base.Function

hopefully it's going to be easy to fix...

…method

…findExportedSymbolsFor` method

JaroslavTulach · 2023-05-17T09:56:12Z

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

+        })
+
+      bindingMap.resolvedImports =
+        bindingMap.resolvedImports.appendedAll(resolvedImports)


Another place that is modifying bindingMap!?

This scares me. Last week I saw how wild operations with BindingMap were. Various parts of BindingMap are filled by various passes. This change makes them even wilder (previously the resolvedImports were only modified at ImportResolver at least) and I am not sure why? Originally we were supposed to fix #5936 - however for that we don't need any changes to bindingMap.resolvedImports.

While #5936 was supposed to be addressed by a bugfix, this PR is implementing some new functionality. I don't think we should integrate new functionality at this time of the release.

Akirathan · 2023-05-17T10:49:21Z

This PR grew out of hand and contains changes that are not preferable during the Enso release phase. With @JaroslavTulach we have, therefore, concluded that it is best to leave this PR at this moment and to dissect only a subset of the functionality into (preferably a Compiler pass) another PR that only reports an error in case of non-existing symbols, and does not provide additional information in the binding map.

Most of the functionality in this PR stays relevant for symbol resolution.

Akirathan linked an issue Apr 27, 2023 that may be closed by this pull request

Importing a nonexistent module/type using from syntax does not yield any errors #5936

Closed

Akirathan added 4 commits May 2, 2023 16:51

[WIP] Add some tests

d1762c0

Improve docs

ff5856b

bench_download_tool: since and until are dates, not datetimes

b77252e

[WIP] Add SymbolsImportResolution compiler step

6c06775

Akirathan force-pushed the wip/akirathan/import-from-error-5936 branch from e3e17c7 to 6c06775 Compare May 5, 2023 22:13

Akirathan and others added 13 commits May 6, 2023 00:14

Add test for a malformed from import

14b7e1d

Update tests

2348881

Add logging

b31dfb0

SymbolsImportResolution can resolve members of types

a46b07d

Add ImportExportsTest

74325e2

Use custom GraalVM context in ImportExportTest

5833208

Merge branch 'develop' into wip/akirathan/import-from-error-5936

f9bce18

Add some ImportsExportsTest

163e537

Fix SYmbolsImportResolution so that it can:

4b94353

- import all symbols from module - import all symbols from a type - recognize that a type does not exist - import just some symbols from module/type

ImportResolver can can resolve a type or a static method from a module

3f1b2bf

Add some error messages to IR's ImportExport error reasons

bc0eebf

ResolvedConstructor and ResolvedMethod can also be ImportTarget

e3c22d6

Add more import/export tests

6f4df8c

Akirathan force-pushed the wip/akirathan/import-from-error-5936 branch from b7c8cbb to 6f4df8c Compare May 11, 2023 16:09

hubertp reviewed May 12, 2023

View reviewed changes

Akirathan added 8 commits May 12, 2023 14:57

Add final version of ImportExportTest

a283dc8

Do not create modules outside test context

3ef09bd

Remove duplicated test

48db11d

Append all the transitive imports to the resolvedImports

9ba4f25

Fix compilation

2c5ea96

Fix findAllStaticMethodsAndTypes.

903e73d

It no longer find also instance methods.

Describe ignored tests

cbedc07

Fix rest of ImportExportTest

aacbdff

Akirathan added 9 commits May 12, 2023 17:53

Add "assign module-level foreign static method" to BindingAnalysisTest

a8b93b9

Turn off logging in ImportExportTest

7a38656

Use Either for SymbolResolution result type

22c9727

Remove deprecated methods of Either

961678a

Use flatMap instead of the mutable buffer

a4af67b

Add some default implementation for overriden methods in Resolved nam…

85c8e5d

…es in IR

Use flatMap instead of mutable buffer

66e662d

Refactoring - move methods in SymbolsImportResolution

d0bac0e

scalafmtAll

bd6ae72

Akirathan added 5 commits May 16, 2023 12:42

Merge branch 'develop' into wip/akirathan/import-from-error-5936

16fdebf

Fix find methods for ResolvedConstructor and ResolvedMethod

a65e487

Remove multiple imports of the same symbol

36c0b74

Add some tests for exporting and checks for findExportedSymbolsFor …

ff0e1c7

…method

ResolvedConstructor and ResolvedMethod do not export any symbols in `…

ea6185d

…findExportedSymbolsFor` method

JaroslavTulach reviewed May 17, 2023

View reviewed changes

Akirathan added 2 commits May 17, 2023 12:51

Add more tests

6a8ad23

Cosmetics

3cb1428

Akirathan mentioned this pull request May 17, 2023

Import resolver should be capable of resolving also methods and constructors #6729

Open

Akirathan linked an issue May 17, 2023 that may be closed by this pull request

Import resolver should be capable of resolving also methods and constructors #6729

Open

Akirathan closed this Sep 22, 2023

Akirathan self-assigned this Sep 22, 2023

Akirathan deleted the wip/akirathan/import-from-error-5936 branch December 21, 2023 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import resolution throws exception for non-existing symbols #6457

Import resolution throws exception for non-existing symbols #6457

Akirathan commented Apr 27, 2023 •

edited

Loading

CLAassistant commented May 11, 2023

hubertp May 12, 2023

Akirathan May 12, 2023

hubertp May 12, 2023

Akirathan May 12, 2023

hubertp May 12, 2023

hubertp May 12, 2023

Akirathan May 12, 2023

hubertp May 12, 2023

hubertp May 12, 2023

hubertp May 12, 2023

JaroslavTulach commented May 13, 2023

JaroslavTulach May 17, 2023 •

edited

Loading

Akirathan commented May 17, 2023

		val exportedSymbols: ListBuffer[BindingsMap.ImportTarget] = ListBuffer.empty
		importedModuleBindingMap.exportedSymbols.foreach {

		val bindingMap = module.getIr.unsafeGetMetadata(
		BindingAnalysis, "Should be here"

Import resolution throws exception for non-existing symbols #6457

Import resolution throws exception for non-existing symbols #6457

Conversation

Akirathan commented Apr 27, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

CLAassistant commented May 11, 2023

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

JaroslavTulach commented May 13, 2023

JaroslavTulach May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Akirathan commented May 17, 2023

Akirathan commented Apr 27, 2023 •

edited

Loading

JaroslavTulach May 17, 2023 •

edited

Loading