tek / splain

better implicit errors for scala
MIT License
370 stars 29 forks source link

diff of compound types #34

Closed joprice closed 9 months ago

joprice commented 4 years ago

When using zio's env, it is common to have errors where the two types differ by a single member such as Logging with UserRepo with Clock with Random vs Logging with UserRepo with Clock. It would be helpful to have the differences between the two types highlighted as with other type parameters.

tek commented 4 years ago

I'll look into it!

tek commented 4 years ago

built something! release is on the way, please give it a spin and let me know, @joprice !

joprice commented 4 years ago

Nice! I tried it out and below are some of the outputs. I'm not that well versed in this plugin yet, so some of this might be expected:

  1. One is on implicit evidence for two intersection types being equal:

    Error] !I ev1 (Cannot prove that zio.ZEnv with zio.Has[io.prometheus.client.exporter.HTTPServer] with zio.Has[sttp.client.SttpBackend[zio.Task,Nothing,sttp.client.asynchttpclient.WebSocketHandler]] with zio.logging.Logging.Logging with zio.Has[com.iheart.graphql.GraphqlServer.ServerConfig] with com.iheart.graphql.CatalogService.CatalogService with com.iheart.graphql.CollectionService.CollectionService with com.iheart.graphql.LiveMetaService.LiveMetaService with com.iheart.graphql.RecsService.RecsService <:< zio.metrics.prometheus.Registry with zio.config.Config[com.iheart.graphql.GraphqlServer.ServerConfig] with zio.ZEnv with sttp.client.asynchttpclient.zio.SttpClient.): zio.Has[Clock.Service] with zio.Has[Console.Service] with zio.Has[System.Service] with zio.Has[Random.Service] with zio.Has[Blocking.Service] with zio.Has[exporter.HTTPServer] with zio.Has[client.SttpBackend[zio.Task[scala.Any, lang.Throwable, ?], scala.Nothing, asynchttpclient.WebSocketHandler]] with zio.Has[Logging.Service] with zio.Has[GraphqlServer.ServerConfig] with zio.Has[CatalogService.Service] with zio.Has[CollectionService.Service] with zio.Has[LiveMetaService.Service] with zio.Has[RecsService.Service] Predef.<:< zio.Has[Registry.Service] with zio.Has[GraphqlServer.ServerConfig] with zio.Has[Clock.Service] with zio.Has[Console.Service] with zio.Has[System.Service] with zio.Has[Random.Service] with zio.Has[Blocking.Service] with zio.Has[client.SttpBackend[zio.Task[scala.Any, lang.Throwable, ?], scala.Nothing, asynchttpclient.WebSocketHandler]]

    The error effectively gets doubled, once without color, followed by once in green. This is an interesting case because <:< is not built-in, but it's treated as a built-in infix equality type, where the left and the right types are diffed visually by the user.

  2. I see <none> in a case where one type is missing

    zio.ZLayer[zio.Has[GraphqlServer.PrometheusExporterConfig] with zio.Has[Exporters.Service] with zio.Has[Registry.Service]|<none>, lang.Throwable, zio.Has[exporter.HTTPServer]]

    I assume this is expected, but I read it at first as a failure scenario of the plugin. Maybe there could be a table of errors in the readme showing common errors and what they result in.

  3. This scenario works perfectly

    trait R {
        type F[A] = A
        type B
        type C
        type D
        def f(): F[B with C] = null.asInstanceOf[F[C with D]]
      }

    giving

    this.C with this.D|this.B

But, in some cases I see

<noprefix>.C with <noprefix>.D|<noprefix>.B

The here seems might be related to the setting splain:keepmodules, because I also had the compiler error "only traits and abstract classes can have declared but undefined members" as the time, as I had those types above that are wrapped in R defined directly in an object. Maybe the wrapping type is not yet determined in this case so the "noprefix" is unavoidable? Maybe a note in the readme on the conventions of "noprefix", "none", etc would help?

