Exclude universal members (getClass, toString, etc) from root module import #8541
Conversation
There was a problem hiding this comment.
Thanks. I hate to give the impression that I'm an easier reviewer than all the rest, but lgtm.
This can't have been strange enough to be a puzzler, right? I'm always concerned that we give thought to breaking our users but very little to the puzzlers guy. Should we add it to the community build?
| @@ -0,0 +1,4 @@ | |||
|
|
|||
| import ne.scala | |||
There was a problem hiding this comment.
So @jducoeur's paying you under the table after all, hmm.
There was a problem hiding this comment.
I've owned the domain for less than 24 hours, and I'm already a world power!
|
|
||
| c.initRootContext(throwing, checking) | ||
| c | ||
| contextWithXML.make(tree, unit = unit).tap(_.initRootContext(throwing, checking)) |
|
The improvement would be to construct the masking selectors for universal members only once. |
| val sels = universals.iterator | ||
| .map(ImportSelector.mask).toList | ||
| .appended(ImportSelector.wild) | ||
| c.make(gen.mkImportFromSelector(sym, sels), unit = unit) |
There was a problem hiding this comment.
This hurts compiler performance markedly. I haven't investigated profiles, so I'm not sure whether the cost is creating this Import or the overhead of subsequent symbol lookups using it rather than the simple wildcard.
Could we instead make ImportInfo.importedSymbol exclude these, perhaps conditionally on isRootImport?
It already filters its results through isUnimportable which excludes constructors and private[this]. There is also logic in there that purports to stop all imports of members of AnyRef, but was disabled fully, maybe unintentionally, in 2b4cd6c.
There was a problem hiding this comment.
Definitely for minor values of major.
There was a problem hiding this comment.
I was more worried about compiler performance when merging this one: https://github.com/scala/scala/pull/8538/files#diff-62ccdfe29e58b8e9f06ed66d4cfc88a5R590-R593
There was a problem hiding this comment.
Basically Any is not your friend. And how could it be?
There was a problem hiding this comment.
JZ's improvement in the linked PR. I'll follow up by running benchmarks and then by putting LR's mind at ease, if I am able.
|
I've added the "release notes" since two projects in the community build were affected: akka/akka#28355, lagom/lagom#2579 |
|
See hrhino's prescient approval comment above:
Also the part where he cops to being an easy review. It's been said many times on this project that there is no bug so outré that no code in the wild relies on it. This behavior was actually as specified and not a bug; it could have been a lint. In fact, I am inclined to demote it to lint. WDYT @SethTisue @hrhino ? Or leave it as it is, because it's always a bug? In that case, we can make it a non-lint warning without irking anyone; a behavior change can be irksome; if only we could specify what java.lang and scala.Predef export! |
SethTisue
changed the title
|
I think I would have also signed off on a PR that made it a lint. mainly, I am happy we now have one or the other, rather than neither. |
|
Thanks for the reminder to get my NEScala tickets. |
|
So long as you both are happy. I don't like that it's a bit of a special case, but there's nothing which says the spec can't specify that root contexts work like the first implementation, masking |
definitions.isImportablemakes the faulty assertion that universal members are not importable.In fact, they are imported by default from
Predef. This is observable in scala 2 imports and in scala 3 top level definitions.Fixes scala/bug#5389