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

Implement conversions (#180312665) #3227

Merged
merged 15 commits into from
Feb 6, 2022
Merged

Conversation

ekmett
Copy link
Contributor

@ekmett ekmett commented Jan 14, 2022

Pull Request Description

Data analysts should be provided with a unified way to convert between data structures (from method)

Important Notes

This was slightly harder than expected.

Implemented a notion of an UnresolvedConversion.

This allows the user to write

type Foo foo
type Bar bar
type Baz baz

Foo.from (that:Bar) = Foo that.bar

Foo.from (that:Baz) = Foo that.baz

main = (Foo.from (Bar 10)).foo + (Foo.from (Baz 20)).foo

In addition to handling atoms and atom constructors properly we also handle Text (both foreign and domestic), functions, arrays and all the other built-in types of the language.

The format of a conversion is required to be

<Type>.from (that:<Type>) <other arguments> = ...

the name of the first parameter must be that or _, and all other arguments must be defaulted.

Overloading is used to resolve the dispatch of the from method.

Overloads resolve from the most specific type of that to the least.

An incidental issue had to be fixed along the way:

All type level errors were being swallowed by the compiler. Notably, operator methods could not be typed, but the error for this was being swallowed silently by the compiler.

UnresolvedConversions are exposed to users like any other UnresolvedSymbols in the Meta API. The user should see no difference (other than the dynamic overloading behavior).

Extension conversions can be defined in imported modules just like extension methods.

Documentation should be supplied as we update the standard library to use this feature.

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 documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

start wip branch for conversion methods for collaborating with marcin

add conversions to MethodDispatchLibrary (wip)

start MethodDispatchLibrary implementations

conversions for atoms and functions

Implement a bunch of missing conversion lookups

final bug fixes for merged methoddispatchlibrary implementations

UnresolvedConversion.resolveFor

progress on invokeConversion

start extracting constructors (still not working)

fix a bug

add some initial conversion tests

fix a bug in qualified name resolution, test conversions accross modules

implement error reporting, discover a ton of ignored errors...

start fixing errors that we exposed in the standard library

fix remaining standard lib type errors not caused by the inability to parse type signatures for operators

TODO: fix type signatures for operators. all of them are broken

fix type signature parsing for operators

test cases for meta & polyglot

play nice with polyglot

start pretending unresolved conversions are unresolved symbols

treat UnresolvedConversons as UnresolvedSymbols in enso user land
@CLAassistant
Copy link

CLAassistant commented Jan 14, 2022

CLA assistant check
All committers have signed the CLA.

RELEASES.md Outdated
Comment on lines 11 to 15
## Interpreter/Runtime

- Added support for overloaded conversions. This allows the `from` method to
be implemented with several overloads. ([#3227](https://github.com/enso-org/enso/pull/3227))

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, we are aiming to have a single changelog and currently the one to use is at app/gui/CHANGELOG.md. cc: @mwu-tow please correct me if I'm wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to put this in the right place. I was just reacting to a red checkmark in the build process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, app/gui/CHANGELOG.md is the location currently enforced by the CI. It'll be moved to the repository root,

@4e6
Copy link
Contributor

4e6 commented Jan 17, 2022

@ekmett please update the docs. I don't see that the new syntax of conversion functions is described anywhere (presumably it should go to the docs/syntax/functions.md doc). Also, it is worth describing the implementation the docs/runtime/function-call-flow.md

@4e6
Copy link
Contributor

4e6 commented Jan 17, 2022

Also, do you plan to support the complex types?

i.e. the case

type Maybe
    type Some value
    type Nothing

Maybe.from (that : Any) = Some that

@ekmett
Copy link
Contributor Author

ekmett commented Jan 17, 2022

The direction you gave in your Maybe example should work (insofar as it parses) today. The other direction converting from a Maybe, has to be written case by case off the actual constructors for Some and Nothing, but conversions into something that is morally a sum type should just work.

@wdanilo and @kustosz want to make "proper" sum types, but that starts to expose all sorts of infelicities in the type system (or lack thereof as the case may be). With those in hand, we'd be able to define the other direction where you have

Foo.from (that : Maybe) = ...

Doing that today will fail because the maybe Atom is never constructed. You should be able to use

Foo.from (that : Some) = ...
Foo.from (that : Nothing) = ... 

as a workaround for the lack of first class sum types today.

Having conversions adds a lot of pressure to make those things real.

@kustosz
Copy link
Contributor

kustosz commented Jan 17, 2022

@ekmett @4e6

Maybe.from won't work currently, simply because Maybe is not an object one could reference (it disappears very early in the compilation process).

Moreover, thinking about this example has made me realize that we probably have a bug in the current code for methods desugaring. Thing is, in Maybe.from, Maybe is in a contravariant position, while our desugaring of methods assumes this to be covariant. Which is to say this:

type Maybe
    type Some a
    type None

    from (x: My_Type) = None

Will desugar to:

type Some a
type None

None.from (x: My_Type) = None
Some.from (x: My_Type) = None # THIS IS HOPELESSLY BROKEN!

A glance at the code tells me this is actually the case.
We need to fix this and explicitly disallow from definitions inside sum type definitions, until we make Maybe into a thing that could be referenced, and can make the desugaring yield the correct code:

Maybe.from (x:My_Type) = None

@ekmett
Copy link
Contributor Author

ekmett commented Jan 18, 2022

Ugh. It is even worse than I feared.

@4e6
Copy link
Contributor

4e6 commented Jan 27, 2022

There are some failing tests in the last CI run: https://github.com/enso-org/enso/runs/4819644988?check_suite_focus=true

You can use runtime/test SBT command to run all the runtime tests locally, or runtime/testOnly *.TailCallTest to run individual test suite, or runtime/testOnly *.TailCallTest -- -z "in tail position" to run individual tests

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

@ekmett
Copy link
Contributor Author

ekmett commented Feb 2, 2022

The errors for RuntimeErrorsTest.scala seem to be unrelated. Something is switching some block of ExpressionUpdates in for what used to be an ExecutionUpdate? But we never touched anything around panics themselves.

…sts that aren't ExpressionUpdates vs. ExecutionUpdate behavioral changes
@4e6
Copy link
Contributor

4e6 commented Feb 3, 2022

Builtins module produces syntax errors, which messes up the expected messages in runtime tests. I guess, since the Builtins module is synthetic, we can ignore errors and warnings that it is producing. I'll push a comment to this branch that filters out unwanted diagnostic messages.

@kustosz
Copy link
Contributor

kustosz commented Feb 3, 2022

Builtins module produces syntax errors, which messes up the expected messages in runtime tests. I guess, since the Builtins module is synthetic, we can ignore errors and warnings that it is producing. I'll push a comment to this branch that filters out unwanted diagnostic messages.

No no no, it is quite important for Builtins to still compile. I'm going to fix it instead :)

@4e6
Copy link
Contributor

4e6 commented Feb 3, 2022

@kustosz feel free to revert my commit then

@ekmett
Copy link
Contributor Author

ekmett commented Feb 4, 2022

Do we have any remaining blockers?

@ekmett ekmett mentioned this pull request Feb 4, 2022
4 tasks
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.

8 participants