SuggestionBuilder needs to send ascribedType of constructor parameters #6655

4e6 · 2023-05-12T09:08:41Z

Pull Request Description

close #6611

Changelog:

update: run compiler passes on the ascribedType field of the constructor arguments
update: suggestion builder uses the type information attached to ascribedType
feat: resolve qualified names in type signatures

Important Notes

Checklist

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

The documentation has been updated, if necessary.
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.

JaroslavTulach

Much more Scala-like changes! I assume you will need the same changes as I did overall the code base. Plus you need support for type arguments in constructor - I solved it by adding resolveType additional parameter with type parameter names.

JaroslavTulach · 2023-05-12T09:29:22Z

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

@@ -4723,6 +4722,7 @@ object IR {
      def mapExpressions(fn: Expression => Expression): Specified = {
        copy(
          name         = name.mapExpressions(fn),
+          ascribedType = ascribedType.map(fn),


I guess this change of IR.DefinitionArgument.Specified.mapExpressions is answering my question:

I was hoping the d.mapExpressions(resolveExpression ...) will iterate over IR.DefinitionArgument, but it doesn't.

Overall the changes in this PR look more Scala-like than my #6584

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

hubertp

I'm missing some tests in TypeSignaturesTest since this change touches that pass. But overall, looks sane.

hubertp · 2023-05-12T14:28:03Z

engine/runtime/src/main/scala/org/enso/compiler/context/SuggestionBuilder.scala

+  private def buildArgument(arg: IR.DefinitionArgument): Suggestion.Argument = {
+    val signatureOpt = arg.name.getMetadata(TypeSignatures).map { meta =>
+      buildTypeSignature(meta.signature) match {
+        case Vector(targ) => buildTypeArgumentName(targ)


This seems to assume that you don't deal with Function, which can return vector of size > 1?

hubertp · 2023-05-12T14:32:05Z

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

+  private def resolveDefinitionData(
+    data: IR.Module.Scope.Definition.Data
+  ): IR.Module.Scope.Definition.Data = {
+    data.copy(


We should eventually start using a smarter copy that only copies if data.arguments.map(resolvedArgument) != data.arguments. But that optimization can be done in multiple places so can be done later.

Am I right that transformExpressions does such kind of optimization a bit? My understanding is that transformExpression callback may specify partial match on just a few IR.Expression.SubTypes and only those get duplicated - the rest of the IR tree remains unchanged.

Shall we prefer the use of transformExpressions over copy? Are there any places to perform such a replacement in current code?

start using a smarter copy

Is there such a "smarter copy" already or do we have to write one? Can we create such "no-copy-if-unmodified" copy in a generic way or do we have to manually override each copy implementation or wrap each copy invocation?

radeusgd

Tests look good. I guess a negative test with unknown type could be nice, but if it's already handled by @JaroslavTulach 's PR I guess it's also fine to skip it.

4e6 · 2023-05-12T20:55:05Z

PR enables the resolution of qualified names in type signatures. At this moment stdlib compilation fails with two errors:

In module Standard.Table.Internal.Excel_Reader:
Compiler encountered errors:
Excel_Reader.enso[33:42-33:68]: The name `ExcelHeaders.HeaderBehavior` could not be found.
In module Standard.Table.Data.Type.Value_Type:
Compiler encountered errors:
Value_Type.enso[135:77-135:84]: The name `SQL_Type` could not be found.
Aborting due to 2 errors and 0 warnings.

JaroslavTulach · 2023-05-13T05:47:54Z

Excel_Reader.enso[33:42-33:68]: The name ExcelHeaders.HeaderBehavior could not be found.

An attempt to import Java inner class! Mitigated in 74c6f99 and reported #6678

Value_Type.enso[135:77-135:84]: The name SQL_Type could not be found.

Same problem as in my PR - addressed by e56bb3a

Let's see how far the CI can get...

JaroslavTulach · 2023-05-13T07:17:03Z

3d73eb7 addresses this failure.

4e6 · 2023-05-13T08:08:59Z

distribution/lib/Standard/Table/0.0.0-dev/src/Data/Type/Value_Type.enso

@@ -1,5 +1,6 @@
 from Standard.Base import all
 import Standard.Base.Errors.Illegal_Argument.Illegal_Argument
+import Standard.Database.Data.SQL_Type.SQL_Type


This makes Standard.Table library dependent on Standard.Database. I'm not sure if we want that because the Database lib already depends on the Table

In fact, this caused an issue with IR caching https://github.com/enso-org/enso/actions/runs/4966001323/jobs/8887184345#step:10:3121
What happened is that IR for Standard.Database.Connection.Database module was not cached, and it resulted in compilation error:

INFO ide_ci::program::command: bashℹ️ In module Standard.Database.Connection.Database: INFO ide_ci::program::command: bashℹ️ Compiler encountered errors: INFO ide_ci::program::command: bashℹ️ Database.enso[23:11-23:28]: The name `Connection_Details` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[23:33-23:50]: The name `Connection_Options` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[23:55-23:64]: The name `Connection` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[23:68-23:76]: The name `SQL_Error` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[24:25-24:42]: The name `Connection_Options` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[28:29-28:41]: The name `Single_Choice` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[30:28-30:33]: The name `Vector` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[31:63-31:67]: The name `False` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[35:9-35:14]: The name `Option` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[36:5-36:17]: The name `Single_Choice` could not be found. INFO ide_ci::program::command: bashℹ️ Database.enso[36:27-36:33]: The name `Display` could not be found. INFO ide_ci::program::command: bash⚠️ Aborting due to 11 errors and 0 warnings. INFO ide_ci::program::command: bashℹ️ Execution finished with an error: Compilation aborted due to errors.

This makes Standard.Table library dependent on Standard.Database

Cyclic dependencies are bad. It'd be good to prevent them with some failure - #6679

In fact, this caused the issue with IR caching

Oh, my dear! I was hunting the problem for almost a week and it is caused by such a little oversight. Thank you very much for working on this PR in parallel with #6584 - it was very refreshing and useful to see different approach to the problem. Without your help I'd still be blindly searching for a bug in BindingsMap.

4e6 · 2023-05-13T09:47:50Z

I can reproduce the issue with IR caches that fails the CI:

clean build-distribution and .local/share/enso/cache directories
run $ sbt buildEngineDistribution
run $ ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --run test/Benchmarks

there's no issues when run with --no-ir-caches flag

4e6 · 2023-05-13T18:31:46Z

Merging it to unblock @JaroslavTulach

…z-6260 * develop: Build nightlies every day. (#6681) Force `newDashboard` default on the CI-built packages. (#6680) Verify ascribed types of parameters really exist (#6584) SuggestionBuilder needs to send ascribedType of constructor parameters (#6655) Improvements to the Table visualization. (#6653) Removing flush (#6670) Improving widgets for take/drop (#6641)

hubertp · 2023-05-15T09:46:27Z

I can reproduce the issue with IR caches that fails the CI:

clean build-distribution and .local/share/enso/cache directories

run $ sbt buildEngineDistribution

run $ ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --run test/Benchmarks

there's no issues when run with --no-ir-caches flag

I'm guessing this was the circular dependency thing because I have no issues right. And yes, I'm able to reproduce the problems when I introduce that import.

* develop: Build nightlies every day. (#6681) Force `newDashboard` default on the CI-built packages. (#6680) Verify ascribed types of parameters really exist (#6584) SuggestionBuilder needs to send ascribedType of constructor parameters (#6655) Improvements to the Table visualization. (#6653) Removing flush (#6670) Improving widgets for take/drop (#6641) disable authentication and newDashboard by default (#6664) Add COOP+COEP+CORP headers (#6646)

feat: resolve ascribed types

d3e8582

4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label May 12, 2023

4e6 self-assigned this May 12, 2023

JaroslavTulach reviewed May 12, 2023

View reviewed changes

JaroslavTulach mentioned this pull request May 12, 2023

Verify ascribed types of parameters really exist #6584

Merged

2 tasks

4e6 added 2 commits May 12, 2023 16:57

fix: alias analysis

fccff47

fix: getter return type

ef4fe4a

4e6 marked this pull request as ready for review May 12, 2023 14:20

4e6 requested review from jdunkerley, radeusgd, GregoryTravis, hubertp and Akirathan as code owners May 12, 2023 14:20

GregoryTravis approved these changes May 12, 2023

View reviewed changes

hubertp approved these changes May 12, 2023

View reviewed changes

radeusgd approved these changes May 12, 2023

View reviewed changes

fix: compilation errors

a5ba9dc

enso-bot bot mentioned this pull request May 13, 2023

Broken redefinition of variable inside if-then expression #6211

Open

JaroslavTulach added 2 commits May 13, 2023 07:54

Importing Standard.Database.Data.SQL_Type.SQL_Type

e56bb3a

Mitigate problems of accessing Java inner classes by using Any

74c6f99

JaroslavTulach force-pushed the wip/db/6611-suggestionbuilder-needs-to-send-ascribedtype-of-constructor-parameters branch from 11bf895 to 74c6f99 Compare May 13, 2023 05:55

With from Standard.Base import all one only uses Vector

3d73eb7

4e6 commented May 13, 2023

View reviewed changes

4e6 added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label May 13, 2023

fix: stdlib tests

f011a83

fix: circular dependency

4e115b6

test: disable flaky test

61f9399

4e6 added the CI: Ready to merge This PR is eligible for automatic merge label May 13, 2023

mergify bot merged commit 7067917 into develop May 13, 2023

mergify bot deleted the wip/db/6611-suggestionbuilder-needs-to-send-ascribedtype-of-constructor-parameters branch May 13, 2023 18:33

enso-bot bot mentioned this pull request May 14, 2023

SuggestionBuilder needs to send ascribedType of constructor parameters #6611

Closed

JaroslavTulach mentioned this pull request May 14, 2023

Avoid cyclic dependencies in standard libraries by reporting an error #6679

Closed

JaroslavTulach added a commit that referenced this pull request May 14, 2023

Merging with Dmitry's #6655

50e1da7

hubertp mentioned this pull request May 15, 2023

Make compilation of stdlib components more resilient to cache corruption #6135

Closed

enso-bot bot mentioned this pull request May 16, 2023

Investigate flood of suggestionsDatabaseUpdate messages on startup #6706

Closed

enso-bot bot mentioned this pull request Jun 1, 2023

Project folder renaming stopped working #6515

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SuggestionBuilder needs to send ascribedType of constructor parameters #6655

SuggestionBuilder needs to send ascribedType of constructor parameters #6655

4e6 commented May 12, 2023 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach May 12, 2023

hubertp left a comment

hubertp May 12, 2023

hubertp May 12, 2023

JaroslavTulach May 13, 2023

radeusgd left a comment

4e6 commented May 12, 2023

JaroslavTulach commented May 13, 2023 •

edited

Loading

JaroslavTulach commented May 13, 2023

4e6 May 13, 2023

4e6 May 13, 2023

JaroslavTulach May 14, 2023

4e6 commented May 13, 2023 •

edited

Loading

4e6 commented May 13, 2023

hubertp commented May 15, 2023 •

edited

Loading

SuggestionBuilder needs to send ascribedType of constructor parameters #6655

SuggestionBuilder needs to send ascribedType of constructor parameters #6655

Conversation

4e6 commented May 12, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach May 12, 2023

Choose a reason for hiding this comment

hubertp left a comment

Choose a reason for hiding this comment

hubertp May 12, 2023

Choose a reason for hiding this comment

hubertp May 12, 2023

Choose a reason for hiding this comment

JaroslavTulach May 13, 2023

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

4e6 commented May 12, 2023

JaroslavTulach commented May 13, 2023 • edited Loading

JaroslavTulach commented May 13, 2023

4e6 May 13, 2023

Choose a reason for hiding this comment

4e6 May 13, 2023

Choose a reason for hiding this comment

JaroslavTulach May 14, 2023

Choose a reason for hiding this comment

4e6 commented May 13, 2023 • edited Loading

4e6 commented May 13, 2023

hubertp commented May 15, 2023 • edited Loading

4e6 commented May 12, 2023 •

edited

Loading

JaroslavTulach commented May 13, 2023 •

edited

Loading

4e6 commented May 13, 2023 •

edited

Loading

hubertp commented May 15, 2023 •

edited

Loading