Retire implicit shadower in favour of post-hoc analysis of typedImplicit by retronym · Pull Request #7757 · scala/scala

retronym · 2019-02-15T11:29:52Z

The motivations are:

performance (the HashSet in LocalShadower is an allocation hotspot)
correctness (further aligning typedIdent and inferImplicit)

adriaanm · 2019-03-07T09:20:22Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

          Some(collectImplicits(owner.thisType.implicitMembers, owner.thisType))
        }
-      } else if (scope != nextOuter.scope && !owner.isPackageClass) {
+      } else if (scope != nextOuter.scope && !owner.isPackageClass && !(owner.isClass && owner.isInitialized)) {


can we check whether this is local in one shot? Since isClass covers isPackageClass?

adriaanm · 2019-03-07T09:22:57Z

Let's see what the community build says, and if it looks good -- port to 2.13. Too late in the 2.12 cycle to tweak implicit search

SethTisue · 2019-03-07T20:49:03Z

queued:
https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/4236/

retronym · 2019-03-19T03:15:37Z

I'm investigating a failure in the community build:

[scalaz] [error] /home/jenkins/workspace/scala-2.12.x-integrate-community-build/target-0.9.16/project-builds/scalaz-6ccf456dfa5e8117267ec550148b70622d949700/tests/src/test/scala/scalaz/TraverseTest.scala:45: ambiguous implicit values:
[scalaz] [error]  both value idInstance in package scalaz of type => scalaz.Traverse1[scalaz.Id.Id] with scalaz.Monad[scalaz.Id.Id] with scalaz.BindRec[scalaz.Id.Id] with scalaz.Comonad[scalaz.Id.Id] with scalaz.Distributive[scalaz.Id.Id] with scalaz.Zip[scalaz.Id.Id] with scalaz.Unzip[scalaz.Id.Id] with scalaz.Align[scalaz.Id.Id] with scalaz.Cozip[scalaz.Id.Id] with scalaz.Optional[scalaz.Id.Id]
[scalaz] [error]  and value idInstance in package scalaz of type => scalaz.Traverse1[scalaz.Id.Id] with scalaz.Monad[scalaz.Id.Id] with scalaz.BindRec[scalaz.Id.Id] with scalaz.Comonad[scalaz.Id.Id] with scalaz.Distributive[scalaz.Id.Id] with scalaz.Zip[scalaz.Id.Id] with scalaz.Unzip[scalaz.Id.Id] with scalaz.Align[scalaz.Id.Id] with scalaz.Cozip[scalaz.Id.Id] with scalaz.Optional[scalaz.Id.Id]
[scalaz] [error]  match expected type scalaz.Monad[scalaz.Id.Id]
[scalaz] [error]       List.range(0,N).traverseS(x => modify((x: Int) => x+1))(0) must_=== (
[scalaz] [error]                                                              ^

retronym · 2019-03-19T05:02:53Z

Minimized the regression to:

$ tail test/files/pos/implicit-my-package-object-two-files/*.scala
==> test/files/pos/implicit-my-package-object-two-files/A.scala <==
package a

package object b {
  implicit def foo: String = ""
}

==> test/files/pos/implicit-my-package-object-two-files/B.scala <==
package b

object Test {
  import b._

  foo
  implicitly[String]
}

The implicit foo appears twice in context.implicitss, and the leniency around package prefixes in hasMatchingSymbol means that we think that both of them are eligible candidates, but they then are deemed ambiguous. Previously, LocalShadower ruled out the candidate provided from the enclosing package's this-type.

retronym · 2019-03-19T06:15:47Z

I've pushed a fix that still reinstates a simpler form of shadowing checks into the search. Because of the improvements to hasMatchingSymbol, all this needs to do is post-filter the eligible candidates. That list if often empty, so we're not paying the cost to allocate the HashSet anywhere near as much.

…typedImplicit I'm not 100% sure about neg/t729.scala, which I've converted to a pos test. The original bug report is archived: https://lrytz.github.io/scala-aladdin-bugtracker/displayItem.do%3Fid=729.html

Mark the input `Ident` with an attachment and search for that to find the corresponding tree in the type checked output. `itree2` does _not_ have macros expanded (even whitebox), that comes later in `itree3`. I added a test for such cases to be sure.

retronym · 2019-03-21T03:31:31Z

https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/4273

diesalbla · 2019-03-22T23:56:25Z

@retronym Now that #7890 has been merged, do you still want to push for this one?

retronym · 2019-05-28T09:02:42Z

@diesalbla It is still worthwhile. I realise now that simply re-using the java.util.HashSet doesn't avoid allocation of its internal Node classes.

I'm now also considering a different change (retronym#49) that doesn't change current semantics. I'll close this PR now and do my benchmarking over there :)

scala-jenkins added this to the 2.12.9 milestone Feb 15, 2019

retronym added the WIP label Feb 15, 2019

retronym force-pushed the topic/retire-shadower-again branch from 1192539 to fb27e4d Compare February 15, 2019 11:31

retronym mentioned this pull request Feb 15, 2019

SI-9208 Retire implicit shadower #5555

Closed

retronym force-pushed the topic/retire-shadower-again branch from fb27e4d to 7c2d2bd Compare February 16, 2019 04:57

diesalbla mentioned this pull request Feb 19, 2019

Avoid creating lists in Implicits - eligible #7769

Closed

retronym force-pushed the topic/retire-shadower-again branch from 7c2d2bd to 86ecd0a Compare March 1, 2019 04:19

som-snytt mentioned this pull request Mar 1, 2019

Implicit shadowing fail scala/bug#11421

Closed

retronym force-pushed the topic/retire-shadower-again branch from 6050707 to 593999b Compare March 4, 2019 03:12

retronym requested a review from adriaanm March 7, 2019 05:22

adriaanm self-assigned this Mar 7, 2019

adriaanm reviewed Mar 7, 2019

View reviewed changes

retronym force-pushed the topic/retire-shadower-again branch from 593999b to 42403cf Compare March 19, 2019 06:14

retronym added 2 commits March 19, 2019 16:34

Retire implicit shadower in favour of post-hoc analysis of result of …

552b593

…typedImplicit I'm not 100% sure about neg/t729.scala, which I've converted to a pos test. The original bug report is archived: https://lrytz.github.io/scala-aladdin-bugtracker/displayItem.do%3Fid=729.html

retronym force-pushed the topic/retire-shadower-again branch from 42403cf to 87aab31 Compare March 19, 2019 06:34

retronym closed this May 28, 2019

retronym mentioned this pull request May 28, 2019

Introduce a new implementation of implicit shadowing retronym/scala#49

Closed

SethTisue removed this from the 2.12.9 milestone May 28, 2019

retronym mentioned this pull request May 29, 2019

Introduce a new implementation of implicit shadowing #8109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retire implicit shadower in favour of post-hoc analysis of typedImplicit#7757

Retire implicit shadower in favour of post-hoc analysis of typedImplicit#7757
retronym wants to merge 2 commits intoscala:2.12.xfrom
retronym:topic/retire-shadower-again

retronym commented Feb 15, 2019

Uh oh!

adriaanm Mar 7, 2019

Uh oh!

adriaanm commented Mar 7, 2019

Uh oh!

SethTisue commented Mar 7, 2019 •

edited

Loading

Uh oh!

retronym commented Mar 19, 2019

Uh oh!

retronym commented Mar 19, 2019 •

edited

Loading

Uh oh!

retronym commented Mar 19, 2019

Uh oh!

retronym commented Mar 21, 2019

Uh oh!

diesalbla commented Mar 22, 2019

Uh oh!

retronym commented May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

retronym commented Feb 15, 2019

Uh oh!

adriaanm Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

adriaanm commented Mar 7, 2019

Uh oh!

SethTisue commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

retronym commented Mar 19, 2019

Uh oh!

retronym commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

retronym commented Mar 19, 2019

Uh oh!

retronym commented Mar 21, 2019

Uh oh!

diesalbla commented Mar 22, 2019

Uh oh!

retronym commented May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SethTisue commented Mar 7, 2019 •

edited

Loading

retronym commented Mar 19, 2019 •

edited

Loading