-
Notifications
You must be signed in to change notification settings - Fork 323
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
Sum type info in suggestions DB #3422
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skipped review of runtime/*
for two reasons:
- I don't understand that code base much.
- it is not really important how this is implemented
I am focusing on
- tests - they look great.
@Suggestion
API - the proposed state differs from the current spec
It would make it easier for me to keep the implementation, keep the tests and drop the suggestions part and redo it just like #3407 shows. The other option is to update the design spec.
engine/runtime/src/test/scala/org/enso/compiler/test/context/SuggestionBuilderTest.scala
Outdated
Show resolved
Hide resolved
engine/runtime/src/test/scala/org/enso/compiler/test/context/SuggestionBuilderTest.scala
Outdated
Show resolved
Hide resolved
engine/polyglot-api/src/main/scala/org/enso/polyglot/Suggestion.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to squash commits a bit. Give the commits better name and integrate. Great to see this working!
.Argument("this", "Unnamed.Test.MyType", false, false, None) | ||
.Argument( | ||
"this", | ||
"Unnamed.Test.MyType", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this change? Some automatic refactoring? Otherwise the new construct looks semantically the same as the old one to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, style hint from IntelliJ (one that I agree with – use named args for booleans)
engine/runtime/src/test/scala/org/enso/compiler/test/context/SuggestionBuilderTest.scala
Show resolved
Hide resolved
engine/runtime/src/test/scala/org/enso/compiler/test/context/SuggestionBuilderTest.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update docs. Other than that LGTM
CHANGELOG.md
Outdated
@@ -198,6 +199,8 @@ | |||
[3412]: https://github.com/enso-org/enso/pull/3412 | |||
[3414]: https://github.com/enso-org/enso/pull/3414 | |||
[3417]: https://github.com/enso-org/enso/pull/3417 | |||
[3417]: https://github.com/enso-org/enso/pull/3417 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate link
@@ -30,6 +30,10 @@ sealed trait Tree[+A] { | |||
final def map[B](f: A => B): Tree[B] = | |||
Tree.map(this)(f) | |||
|
|||
@JsonIgnore | |||
final def collectFirst[B](f: PartialFunction[A, B]): Option[B] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some documentation
@@ -639,6 +652,9 @@ object SuggestionBuilder { | |||
sealed private trait TypeArg | |||
private object TypeArg { | |||
|
|||
case class Sum(name: QualifiedName, variants: Seq[QualifiedName]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation
throw new CompilerError( | ||
"Bindings cannot be obtained from an abstract module reference." | ||
) | ||
def resolveTypeName( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation
@@ -721,6 +731,8 @@ object BindingsMap { | |||
*/ | |||
case class Cons(name: String, arity: Int, allFieldsDefaulted: Boolean) | |||
|
|||
case class Type(name: String, members: Seq[String]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add doc
def toConcrete(moduleMap: ModuleMap): Option[ResolvedTypeName] | ||
} | ||
|
||
case class ResolvedType(override val module: ModuleReference, tp: Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documentation
/** A result of successful name resolution. | ||
*/ | ||
sealed trait ResolvedName { | ||
def module: ModuleReference | ||
sealed trait ResolvedName extends ResolvedTypeName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proably doc should be updated as well
Pull Request Description
A draft of simple changes to the compiler to expose sum type information. Doesn't break the stdlib & at the same time allows for dropdowns. This is still broken, for example it doesn't handle exporting/importing types, only ones defined in the same module as the signature. Still, seems like a step in the right direction – please provide feedback.
Important Notes
I've decided to make the variant info part of the type, not the argument – it is a property of the type logically.
Also, I've pushed it as far as I'm comfortable – i.e. to the
SuggestionHandler
– I have no idea if this is enough to show in IDE? cc @4e6Checklist
Please include the following checklist in your PR:
./run dist
and./run watch
.