-
Notifications
You must be signed in to change notification settings - Fork 326
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
Get rid of free-floating atoms. Everything has a type now! #3671
Conversation
engine/runtime/src/main/java/org/enso/interpreter/runtime/library/types/TypesLibrary.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/pass/analyse/BindingAnalysis.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 don't think I know the interpreter codebase to analyse the details very thoroughly without spending about a week on learning it deeper, so I'm approving based on other's approval and the fact that it works. It may be wise for @hubertp to review this PR post-mortem, as I think he knows these parts much better.
As for what I saw - it seems to look good.
I'm sooooo glad of the unified method dispatch in one place instead of being scattered in various type definitions. This is AWESOME!
Apart from that I have a few minor comments or questions. What I think would be good to sort out before merging is removing leftover commented-out code (I marked a few, I'm not entirely sure if there's more) - unless it should be kept - but then some comment explaining why would that be needed.
I've found at least one FIXME - I think it should be associated with some Pivotal task - then the link can be added to the comment.
engine/runtime/src/main/java/org/enso/interpreter/runtime/builtin/BuiltinType.java
Outdated
Show resolved
Hide resolved
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
Co-authored-by: James Dunkerley <[email protected]>
type Custom_Type_With_Error | ||
Custom_Type_With_Error_Data | ||
|
||
to_text : Text | ||
to_text self = Error.throw (Illegal_State_Error_Data "foo_error") |
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'm not sure if I understand what happened here? You removed the ctor and pass the Type in the tests, so that you want to rely on the static variant of to_text
for formatting? I guess that can work, but shouldn't then to_text
not have the self
parameter anymore?
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.
probably being overzelaous, can be reverted
|
||
OperatorToFunction.runModule(moduleInput, modCtx) shouldEqual moduleOutput | ||
} | ||
// "be translated in module contexts" in { |
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.
@kustosz this can go, right?
@@ -73,7 +73,7 @@ class DocumentationCommentsTest extends CompilerTest with Inside { | |||
*/ | |||
def getDoc(ir: IR): String = { | |||
val meta = ir.getMetadata(DocumentationComments) | |||
meta shouldBe defined | |||
// meta.shouldBe(defined) |
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.
@kustosz intentional?
Pull Request Description
This is a step towards the new language spec. The
type
keyword now means something. So we now haveas a thing one may write. Also
Some
andNone
are not standalone types now – onlyMaybe
is.This halfway to static methods – we still allow for things like
Number + Number
for backwards compatibility. It will disappear in the next PR.The concept of a type is now used for method dispatch – with great impact on interpreter code density.
Some APIs in the STDLIB may require re-thinking. I take this is going to be up to the libraries team – some choices are not as good with a semantically different language. I've strived to update stdlib with minimal changes – to make sure it still works as it did.
It is worth mentioning the conflicting constructor name convention I've used: if
Foo
only has one constructor, previously namedFoo
, we now have:This is now necessary, because we still don't have proper statics. When they arrive, this can be changed (quite easily, with SED) to use them, and figure out the actual convention then.
I have also reworked large parts of the builtins system, because it did not work at all with the new concepts.
It also exposes the type variants in SuggestionBuilder, that was the original tiny PR this was based on.
PS I'm so sorry for the size of this. No idea how this could have been smaller. It's a breaking language change after all.
Important Notes
Checklist
Please include the following checklist in your PR:
Scala,
Java,
and
Rust
style guides.
./run ide build
and./run ide watch
.