SI-9881 Fix ImportHandler's reporting of importedNames and importedSymbols #5326
Conversation
|
@retronym I cherry-picked some of your changes from https://github.com/retronym/scala/tree/review/4698 to filter out flattened symbols. It seems there hasn't been activity in that branch in the past year. Are you still working on it? If there is pending work in the area, please let me know and we can coordinate that. |
|
Thanks for picking this up. My branch is dormant, I'm not currently working on this. |
retronym
changed the title
|
@retronym can you help review this PR, especially the changes I cherry-picked from your branch? |
There was a problem hiding this comment.
There are some solid improvements in here, thank you! I know it's been too long, but could you take a look at a few of my suggestions? To avoid test failures, I'd suggest moving the fix for index of out bounds in names first, and squashing the WIP commit into your own.
| @@ -4549,7 +4549,12 @@ trait Types | |||
|
|
|||
| /** Members which can be imported into other scopes. | |||
| */ | |||
| def importableMembers(pre: Type): Scope = pre.members filter isImportable | |||
| def importableMembers(pre: Type): Scope = { | |||
There was a problem hiding this comment.
I think this change is too risky for 2.11.9. How about moving it to the invocation of importableMembers in the repl?
There was a problem hiding this comment.
Will move to MemberHandlers
| } | ||
| private def importableTargetMembers = importableMembers(targetType).toList | ||
| private def importableTargetMembers = importableMembers(exitingTyper(targetType)).toList |
There was a problem hiding this comment.
Does this PR introduce any new call-sites of importableTargetMembers that aren't already in exitingTyper?
Currently, IntelliJ says:
lazy val individualSymbols: List[Symbol] = exitingTyper(importableTargetMembers filter (m => selectorNames(m.name)))
lazy val wildcardSymbols: List[Symbol] = exitingTyper(if (importsWildcard) importableTargetMembers else Nil)
There was a problem hiding this comment.
Nope it doesn't introduce new call sites. I just moved exitingTyper from both call sites to importableMembers itself.
| @@ -214,27 +214,34 @@ trait MemberHandlers { | |||
| val Import(expr, selectors) = imp | |||
| def targetType = intp.global.rootMirror.getModuleIfDefined("" + expr) match { | |||
| case NoSymbol => intp.typeOfExpression("" + expr) | |||
| case sym => sym.thisType | |||
| case sym => sym.moduleClass.thisType | |||
There was a problem hiding this comment.
Good catch!
/** If symbol is a class, the type `this.type` in this class,
* otherwise `NoPrefix`.
* We always have: thisType <:< typeOfThis
*/
def thisType: Type = NoPrefix
From the repl's :power mode:
scala> object o
defined object o
scala> val module = typeOf[o.type].termSymbol
module: $r.intp.global.Symbol = object o
scala> module.thisType
res7: $r.intp.global.Type = <noprefix>
scala> module.moduleClass.thisType
res8: $r.intp.global.Type = o.type
However, this can be had more directly:
scala> module.tpe
res12: $r.intp.global.Type = o.type
There was a problem hiding this comment.
Will use the simpler version.
|
|
||
| // non-wildcard imports | ||
| private def individualSelectors = selectors filter { x => | ||
| x.name != nme.USCOREkw && x.name != null && x.rename != nme.USCOREkw & x.rename != null |
There was a problem hiding this comment.
Could we encapsulate this stuff in Contexts, say? Looks like we already have warnUnusedImports, which compares against the more logical nme.WILDCARD, by the way.
There was a problem hiding this comment.
Will create some helpers in Contexts.
|
Ping! If you'd like this to be considered for 2.11.9, please let us know by the end of the week and we'll do our best to help you get this over the finish line. |
|
Please consider resubmitting for 2.12.2! |
|
@adriaanm Sorry I didn't get back to this in time. I'll rebase against 2.12.x and address your comments. |
This PR attempts to fix
importedNamesandimportedSymbolsinImportHandler. Currently they are broken, causing the following issues:Changes in this PR:
names&renames. Usenamesto lookup symbols from owner. Userenamesto constructimportedNames.sym.thisTypewill never work for module symbols. Change tosym.typeOfThissym.moduleClass.thisType.typeOfExpressioninadvertently resolves type underjvmphase, disregarding requested phase from caller. This is used inImportHandlerto findimportableMembersfrom objects.importableMembers.