Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type constructors #3464

Closed
wants to merge 6 commits into from
Closed

Conversation

ekmett
Copy link
Contributor

@ekmett ekmett commented May 20, 2022

Pull Request Description

This is a work-in-progress draft PR of what we need to get type constructors to exist for real in the compiler.

Important Notes

This removes ResolvedTypeName, and merges it with ResolvedName. It then expands AtomConstructor with parentType and variants information. variants being the name used internally for member types of the parent type.

Open issues:

Status: This now pulls the information from the right place, but doesn't yet populate all of that information.

We need to populate the parent side of the code I ported from Marcin's branch and the child portion of my code to make the two engines work together.

I've yet to fully fix the test suite, though I started with a couple of errors due to resource exhaustion that don't appear to be caused by me.

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 code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

@ekmett ekmett requested review from 4e6 and JaroslavTulach as code owners May 20, 2022 13:34
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid adding Scala data structures into the engine code that is supposed to be Partially Evaluated. Converting to plain @CompilationFinal(dimension=1) AtomConstructor[] shall be enough.

Please make the parentType and variants technically immutable after parsing or make sure changes to the fields properly invalidate partially evaluated code depending on them.

@JaroslavTulach JaroslavTulach requested a review from kustosz May 23, 2022 07:17
@JaroslavTulach
Copy link
Member

JaroslavTulach commented May 31, 2022

I am trying to reproduce the current (with e47d05a) failures by doing:

enso$ sbt --java-home /graalvm-ce-java11-21.3.0/
sbt:enso> buildEngineDistribution
sbt:enso> buildEngineDistribution
sbt:enso> project runtime
sbt:runtime> testOnly *TextTest*
[info]   - should support converting values to display texts *** FAILED *** (62 milliseconds)
[info]     org.enso.interpreter.test.InterpreterException: Compile error: The name Panic is ambiguous. Possible candidates are:
[info]     Type Panic defined in module Standard.Base.Error.Common;
[info]     Type Panic defined in module Standard.Base.Error.Common;
[info]     at <enso>.argument<0>(Test:15)
[info]     at <enso>.argument<1>(Test:15)
[info]     at <enso>.Test.main(Test:15)
[info]     at org.graalvm.sdk/org.graalvm.polyglot.Value.execute(Value.java:841)
[info]     at org.enso.polyglot.Function.execute(Function.scala:16)
[info]     at org.enso.interpreter.test.InterpreterRunner.$anonfun$eval$1(InterpreterTest.scala:179)
[info]     at org.enso.interpreter.test.InterpreterException$.rethrowPolyglot(InterpreterException.scala:18)
[info]     at org.enso.interpreter.test.InterpreterRunner.eval(InterpreterTest.scala:177)
[info]     at org.enso.interpreter.test.InterpreterRunner.eval$(InterpreterTest.scala:174)
[info]     at org.enso.interpreter.test.semantic.TextTest.eval(TextTest.scala:5)

There are other tests failing with similar errors. Finding the cause could fix them all.

I can debug this by telling my IDE to Socket Listen on port 5005 and then executing:

sbt:runtime> withDebug --debugger testOnly -- *TextTest*

when the breakpoint stops at engine/runtime/src/main/scala/org/enso/compiler/pass/resolve/MethodDefinitions.scala:110 I can see err.candidates list contains the same Module as well as BindingsMap$ModuleReference$Concrete twice.

Eliminating the duplicity by:

enso.master$ git diff
diff --git a/engine/runtime/src/main/scala/org/enso/compiler/data/BindingsMap.scala b/engine/runtime/src/main/scala/org/enso/compiler/data/BindingsMap.scala
index 4e90a9fc7..d82cc6580 100644
--- a/engine/runtime/src/main/scala/org/enso/compiler/data/BindingsMap.scala
+++ b/engine/runtime/src/main/scala/org/enso/compiler/data/BindingsMap.scala
@@ -195,7 +195,7 @@ case class BindingsMap(
   private def handleAmbiguity(
     candidates: List[ResolvedName]
   ): Either[ResolutionError, ResolvedName] = {
-    candidates.distinct match {
+    candidates.distinctBy(_.module) match {
       case List()   => Left(ResolutionNotFound)
       case List(it) => Right(it)
       case items    => Left(ResolutionAmbiguous(items))

seems to fix the TextTest. Now:

sbt:runtime> testOnly *TextTest*

passes OK.

ekmett added 5 commits June 16, 2022 21:04
rebased my previous work on top of Marcin's patch where it could apply cleanly.

This broke my approach for resolving AtomConstructors.

I've eliminated his UnionType machinery in favor of heavier AtomConstructors, as we had discussed previously.

This needs a pass that appropriately creates the AtomConstructors for complex type declarations. These are apparently now circular, hence the need for @CompilationFinal

This does not include static resolution. It does not include a proper mapping to parents that preserve detailed type information and doesn't have name punning support.
* split off just to get the obvious stuff up in an easily reviewed form.
@kustosz kustosz force-pushed the compiler/wip/eak/type-constructors branch from 2af39a0 to 0545f7a Compare June 17, 2022 10:39
@@ -1,3 +1,3 @@
type Any
type Any_Tp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the _Tp suffix stand for? ToP level type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to understand this rename.

Is it temporary? If not, I'm a bit skeptical about the _Tp suffix. Also we'd probably need to change all type signatures taking Any to be Any_Tp instead, right? (And analogously for all the others)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is _Tp as _TemPorary`?

@ekmett ekmett closed this Nov 2, 2022
@farmaazon farmaazon deleted the compiler/wip/eak/type-constructors branch May 9, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants