Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Exclude universal members (getClass, toString, etc) from root module import #8541

Merged
merged 1 commit into from Nov 18, 2019
Merged

Exclude universal members (getClass, toString, etc) from root module import #8541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/ast/TreeGen.scala
View file
Open in desktop
Expand Up @@ -39,7 +39,7 @@ abstract class TreeGen extends scala.reflect.internal.TreeGen with TreeDSL {
def mkImport(qualSym: Symbol, name: Name, toName: Name): Import =
mkImportFromSelector(qualSym, ImportSelector(name, 0, toName, 0) :: Nil)

private def mkImportFromSelector(qualSym: Symbol, selector: List[ImportSelector]): Import = {
def mkImportFromSelector(qualSym: Symbol, selector: List[ImportSelector]): Import = {
assert(qualSym ne null, this)
val qual = gen.mkAttributedStableRef(qualSym)
val importSym = (
Expand Down
23 changes: 15 additions & 8 deletions src/compiler/scala/tools/nsc/typechecker/Contexts.scala
View file
Open in desktop
Expand Up @@ -13,16 +13,17 @@
package scala.tools.nsc
package typechecker

import scala.collection.{immutable, mutable}
import scala.annotation.tailrec
import scala.reflect.internal.util.{ ReusableInstance, shortClassOfInstance, ListOfNil, SomeOfNil }
import scala.collection.{immutable, mutable}
import scala.reflect.internal.util.{ReusableInstance, shortClassOfInstance, ListOfNil, SomeOfNil}
import scala.util.chaining._

/**
* @author Martin Odersky
*/
trait Contexts { self: Analyzer =>
import global._
import definitions.{JavaLangPackage, ScalaPackage, PredefModule, ScalaXmlTopScope, ScalaXmlPackage}
import definitions.{JavaLangPackage, ScalaPackage, PredefModule, ScalaXmlTopScope, ScalaXmlPackage, isUniversalMember}
import ContextMode._
import scala.reflect.internal.Flags._

Expand Down Expand Up @@ -137,7 +138,16 @@ trait Contexts { self: Analyzer =>
}

def rootContext(unit: CompilationUnit, tree: Tree = EmptyTree, throwing: Boolean = false, checking: Boolean = false): Context = {
val rootImportsContext = rootImports(unit).foldLeft(startContext)((c, sym) => c.make(gen.mkWildcardImport(sym), unit = unit))
val rootImportsContext = rootImports(unit).foldLeft(startContext) { (c, sym) =>
if (sym.isPackage) c.make(gen.mkWildcardImport(sym), unit = unit)
else {
val universals = sym.typeSignature.members.filter(isUniversalMember).map(_.name).toSet
val sels = universals.iterator
.map(ImportSelector.mask).toList
.appended(ImportSelector.wild)
c.make(gen.mkImportFromSelector(sym, sels), unit = unit)
Copy link
Member

@retronym retronym Nov 20, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely for minor values of major.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was more worried about compiler performance when merging this one: https://github.com/scala/scala/pull/8538/files#diff-62ccdfe29e58b8e9f06ed66d4cfc88a5R590-R593

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically Any is not your friend. And how could it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

// there must be a scala.xml package when xml literals were parsed in this unit
if (unit.hasXml && ScalaXmlPackage == NoSymbol)
Expand All @@ -150,10 +160,7 @@ trait Contexts { self: Analyzer =>
if (!unit.hasXml || ScalaXmlTopScope == NoSymbol) rootImportsContext
else rootImportsContext.make(gen.mkImport(ScalaXmlPackage, nme.TopScope, nme.dollarScope))

val c = contextWithXML.make(tree, unit = unit)

c.initRootContext(throwing, checking)
c
contextWithXML.make(tree, unit = unit).tap(_.initRootContext(throwing, checking))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fancy!

}

def rootContextPostTyper(unit: CompilationUnit, tree: Tree = EmptyTree): Context =
Expand Down
1 change: 1 addition & 0 deletions src/reflect/scala/reflect/internal/Trees.scala
View file
Open in desktop
Expand Up @@ -498,6 +498,7 @@ trait Trees extends api.Trees {
val wild = ImportSelector(nme.WILDCARD, -1, null, -1)
val wildList = List(wild) // OPT This list is shared for performance.
def wildAt(pos: Int) = ImportSelector(nme.WILDCARD, pos, null, -1)
def mask(name: Name) = ImportSelector(name, -1, nme.WILDCARD, -1)
}

case class Import(expr: Tree, selectors: List[ImportSelector])
Expand Down
4 changes: 4 additions & 0 deletions test/files/neg/t5389.check
View file
Open in desktop
@@ -0,0 +1,4 @@
t5389.scala:2: error: not found: object ne
import ne.scala
^
one error found
4 changes: 4 additions & 0 deletions test/files/neg/t5389.scala
View file
Open in desktop
@@ -0,0 +1,4 @@

import ne.scala
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So @jducoeur's paying you under the table after all, hmm.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've owned the domain for less than 24 hours, and I'm already a world power!


class C