tek / splain

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

splain hiding original compiler information in cases of simple type mismatch. #47

Closed tribbloid closed 2 years ago

tribbloid commented 3 years ago

This is a problem that manifests for different kind of errors. The most simple one being a type signature mismatch WITHOUT implicit search. E.g.:

(before applying splain):


[Error] /home/peng/git/shapesafe/macro/src/main/scala/com/tribbloids/shapesafe/m/shape/op/EinSumOps.scala:16: value GetField is not a member of object com.tribbloids.shapesafe.m.util.RecordUtils
[Error] /home/peng/git/shapesafe/macro/src/main/scala/com/tribbloids/shapesafe/m/util/RecordUtils.scala:20: type mismatch;
 found   : shapeless.ops.record.Selector[H,W#T]
 required: shapeless.ops.record.Selector[H,w.T]
Note: W#T >: w.T, but trait Selector is invariant in type K.
You may wish to define K as -K instead. (SLS 4.5)
 Note: implicit method getter is not applicable here because it comes after the application point and it lacks an explicit result type
two errors foun

(after):

[Error] /home/peng/git/shapesafe/macro/src/main/scala/com/tribbloids/shapesafe/m/shape/op/EinSumOps.scala:16: value GetField is not a member of object com.tribbloids.shapesafe.m.util.RecordUtils
[Error] /home/peng/git/shapesafe/macro/src/main/scala/com/tribbloids/shapesafe/m/util/RecordUtils.scala:20: type mismatch;
  ops.record.Selector[H, W.T]
 Note: implicit method getter is not applicable here because it comes after the application point and it lacks an explicit result type

The problem is that scala compiler verbosity has gradually improve over time (as well as capabilities, so old presentations of advanced types gradually became obsolete)

tek commented 3 years ago

yeah, there are quite a few of those little problems. Not sure whether it's worthwile to fix before splain has been merged into the compiler though.

tribbloid commented 3 years ago

really? There is a PR to merge splain into scala-compiler? I initially thought it starts from a backport from a dotty feature.

Personally I don't see the problem of splain being a compiler plugin as is. The point is being an augmentation, not a replacement. This means that the black magic in representations doesn't need to stay up-to-date. As long as the latest version of the original compiler's log is preserved authentically. splain did a good job for some special cases, e.g. for any abstract type with @ImplicitNotFound annotation. (may be because it is an old compiler feature?). We just need to make sure that such display is general enough and is agnostic to changes (from those whimsical grad students)

(I prefer to mostly use well-known reflection APIs like toString but that's just my opinion)

tek commented 3 years ago

yes, here: https://github.com/scala/scala/pull/7785 has been a few years already :sweat_smile:

I would say that one part of the motivation for splain's terse output is to hide messages that are obvious to the seasoned programmer. Of course it's subjective whether this particular note is useful. However, I would assume that preserving this one shouldn't be that much of a challenge.

tribbloid commented 3 years ago

That's a long and still active thread, looks like the implicit tree feature got a lot of support, and you are so close to getting approved

IMHO some of the core committers share my concerns: the presentation is too different from dotty and removes information that may be obvious in SOME cases. It appears that your best shot to push through is to mimic dotty's message at default (convention over configuration), and open up improved/reduced verbosity when seasoned user explicitly asked

Those "SOME cases" may works for advanced users 7 years ago, but right now they are dealing with some particular savage beasts. Like thousands of dependent types that each depends on an object/singleton somewhere in the memory space. Or refined type of which member definitions sprawl over 2 screens. Plus the library's name is 'splain', as in 'Stephen Colbert's white man-splanation'

My short-term intention is still bringing native compiler message back (in other words, to be on par with the case when @ImplicitNotFound is define), until test coverage is high enough and agreed by a dotty maintainer. Do you know part of the code that handles messages like @ImplicitNotFound?

tek commented 3 years ago

since you appear to have a pretty clear view of what is necessary and I haven't been doing any Scala lately, would you like to have write access to the repo?

tribbloid commented 3 years ago

sure I wouldn't mind co-maintaining the project. But my use case is quite narrow: https://github.com/tribbloid/shapesafe

Do you know other users that heavily relies on splain?

tek commented 3 years ago

incidentally, one of the initial motivators for splain was a project I built with breeze as well :sweat_smile: I would point out though that your use case is a bit more extreme than the usual "show the chain of json instances" problem that most people have. Not to deny the validity of your endeavours though, so if you're willing to put in the work, I'm more than happy to have you collaborate.

I only know about the issues people file here, the feeback on the compiler PR, and that there are about 15k downloads a month. I have not received many usage reports by users.

tribbloid commented 3 years ago

I also found that many problems like this was supposed to be captured by tests some time ago. But some test ground truths are weird. E.g. the divergence test:

object Diverging
{
  implicit def f(implicit c: C): C = ???
  implicitly[C]
}

has an empty error message as ground truth.

The issue here is that the revised ground truth may carries the entire package path, including those wrapper$xxx that are generated randomly by the compilation fixture. Do you recommend making these random package & object names deterministic?

tek commented 3 years ago

probably wouldn't hurt!

curious, looks like I only added the code for that test and never included it in ErrorsSpec. No clue what my plan was there. Maybe to do some additional analysis.

tribbloid commented 2 years ago

After trying the latest build the problem appears to be gone. Closing