unit tests in splain 0.5.9 and scala 2.13.6 are different. Should they be unified? about splain

[tek]

looks fine!

[tribbloid]

They are used for scala version resolving that should be shared between submodules (at this moment there is only core, but you may have more in the future). gradle unfortunately doesn't have a native way to share script code across projects, so the buildSrc need to be compiled and become a dependency on the build classpath

I'll do 2 things:

• create a branch called v0.5/release pointing to our status quo master
• submit a squashed PR from the 1.0.0/gradle branch, after README is completed, and every part in sbt has a counterpart in gradle

WARNING, this PR will be huge. And if submitted as-is, will drastically change the way it is built/tested/published. It will no longer support sbt or specs2, only gradle & scalatest. Please take your time to verify that it is fully compatible with your toolchain

[tek]

sounds sensible!

[tribbloid]

From what I observed in AnalyzerSpec, it appears that the option "-Vimplicits -Vtype-diffs" should always be enabled when using the plugin, otherwise the entire output will be ignored by scalac. Is this correct?

[tek]

yep those are the options that activate splain

[tribbloid]

thanks a lot! Recently, waiting for a solution for this divergence between 2 metaprogramming stages:

https://stackoverflow.com/questions/69428713/scala-runtime-compilation-how-to-get-line-number-of-the-error-message

[tribbloid]

OK I have make most tests from scalac working, but not all:

All failures were caused by this scala macro issue:

scala/bug#6393

It may justify circumventing it instead of solving it head on.

[tribbloid]

@tek If the ammonite interpreter (one component of the Singapore stack) works in our case, would you consider using it?

https://ammonite.io/

[tribbloid]

