Add SIP compliant by-name implicits #5461
Conversation
milessabin
commented
Nov 18, 2018
milessabin
commented
- Replaced existing implementation of by-name implicits with a SIP compliant one.
- Ported the Scala 2 tests.
- Removed now redundant -Xmin-implicit-search-depth.
|
Some description in the commit of how the new semantics differs from what was implemented before would also be nice |
| implicitly[C[C[C[C[C[C[String]]]]]]] |
There was a problem hiding this comment.
How come this does not diverge anymore ?
There was a problem hiding this comment.
It doesn't diverge because the reduction steps are either neutral or reduce the size of the type being resolved. The description of the algorithm in the SIP explains in more detail.
There was a problem hiding this comment.
The reason it diverged previously is because the only check on divergence was a hard limit (via a command line argument) on the number of reduction steps permitted.
There was a problem hiding this comment.
I see, can you also add a neg test where an implicit search diverges now ?
There was a problem hiding this comment.
All the neg tests in this PR report divergence.
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Agreed that there needs to be more documentation in the PR. That said, the algorithm is described in detail in the SIP, complete with a termination proof. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/5461/ to see the changes. Benchmarks is based on merging with master (0a1aed2) |
milessabin
force-pushed
the
topic/byname-implicits
branch
from
416e315
to
1baaf04
Compare
|
Rebased, added documentation and addressed feedback. |
milessabin
dismissed
odersky’s stale review
Documentation added and addressed your and @smarter's comments.
| * search history (`root`) which in turn maintains a possibly empty dictionary of | ||
| * recursive implicit terms constructed during this search. | ||
| * | ||
| * A search history provides operations to created a nested search history, check for |
| // as we ascend the chain of open implicits to the outermost search context. | ||
|
|
||
| @tailrec | ||
| def loop(ois: List[(Candidate, Type)], belowByname: Boolean): Boolean = { |
There was a problem hiding this comment.
redundant pair of braces? We usually put the match directly after the =.
| ois match { | ||
| case Nil => false | ||
| case (hd@(cand1, tp)) :: tl => | ||
| (if (cand1.ref == cand.ref) { |
There was a problem hiding this comment.
Why not fuse the scrutinee and the match? I.e.
if (cand1.ref == cand.ref) {
val wideTp = tp.widenExpr
lazy val wildTp = wildApprox(wideTp)
if (belowByname && (wildTp <:< wildPt)) false
else if ((wideTp.typeSize < ptSize && wideTp.coveringSet == ptCoveringSet) || (wildTp == wildPt)) true
else loop(tl, isByname(tp) || belowByname)
}
else loop(tl, isByname(tp) || belowByname)The recursive call is duplicated, but everything else is shorter and nicer (and less boxing, too).
| else { | ||
| result match { | ||
| case failure: SearchFailure => failure | ||
| case success@SearchSuccess(tree, _, _) => |
| tpd.ref(vsymMap(id.symbol)) | ||
| case tree => tree | ||
| }) | ||
| val nrhss = rhss.map(rhs => rhsMap(rhs)) |
|
Looks generally good to me. |
* Replaced existing implementation of by-name implicits with a SIP compliant one. * Ported the Scala 2 tests. * Removed now redundant -Xmin-implicit-search-depth.
* Fixed whitespace and comment typos. * Simplified loop logic in checkDivergence.
milessabin
force-pushed
the
topic/byname-implicits
branch
from
1baaf04
to
32b6907
Compare
|
Incorporated review feedback and rebased. I'll merge when it goes green. |
Assigned number 36 to SIP “By-name implicit arguments”. Implemented in Scala 2.13 in scala/scala#6050 Implemented in Scala 3 in scala/scala3#5461
Assigned number 36 to SIP “By-name implicit arguments”. Implemented in Scala 2.13 in scala/scala#6050 Implemented in Scala 3 in scala/scala3#5461
