non-orphan implicit typeclasses #912

lewisjkl · 2023-04-19T23:00:33Z

Making it so typeclass instances can be added in companion objects in the generated code. This uses a mechanism similar to refinements to specify the location of the typeclass and the interpreter that provides it.

Will add more details as this PR comes out of draft status.

lewisjkl · 2023-04-19T23:01:08Z

modules/example/src/smithy4s/example/MovieTheater.scala

+    MovieTheater.apply
+  }.withId(id).addHints(hints)
+
+  implicit val movieTheaterEq: cats.Eq[MovieTheater] = EqInterpreter.fromSchema(schema)


This is the main thing to look at here. I still have a ways to go on this PR, but I wanted to put it up now to get early feedback before I get further along with it.

Looks great to me.

kubukoz · 2023-04-20T00:06:13Z

ohh la la. I may like this.

kubukoz · 2023-04-20T00:10:06Z

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

+
+    val precompiler = new Alt.Precompiler[Schema, AltEq] {
+      def apply[A](label: String, instance: Schema[A]): AltEq[A] = {
+        // Here we "cheat" to recover the `Alt` corresponding to `A`, as this information


Shall we add support for this in Precompiler?

kubukoz · 2023-04-20T00:11:55Z

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

+      }
+    }
+
+    val precompiler = new Alt.Precompiler[Schema, AltEq] {


nitpick: usually I just inline this

kubukoz · 2023-04-20T00:13:38Z

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

+  def primitiveEq[P](primitive: Primitive[P]): Eq[P] = {
+    primitive match {


This smells an awful lot like Primitive.deriving.

kubukoz · 2023-04-20T00:15:20Z

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

+    val eqA: Eq[A] = self(schema)
+    (x: B, y: B) => eqA.eqv(refinement.from(x), refinement.from(y))


Suggested change

val eqA: Eq[A] = self(schema)

(x: B, y: B) => eqA.eqv(refinement.from(x), refinement.from(y))

self(schema).contramap(refinement.from)

same in bijections

kubukoz · 2023-04-20T00:17:08Z

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

+  ): Eq[S] = { (x: S, y: S) =>
+    {
+      def forField[A2](field: Field[Schema, S, A2]): Boolean = {
+        val eqField = field.foldK(new Field.FolderK[Schema, S, Eq]() {


issue: since this is an example we probably want to follow best practices, so I'd suggest some more "precompiled" way: make forField return an S => Boolean and move the (x: S, y: S) => below so that we traverse the fields while instantiating the whole thing.

kubukoz · 2023-04-20T00:18:46Z

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

+    implicit val valueEq: Eq[V] = self(value)
+    Eq[Map[K, V]]


kinda interesting how they made Maps comparable without an Eq for the key

kubukoz · 2023-04-20T00:20:26Z

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

+    tag match {
+      case CollectionTag.ListTag       => Eq[List[A]]
+      case CollectionTag.SetTag        => Eq[Set[A]]
+      case CollectionTag.VectorTag     => Eq[Vector[A]]
+      case CollectionTag.IndexedSeqTag => Eq[IndexedSeq[A]]
+    }


thought: we could have a CollectionTag.deriving in a similar spirit to Primitive.deriving

kubukoz · 2023-04-20T00:22:07Z

modules/example/src/smithy4s/example/MovieTheater.scala

+    MovieTheater.apply
+  }.withId(id).addHints(hints)
+
+  implicit val movieTheaterEq: cats.Eq[MovieTheater] = EqInterpreter.fromSchema(schema)


Looks great to me.

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

kubukoz · 2023-04-20T00:26:38Z

sorry for the comment spam @lewisjkl, this is looking great

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala

modules/protocol/resources/META-INF/smithy/smithy4s.meta.smithy

lewisjkl · 2023-04-20T22:59:43Z

sorry for the comment spam @lewisjkl, this is looking great

No worries @kubukoz I actually copied that EqInterpreter from something that @yisraelU worked on a while back. I really just put it as a placeholder until he PRs in a smithy4s-cats module that adds that among other interpreters. Then we can remove it from this PR and just reference it instead.

yisraelU · 2023-04-21T03:30:28Z

modules/codegen/src/smithy4s/codegen/internals/IR.scala

@@ -280,6 +280,8 @@ private[internals] object Hint {
    case object IndexedSeq extends SpecializedList
  }
  case object UniqueItems extends Hint
+  case class Typeclass(id: ShapeId, targetType: String, interpreter: String)


A nice-to-have would be a type alias wherever a String refers to a Class (for doc purposes)
Perhaps this should extend to the Smithy definitions too

yisraelU · 2023-04-21T03:34:43Z

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

+      hints: List[Hint],
+      tpe: NameRef
+  ): Option[Lines] = {
+    (hints.collectFirst { case h: Hint.Typeclasses =>


I find collectFirst to be a bit misleading. There can be only one ,of type Hint.Typeclass and if there could be more , I think we would want them all

It's not misleading, I think it's just wrong. It should be collect here.

oh of course .I was thinking this was Hints as opposed to a List of hint

I think it's right. There should only ever be one Typeclasses hint which may contain multiple 'Typeclass' instances. It doesn't need to be modeled that way, but that's how I did it so we'd keep the idea that you can only have 1 of a given trait/hint per shape. Collect first is just so I could do the find and map at the same time

but that's how I did it so we'd keep the idea that you can only have 1 of a given trait/hint per shape

I've "violated" that assumption in my commit 😅. For non-meta hints, I think you are correct, but there is a distinction to be had between hints that get rendered and available at runtime and hints that aren't. Trying to abide by the invariant at codegen time seems to be detrimental to the readability of the code .

Also, sorry ,should have amended my comment. It wasn't "wrong", I was just confused between the existence of a Typeclass and a Typeclasses type, which got mixed together in my brain.

Yeah that totally makes sense, I think it's clearer without the extra typeclasses hint so works for me 👍🏼

Baccata · 2023-04-21T09:33:43Z

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

+  private def renderTypeclass(hint: Hint.Typeclass, tpe: NameRef): Line = {
+    val target = NameRef(hint.targetType)
+    val interpreter = NameRef(hint.interpreter)
+    val lowerCasedName = s"${tpe.name.head.toLower.toString}${tpe.name.tail}"


I think we have a helper for this : uncapitalise

Baccata · 2023-04-21T09:35:45Z

modules/codegen/src/smithy4s/codegen/internals/Renderer.scala

+  private def renderTypeclasses(
+      hints: List[Hint],
+      tpe: NameRef
+  ): Option[Lines] = {


You can return Lines

removing EqSchemaVisitor in favour of a universalEquals-based implementation.

…it-typeclasses

Baccata · 2023-04-21T12:33:16Z

Build fails because docs are currently linking a non-existing class. We'll wait for the cats-module to be merged to go back to this PR.

…neystreaming/smithy4s into inlined-implicit-typeclasses

lewisjkl · 2023-05-05T21:40:43Z

Will add changelog for this and #942 on Monday

inlined implicit typeclasses

c53a6c9

lewisjkl commented Apr 19, 2023

View reviewed changes

kubukoz reviewed Apr 20, 2023

View reviewed changes

Baccata reviewed Apr 20, 2023

View reviewed changes

modules/example/src/smithy4s/example/typeclass/EqInterpreter.scala Outdated Show resolved Hide resolved

daddykotex reviewed Apr 20, 2023

View reviewed changes

modules/protocol/resources/META-INF/smithy/smithy4s.meta.smithy Outdated Show resolved Hide resolved

lewisjkl added 2 commits April 20, 2023 16:55

handle more types, add docs

cb20981

fix docs, use toNel

08a26d4

yisraelU reviewed Apr 21, 2023

View reviewed changes

Baccata reviewed Apr 21, 2023

View reviewed changes

Baccata added 3 commits April 21, 2023 11:48

Addressing comments,

3f4716c

removing EqSchemaVisitor in favour of a universalEquals-based implementation.

Polish spacing

f7549c7

Merge remote-tracking branch 'origin/series/0.18' into inlined-implic…

4c9d6e9

…it-typeclasses

lewisjkl added 5 commits May 5, 2023 13:42

Merge branch 'series/0.18' into inlined-implicit-typeclasses

0d91ff2

Merge branch 'inlined-implicit-typeclasses' of https://github.com/dis…

c11322c

…neystreaming/smithy4s into inlined-implicit-typeclasses

update with using cats module

03f5675

update docs

e2d4f44

update example3

39e260b

lewisjkl mentioned this pull request May 8, 2023

start 0.18 changelog #951

Merged

lewisjkl marked this pull request as ready for review May 8, 2023 18:56

Baccata approved these changes May 11, 2023

View reviewed changes

Baccata merged commit 79534b7 into series/0.18 May 11, 2023

Baccata deleted the inlined-implicit-typeclasses branch May 11, 2023 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

non-orphan implicit typeclasses #912

non-orphan implicit typeclasses #912

lewisjkl commented Apr 19, 2023

lewisjkl Apr 19, 2023

kubukoz Apr 20, 2023

kubukoz commented Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz Apr 20, 2023

kubukoz commented Apr 20, 2023

lewisjkl commented Apr 20, 2023

yisraelU Apr 21, 2023 •

edited

Loading

yisraelU Apr 21, 2023

Baccata Apr 21, 2023

yisraelU Apr 21, 2023

lewisjkl Apr 21, 2023

Baccata Apr 21, 2023

Baccata Apr 21, 2023

lewisjkl Apr 21, 2023

Baccata Apr 21, 2023 •

edited

Loading

Baccata Apr 21, 2023

Baccata commented Apr 21, 2023

lewisjkl commented May 5, 2023

		def primitiveEq[P](primitive: Primitive[P]): Eq[P] = {
		primitive match {

		val eqA: Eq[A] = self(schema)
		(x: B, y: B) => eqA.eqv(refinement.from(x), refinement.from(y))

	val eqA: Eq[A] = self(schema)
	(x: B, y: B) => eqA.eqv(refinement.from(x), refinement.from(y))
	self(schema).contramap(refinement.from)

non-orphan implicit typeclasses #912

non-orphan implicit typeclasses #912

Conversation

lewisjkl commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubukoz commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubukoz commented Apr 20, 2023

lewisjkl commented Apr 20, 2023

yisraelU Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata commented Apr 21, 2023

lewisjkl commented May 5, 2023

yisraelU Apr 21, 2023 •

edited

Loading

Baccata Apr 21, 2023 •

edited

Loading