Before that (or the discovery of any other alternative, I'll simply disable those tests temporarily)

[tek]

using it for what?

[tribbloid]

the ammonite interpreter instead of scala macro interpreter

[tek]

ah, sure

[tribbloid]

Thanks a lot, I also found an inconsistency between code and comment:

  /** Remove duplicates and special cases that should not be shown.
   *  In some cases, candidates are reported twice, once as `Foo.f` and once as
   *  `f`. `ImplicitError.equals` checks the simple names for identity, which
   *  is suboptimal, but works for 99% of cases.
   *  Special cases are handled in [[hideImpError]] */
  def formatNestedImplicits(errors: List[ImplicitError]) = {
    val visible = errors.filterNot(hideImpError)
    val chains  = splitChains(visible).map(_.distinct).distinct
    chains.map(formatImplicitChain).flatMap("" :: _).drop(1)
  }

I never found ImplicitError.equals being used anywhere, errors.distinct is used for the entire chain. In one of the native test cases, 2 chains may have identical head, but their tails are associated with different types:

0 = {Types$ClassNoArgsTypeRef@5495} "ImplicitChain.I1"
1 = {Types$ClassNoArgsTypeRef@5496} "__wrapper$1$852c4a3566114a2ab1521a3424032013.__wrapper$1$852c4a3566114a2ab1521a3424032013.ImplicitChain.I1"

This should be a bug hidden by tests that uses a compiler instead of reflection. Should I change it to de-duplicate by their heads?

[tribbloid]

Ideally, the best data structure to be used here should be a graph or a tree, not chains. Deduplication using chains will always be problematic. E.g. in the Tree test case:

object Tree
{
  implicit def i8(implicit p: I9): I8 = ???
  implicit def i7(implicit p: I8): I7 = ???
  implicit def i6a(implicit p: I7): I6 = ???
  implicit def i6b(implicit p: I8): I6 = ???
  implicit def i5(implicit p: I6): I5 = ???
  implicit def i4(implicit p: I5): I4 = ???
  implicit def i3a(implicit p: I4): I3 = ???
  implicit def i3b(implicit p: I4): I3 = ???
  implicit def i2(implicit p: I3): I2 = ???
  implicit def i1a(implicit p: I2): I1 = ???
  implicit def i1b(implicit p: I6): I1 = ???
  implicitly[I1]
}

The error message was pinned to:

newSource1.scala:28: error: implicit error;
!I e: I1
i1a invalid because
!I p: I2
――i2 invalid because
  !I p: I3
――――i3a invalid because
    !I p: I4
――――――i4 invalid because
      !I p: I5
――――――――i5 invalid because
        !I p: I6
――――――――――i6a invalid because
          !I p: I7
――――――――――――i7 invalid because
            !I p: I8
――――――――――――――i8 invalid because
              !I p: I9

――――――――――i6b invalid because
          !I p: I8
――――――――――――i8 invalid because
            !I p: I9

――――i3b invalid because
    !I p: I4
――――――i4 invalid because
      !I p: I5
――――――――i5 invalid because
        !I p: I6
――――――――――i6a invalid because
          !I p: I7
――――――――――――i7 invalid because
            !I p: I8
――――――――――――――i8 invalid because
              !I p: I9

i1b invalid because
!I p: I6
――i6a invalid because
  !I p: I7
――――i7 invalid because
    !I p: I8
――――――i8 invalid because
      !I p: I9
  implicitly[I1]
            ^

But if you read it carefully you'll see that it omitted the i1b <- i6b <- ... search branch. You can't choose to hide something simply because it has been shown before.

This should be the only bug that was revealed by the test case migration, everything else works properly now.

[tribbloid]

What do you think of the following deduplicating format? Slightly shorter:

newSource1.scala:28: error: implicit error;
!I e: I1
i1a invalid because
!I p: I2
――i2 invalid because
  !I p: I3
――――i3a invalid because
    !I p: I4
――――――i4 invalid because ..............[1]
      !I p: I5
――――――――i5 invalid because
        !I p: I6
――――――――――i6a invalid because .........[2]
          !I p: I7
――――――――――――i7 invalid because
            !I p: I8
――――――――――――――i8 invalid because
              !I p: I9

――――――――――i6b invalid because .........[3]
          !I p: I8
――――――――――――i8 invalid because
            !I p: I9

――――i3b invalid because
    !I p: I4
――――――i4 invalid because ..............[see 1]

i1b invalid because
!I p: I6
――i6a invalid because .................[see 2]
――i6b invalid because .................[see 3]
  implicitly[I1]
            ^

[tek]

I'd expect this to only be significant for very few cases, but knock yourself out!

[tribbloid]

"but knock yourself out!"

Is this German?

I can fix it without introducing the short format with reference, which is a significant, backward incompatible change that will affect all use cases. I'd suggest you to contemplate the design carefully

[tek]

nope that's an English idiom 😅 https://en.wiktionary.org/wiki/knock_oneself_out

regarding the equals comment: If I recall correctly, equals is the default method used to test for equality of class instances, which should be used by distinct.

So this is to say: If you feel like implementing a graph deduplication strategy, please go ahead!

[tribbloid]

awww, if it is an English idiom it must be very old.

I have fixed all existing tests in scalac, the ground truths of the following 2 tests were deemed defective, and are corrected in this patch. These are:

• TreeSpec/verboseTree
• BasicSpec/bounds
  This new output is something I haven't seen before:

newSource1.scala:7: error: implicit error;
!I e: F[Arg]

Bounds.g invalid because
nonconformant bounds;
[Arg, Nothing]
[A <: Bounds.Base, B]
  implicitly[F[Arg]]
            ^

Could you review the changes for these 2 cases and approve them?

All the changes has been pushed into 1.0.0/gradle. If you have problem compiling I'll revert to sbt

[tribbloid]

all test passed now:

https://github.com/tek/splain/runs/3901139610?check_suite_focus=true

[tek]

great!

[tribbloid]

@tek can I ask you to review a few things?

• The added paragraphs in README
• all the ignore unit tests in splain/builtin package.I'm under the impression that some of the features (e.g. breakinfix) are DISCARDED PERMANENTLY, but this can only be verified by your consent that those test cases can be discarded SAFELY

[tek]

readme looks nice!

why are all these files in buildSrc checked in?

regarding the discarded features: those have indeed been completely removed. the only reason to keep them that I can come up with if we were to make PRs to scala to reintroduce them, maybe if users were to request them…but for now, I think it's safe to discard them.

[tribbloid]

Merged, this ticket should be superseded by a new one for acceptance test improvement, closing