Replace all `from ... export all` with explicit exports #10369

Akirathan · 2024-06-25T14:56:50Z

Follow-up of #10274. Partially fixes #10258, #6729, #9236

Pull Request Description

Replaces all from ... export all with explicit by-name exports.

Important Notes

Remove isAll and hiding fields from Export IR
Static or extension method can be imported by name
Simplify exports resolution algorithm
A new ConflictingResolutionsError compiler error is thrown when there are conflicting resolutions in a module.
For exmple, a conflicting resolution is a type defined in the module that has the same name as a different submodule.
Previously, this was reported as a warning, but it would now cause issues for the export resolution, so an error is thrown earlier.
Add a lot of tests for BindingsMap.
Add tests for various steps of ExportsResolution.
- We used to have 0 unit tests for this, as well as for BindingsMap.

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,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

And ImportResolverAlgorithm can import it

…inistic. We don't care about it.

module method can be exported directly. And static method can be exported by name.

docs/syntax/imports.md

engine/runtime-compiler/src/main/java/org/enso/compiler/phase/ImportResolverAlgorithm.java

GregoryTravis · 2024-07-10T16:32:46Z

distribution/lib/Standard/Base/0.0.0-dev/src/Main.enso

-from project.Meta.Enso_Project export enso_project
-from project.Network.Extensions export all
-from project.System.File_Format export Auto_Detect, Bytes, File_Format, Infer, JSON_Format, Plain_Text_Format
+export project.Data.Boolean.Boolean


Is from ... export deprecated even when it's not using all?

They are not deprecated, they are just now suffering from #10399. Until that issue is fixed, rule of a thumb is to just use plain export ....

jdunkerley

Approving the libs bits.

JaroslavTulach · 2024-07-11T09:39:48Z

Failing tests:

forgotten closed file prevents delete of directory on windows
SuggestionBuilderTest fails on all platforms
RuntimeSuggestionUpdateTest fails on all platforms
I am not sure what No_Such_Method error means?

Close, so close...

JaroslavTulach

It's great to see all tests are green! I am looking forward to see the speedup after integration.

Akirathan · 2024-07-11T17:23:14Z

There is not enough time to run proper benchmarks before integration. But I have at least run some benchmarks locally (lower score is better - ms/op):

Benchmark name	Old score	New score
ImportStandardLibrariesBenchmark	219	58
ManyErrorsBenchmark	77	74
ManyLocalVarsBenchmark	32	25
ManyNestedBlocksBenchmark	51	46
ManySmallMethodsBenchmark	114	106

Moreover, local experiments with running

enso --no-compile-dependencies --no-global-cache --no-ir-caches --no-read-ir-caches --compile Base

revealed that compilation of Standard.Base now takes 6 seconds, versus the old 31 seconds.

Akirathan · 2024-07-11T17:34:02Z

Done a brief QA with the IDE and everything seems fine. Integrating ...

radeusgd · 2024-07-12T04:46:23Z

docs/syntax/imports.md