tek commented 4 years ago
  1. This is due to the use of the @implicitNotFound annotation, which prints the whole type in the first part of the message, while the second occurence is splain just showing it (there's no diff involved). Not sure if this can be improved.

  2. Yes, I chose <none> as a static string for the case where one of the types has more parents than the other. Do you have a suggestion for how to display this more clearly?

  3. That looks like it's simply an unhandled case. If you can provide a reproducible case for this I can take care of it!

joprice commented 4 years ago
  1. I haven't taken a look at the code yet, but I wonder if the implicit type is available and could be treated like the infix types elsewhere

  2. I'm not sure about that yet as I've only just started using this. No strong feelings. But a section in the readme explaining each one would probably be enough, just so someone can do a quick scan the first time it pops up. Not sure I'm a big fan of it, but some tools associate an id with each error so it can be found in an index, like rust: Rust error[E0412]: type name...., which can be found herehttps://doc.rust-lang.org/error-index.html

  3. Looks like the key to that one is trying to nest those declarations at the top-level of a function:

    object X {
    def x = {
      type F[A] = A
      type B
      type C
      type D
      def f(): F[B with C] = null.asInstanceOf[F[C with D]]
    }
    }

That results in only traits and abstract classes can have declared but undefined members, which causes <noprefix>. This makes sense, since there is no defined owner, so maybe it's not worth looking into. If the type aliases are defined, there is no longer an issue and the output is quite nice: lang.String with scala.Double|scala.Int. (The only issue I have with that output is a feature request for -P:splain:keepmodules:1 to have an option only include objects in the prefix path. That would avoid lang.String, while distinguishing Exporters.Service from Registry.Service. I can also imagine an option to add one level of prefix whenever there is ambiguity, or an option to always dealias).

tek commented 4 years ago
  1. I don't understand how this relates to infix types!

  2. I would prefer the messages to be intuitive without a manual, especially when the plugin will be merged into the compiler. Would you think that leaving the <none> out and only relying on the color to indicate whether the type is missing or surplus would be enough? Might be a problem for apps that reuse the output without color, or colorblind people… For now, I can mention it in the readme but a solution would be nice.

  3. I fixed it! Just ignoring the <noprefix> string, which comes from Symbol, I think. Regarding the keepmodules comments, I won't be tweaking those plugin options any further since it has been expressed on the compiler PR that they should be reduced, so waiting on consensus on that. But it would probably be a good idea to have a sane heuristic as a default, and disambiguation sounds good!

tek commented 4 years ago

I implemented disambiguation of names for diffs, release is coming!

joprice commented 4 years ago

Great! I didn't realize this was going to be merged into the compiler. That's good news!

For the implicit one, the type of the implicit being searched for is <:< https://www.scala-lang.org/api/current/scala/$less$colon$less.html. Is that not an infix type?

joprice commented 4 years ago

I found one more case, where the plugin loses the underlying type from the original error: (X.<refinement>.type (with underlying type X.F[Int with X.T]). Removing the F wrapper provides the more detailed error.

    trait T
    type F[A] = A
    def x[A](y: A): F[A with T] = y.asInstanceOf[A with T]
    def f[A](a: A with String): F[A] = a
    val y: F[Int with T] = x(x(1))
    // error: `<refinement>|lang.String`
    f(y)
    // error: `X.T|<none> with scala.Int|lang.String`
    val z: Int with T = x(x(1))
    f(z)
tek commented 4 years ago

Great! I didn't realize this was going to be merged into the compiler. That's good news!

Here's hoping :slightly_smiling_face: It's been going on for like four years, but it seems to be approaching now. You can follow the process here: https://github.com/scala/scala/pull/7785

For the implicit one, the type of the implicit being searched for is <:< https://www.scala-lang.org/api/current/scala/$less$colon$less.html. Is that not an infix type?

It is, but it is not a diff. You could interpret it that way by explicitly matching on the specific type and the condition that there are refined types involved, but this will be much easier when it's in the compiler, as an analyzer plugin.

I found one more case, where the plugin loses the underlying type from the original error: (X.<refinement>.type (with underlying type X.F[Int with X.T]). Removing the F wrapper provides the more detailed error.

There's a lot of these edge cases, since the type system's data types is aren't functional. I'll give this one a look!

tek commented 4 years ago

so your example doesn't register as a refined type because the code simply doesn't cover all cases, and a refined type as a type argument isn't one that's being accounted for :slightly_smiling_face: I assume that fixing this would require a more substantial structural change, and I would rather do that once the scala PR is through.

tek commented 4 years ago

most of the cases were added ad-hoc when they occurred to me or a reporter sufficiently frequently, cause I didn't start the project with a complete mapping of the type system :smile:

joprice commented 4 years ago

That makes sense. Just trying to provide as many odd cases as I can! Here's another one:

scala.<byname>[A]|mutable.Buffer[lang.String] => zio.ZIO[scala.Any, scala.Nothing, A]

I would expect the argument of the function to be rendered as (=> A) => ... as in the original error.

tek commented 4 years ago

that one was a bit easier.

joprice commented 4 years ago

Sometimes I see Any with Any with Any... in compound types. I don't have an example at hand, but wanted to ask if there was any "deduping" logic, or if you maintained the original type as is. I'm not sure if there's actually any significance in multiple types showing up that should be retained in the output.

tek commented 4 years ago

nope, looking at the parents of a refined type hasn't been done before your request, so there's not much else going on :slightly_smiling_face:

joprice commented 4 years ago

Are the types in the diff ordered before comparing? (Should they be based on the equivalence or non-equivalence of compound types with different orders?) In the case of the usage of zio's Has class, it turns outs that different orders are not significant. It represents an unordered set, but not sure if that's generally true. I'm seeing places where I'd expect a smaller diff, where only one component differs, but a lot of instances like X|none appear in the error.

tek commented 4 years ago

they are first grouped into common and unique types, then the unique types are sorted alphabetically and zipped!

joprice commented 4 years ago

Interesting. I'll try to come up with an example of what I saw.

tek commented 4 years ago

I'm just putting the Symbols into sets and intersecting them. I assume that's quite error prone

tek commented 4 years ago

I encourage you to clone the repo and play with it :smile_cat:

tribbloid commented 2 years ago

@joprice I saw this PR:

https://github.com/tek/splain/pull/49

Has significant improvements on ZIO type parsing. Could you test your case again after 2 years? It may be fixed by now

tribbloid commented 9 months ago

this should be fixed, see the test case group "#34" for detail, namely:

object Compound {
  trait T
  type F[A] = A
  def x[A](y: A): F[A with T] = y.asInstanceOf[A with T]
  def f[A](a: A with String): F[A] = a
  val y: F[Int with T] = x(x(1))
  f(y)

  val z: Int with T = x(x(1))
  f(z)
}

compiles into:

newSource1.scala:7: error: type mismatch;
  Compound.y.type|String
  f(y)
    ^
newSource1.scala:10: error: type mismatch;
  Compound.T|<none> with Int|String
  f(z)
    ^
tribbloid commented 9 months ago

closing