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

[data] add Render instance for Record (#1008) #1019

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

road21
Copy link
Contributor

@road21 road21 commented Jan 15, 2025

I had to extend FieldsLike evidence to derive Render instances, and now it also contains tuples (types) of names and values.
Also I have noticed that an actual type of ~ is never used, so I removed it and now ~ is an abstract type member.

Copy link
Collaborator

@hearnadam hearnadam left a comment

Choose a reason for hiding this comment

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

This is awesome!

kyo-data/shared/src/main/scala/kyo/Record.scala Outdated Show resolved Hide resolved
kyo-data/shared/src/main/scala/kyo/Record.scala Outdated Show resolved Hide resolved
val insts = RecordInternal.summonRender[ev.Names, ev.Values]
Render.instance: (value: Record[Fields]) =>
value.toMap.map((field, value) =>
insts(field.name) match
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will insts always contain all values as the map? Is apply safe here?

Copy link
Contributor Author

@road21 road21 Jan 19, 2025

Choose a reason for hiding this comment

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

It's not safe and there was a bug here because of record subtyping (we may want to print rich record as record with only one field)

I have fixed that and added test

kyo-data/shared/src/main/scala/kyo/Render.scala Outdated Show resolved Hide resolved
@fwbrasil
Copy link
Collaborator

I was about to post a PR generalizing FieldsLike to a TypeSet utility :) We have an issue with the kernel's Boundary that I think we could solved with it: https://github.com/getkyo/kyo/pull/new/typeset. @road21 let me know what you think, I'll pause working on Record for now since you're working on it

builder.toString()
end asText
end new
Render.instance: (value: A) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need Render.instance? I think the compiler can materialize Render directly from an anonymous function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I try to to make Render in inline methods via anonymous function I get compiler warn:

An inline given alias with a function value as right-hand side can significantly increase
generated code size. You should either drop the `inline` or rewrite the given with an
explicit `apply` method.

May be we can just ignore this warn, not sure.

@road21
Copy link
Contributor Author

road21 commented Jan 15, 2025

I was about to post a PR generalizing FieldsLike to a TypeSet utility :) We have an issue with the kernel's Boundary that I think we could solved with it: https://github.com/getkyo/kyo/pull/new/typeset. @road21 let me know what you think, I'll pause working on Record for now since you're working on it

The main reason I've added Names and Values type members to FieldsLike is that erasedValue and match types work bad with abstract types (or opaque types). As I see, you solved this problem with final infix class ~[Name <: String, Value] private ().

I think your approach is much better and TypeSet really can help in many other cases (I haven't seen yet issue with Boundary, but I will check).

So, let's finish your PR first, and after that I will fix this PR.

@fwbrasil
Copy link
Collaborator

Cool! I've just posted #1021

[data] make `~` an abstract type
@road21 road21 force-pushed the records-render-instance-derivation branch from b9170e1 to 46d3079 Compare January 19, 2025 22:24
@@ -432,10 +432,12 @@ class RecordTest extends Test:
""")
}

"not compile when fields lack CanEqual" in {
"not compile when fields lack CanEqual" in pendingUntilFixed { // looks like scala3 bug
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why this test works in main

When I was fighting with it, I minimized my case to

import scala.compiletime.*
import scala.language.strictEquality

class Lol
object Lol:
  inline given CanEqual[Lol, Lol] = error("lol")

val x = new Lol
val y = new Lol

// summon[CanEqual[Lol, Lol]] // doesn't compile
x == y // compile, and this is strange

https://scastie.scala-lang.org/road21/B8hWnSQNTriBcOYndUduRg

For me this looks like a bug in scala3 compiler, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, that's odd. I can follow up on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fwbrasil fwbrasil merged commit 2f07f80 into getkyo:main Jan 20, 2025
3 checks passed
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.

3 participants