+export in `A_Module.enso`:
+
+```
+export project.A_Module.export


This probably should have been:

- export project.A_Module.export + export project.A_Module.method

radeusgd · 2024-07-12T04:55:42Z

engine/runtime-compiler/src/main/java/org/enso/compiler/phase/ImportResolverAlgorithm.java

+    if (parts.size() < 3) {
+      return null;
+    }
+    var constructors = definedConstructors(imp);


Would it make sense to return a Map here? Then the O(N) stream filter could potentially become an O(log N) / O(1) map access.

Also looking at the code it seems like we are constructing this on each call. Have you considered caching the result? I'm not sure how many times this is called, but I'd assume it can be called a lot on heavily used modules, so avoiding to re-construct this List (or Map) every time could give us some additional speedup.

radeusgd · 2024-07-12T05:15:42Z

engine/runtime-compiler/src/main/java/org/enso/compiler/phase/exports/ExportSymbolAnalysis.java

+ * Ensures that all the symbols that are exported exist. If not, an IR error is generated. It is not
+ * a {@link IRPass compiler pass} because it needs access to {@link
+ * org.enso.compiler.PackageRepository}. Ignores renamed exports.


I don't understand the explanation why this isn't a compiler pass.

I mean I'm not convinced it has to be.

But we can access PackageRepository in passes, I've been doing this for #9812.

var packageRepo = moduleContext.pkgRepo().isDefined() ? moduleContext.pkgRepo().get() : null;

or is something wrong about this pattern?

Vast majority of CPU time in ExportsResolution is spent in [BindingsMap.SymbolRestriction.optimize](https://github.com/enso-org/enso/blob/9a373572470ed434fb7b99114553c411ad54718e/engine/runtime-compiler/src/main/scala/org/enso/compiler/phase/ExportsResolution.scala#L173). #10369 dealt with this. This PR only adds the `ExportImportResolutionBenchmark`. # Important Notes - Introduce new [ExportImportResolutionBenchmark](https://github.com/enso-org/enso/blob/9e70f675d8c6d60b74733606b1c48f52e55da7ba/engine/runtime-benchmarks/src/main/java/org/enso/compiler/benchmarks/exportimport/ExportImportResolutionBenchmark.java) that measures the performance of import and export resolution only. - Note that the already existing [ImportStandardLibrariesBenchmark](https://github.com/enso-org/enso/blob/4d49b003752e8771e95d5ae379ce953c85ef020c/engine/runtime-benchmarks/src/main/java/org/enso/compiler/benchmarks/module/ImportStandardLibrariesBenchmark.java) is probably fine for the purpose, but I just wanted to be sure that **ONLY** the import/export resolution is measured and nothing else, so I have isolated that into a new benchmark.

Replace all from ... export all with explicit exports

0137a25

Akirathan self-assigned this Jun 25, 2024

Akirathan added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Jun 25, 2024

Akirathan added 26 commits June 25, 2024 17:03

Add some missing exports

d7362f7

Remove isAll and hiding fields from Export IR

2bf2f4f

Test compiler errors for missing export fields

3140aaa

Add ExportConstructorTest

4cb2bb1

Add ExportedSymbolsTest

2844901

Add ExportCycleDetectionTest

15829b5

Remove ExportCycleDetectionTest

2252a6f

Reintroduce ExportCycleDetectionTest

bad6558

Add test for exported symbols in synthetic modules

6351bc3

Add test for export cycle detection in three modules

4b34b94

Remove unnecessary imports from tests

0ac44a3

Test export from different module

6dc51f3

Implement ProjectUtils.deleteRecursively

87dc19d

ProjectUtils: Main module does not have to exist when creating a project

5148403

Remove BindingMap.SymbolRestriction

e428408

Force compilation in ExportedSymbolsTest

6937908

Add test for importing a constructor

c9f0a36

Add test for exporting a constructor

3a00437

ResolvedConsytructor is an ImportTarget

1c8ec12

fmt

7699d17

ResolvedMethod is ImportTarget.

3bf93ea

And ImportResolverAlgorithm can import it

Fix ExportCycleDetectionTest - reported order of module is non-determ…

21218cb

…inistic. We don't care about it.

Fix ExportStaticMethodTest.

c9b881d

module method can be exported directly. And static method can be exported by name.

Enable test for module method resolution

02e2eb1

fmt

3de9c67

Importing symbols from methods is not allowed

f457a3e

Akirathan added 2 commits July 10, 2024 17:22

Fix error messages in ErrorCompilerTest

fd4f2ab

Fix error messages in ErrorCompilerTest

9eabbcd

JaroslavTulach reviewed Jul 10, 2024

View reviewed changes

Skip ExportSymbol analysis for non-compiled modules

8a7dcc4

Akirathan marked this pull request as ready for review July 10, 2024 16:30

Akirathan requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, marthasharkey, 4e6 and hubertp as code owners July 10, 2024 16:30

Update docs

5b1fd5c

GregoryTravis approved these changes Jul 10, 2024

View reviewed changes

jdunkerley approved these changes Jul 10, 2024

View reviewed changes

enso-bot bot mentioned this pull request Jul 11, 2024

Comparable.from doesn't return Comparable #10355

Closed

micro-distribution exports Boolean type

a4aa2b6

Update RuntimeSuggestionUpdatesTest

4aca14c

JaroslavTulach approved these changes Jul 11, 2024

View reviewed changes

Akirathan merged commit 0f9852a into develop Jul 11, 2024
41 checks passed

Akirathan deleted the wip/akirathan/10258-disallow-export-all branch July 11, 2024 17:34

radeusgd reviewed Jul 12, 2024

View reviewed changes

This was referenced Jul 12, 2024

Analyze and improve the performance of Import/export resolution #9236

Closed

Add ExportImportResolutionBenchmark #10043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace all `from ... export all` with explicit exports #10369

Replace all `from ... export all` with explicit exports #10369

Akirathan commented Jun 25, 2024 •

edited

Loading

GregoryTravis Jul 10, 2024

Akirathan Jul 10, 2024

jdunkerley left a comment

JaroslavTulach commented Jul 11, 2024

JaroslavTulach left a comment

Akirathan commented Jul 11, 2024

Akirathan commented Jul 11, 2024

radeusgd Jul 12, 2024

radeusgd Jul 12, 2024

radeusgd Jul 12, 2024

radeusgd Jul 12, 2024

Replace all from ... export all with explicit exports #10369

Replace all from ... export all with explicit exports #10369

Conversation

Akirathan commented Jun 25, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

GregoryTravis Jul 10, 2024

Choose a reason for hiding this comment

Akirathan Jul 10, 2024

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

JaroslavTulach commented Jul 11, 2024

JaroslavTulach left a comment

Choose a reason for hiding this comment

Akirathan commented Jul 11, 2024

Akirathan commented Jul 11, 2024

radeusgd Jul 12, 2024

Choose a reason for hiding this comment

radeusgd Jul 12, 2024

Choose a reason for hiding this comment

radeusgd Jul 12, 2024

Choose a reason for hiding this comment

radeusgd Jul 12, 2024

Choose a reason for hiding this comment

Replace all `from ... export all` with explicit exports #10369

Replace all `from ... export all` with explicit exports #10369

Akirathan commented Jun 25, 2024 •

edited

Loading