Closed tribbloid closed 2 years ago
sounds sensible!
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?
yep those are the options that activate splain
thanks a lot! Recently, waiting for a solution for this divergence between 2 metaprogramming stages:
OK I have make most tests from scalac working, but not all:
All failures were caused by this scala macro issue:
https://github.com/scala/bug/issues/6393
It may justify circumventing it instead of solving it head on.
@tek If the ammonite interpreter (one component of the Singapore stack) works in our case, would you consider using it?
Before that (or the discovery of any other alternative, I'll simply disable those tests temporarily)
using it for what?
the ammonite interpreter instead of scala macro interpreter
ah, sure
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?
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.
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]
^
I'd expect this to only be significant for very few cases, but knock yourself out!
"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
nope that's an English idiom :sweat_smile: 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!
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:
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
looks fine!
all test passed now:
https://github.com/tek/splain/runs/3901139610?check_suite_focus=true
great!
@tek can I ask you to review a few things?
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.
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:
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
Merged, this ticket should be superseded by a new one for acceptance test improvement, closing
Just one quick example, for the test "chain":
In scala 2.13.6:
In splain 0.5.9:
I assume this is caused by a few bugfixes introduced after 0.5.8.
Now the problem becomes: should the latest tests in our plugin BE A SUPERSET of scalac test?
If so, our tests should have identical format with tests in scalac, afterwards we can easily include their tests without rewriting them
The plugin is already in production, people expect upcoming changes not breaking any existing behaviour (unless for a good reason)