From 08d592fd34f60409be52d3b3b929e18928391b45 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Thu, 31 Oct 2024 17:54:29 +0000 Subject: [PATCH 1/2] Resolve name when named imp is behind wild imps When a named import (such as `import bug.util.List`) is defined before two clashing wildcard imports (`import bug.util.*; import java.util.*`) the name "List" should resolve to it, rather than a resolution error being emitted. This was due to the fact that `findRefRecur` didn't return the precedence at which it found that import, `checkImportAlternatives` used the `prevPrec` to `checkNewOrShadowed`. Now we check against the entire `foundResult`, allowing an early named import to be picked over later wildcard imports. --- .../src/dotty/tools/dotc/typer/Typer.scala | 81 ++++++++++--------- tests/pos/i18529/JCode1.java | 9 +++ tests/pos/i18529/JCode2.java | 9 +++ tests/pos/i18529/List.java | 3 + tests/pos/i18529/SCode1.scala | 9 +++ tests/pos/i18529/SCode2.scala | 9 +++ tests/pos/i18529/Test.scala | 1 + 7 files changed, 82 insertions(+), 39 deletions(-) create mode 100644 tests/pos/i18529/JCode1.java create mode 100644 tests/pos/i18529/JCode2.java create mode 100644 tests/pos/i18529/List.java create mode 100644 tests/pos/i18529/SCode1.scala create mode 100644 tests/pos/i18529/SCode2.scala create mode 100644 tests/pos/i18529/Test.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 13f7b4eb1726..c133d70ff9c7 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -241,38 +241,40 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer !owner.isEmptyPackage || ctx.owner.enclosingPackageClass.isEmptyPackage } + import BindingPrec.* + type Result = (Type, BindingPrec) + val NoResult = (NoType, NothingBound) + /** Find the denotation of enclosing `name` in given context `ctx`. - * @param previous A denotation that was found in a more deeply nested scope, - * or else `NoDenotation` if nothing was found yet. - * @param prevPrec The binding precedence of the previous denotation, - * or else `nothingBound` if nothing was found yet. + * @param prevResult A type that was found in a more deeply nested scope, + * and its precedence, or NoResult if nothing was found yet. * @param prevCtx The context of the previous denotation, * or else `NoContext` if nothing was found yet. */ - def findRefRecur(previous: Type, prevPrec: BindingPrec, prevCtx: Context)(using Context): Type = { - import BindingPrec.* + def findRefRecur(prevResult: Result, prevCtx: Context)(using Context): Result = { + val (previous, prevPrec) = prevResult /** Check that any previously found result from an inner context * does properly shadow the new one from an outer context. - * @param found The newly found result - * @param newPrec Its precedence + * @param newResult The newly found type and its precedence. * @param scala2pkg Special mode where we check members of the same package, but defined * in different compilation units under Scala2. If set, and the * previous and new contexts do not have the same scope, we select * the previous (inner) definition. This models what scalac does. */ - def checkNewOrShadowed(found: Type, newPrec: BindingPrec, scala2pkg: Boolean = false)(using Context): Type = + def checkNewOrShadowed(newResult: Result, scala2pkg: Boolean = false)(using Context): Result = + val (found, newPrec) = newResult if !previous.exists || TypeComparer.isSameRef(previous, found) then - found + newResult else if (prevCtx.scope eq ctx.scope) && newPrec.beats(prevPrec) then // special cases: definitions beat imports, and named imports beat // wildcard imports, provided both are in contexts with same scope - found + newResult else if !scala2pkg && !previous.isError && !found.isError then fail(AmbiguousReference(name, newPrec, prevPrec, prevCtx, isExtension = previous.termSymbol.is(ExtensionMethod) && found.termSymbol.is(ExtensionMethod))) - previous + prevResult /** Assemble and check alternatives to an imported reference. This implies: * - If we expand an extension method (i.e. altImports != null), @@ -285,12 +287,13 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer * shadowed. This order of checking is necessary since an outer package-level * definition might trump two conflicting inner imports, so no error should be * issued in that case. See i7876.scala. - * @param previous the previously found reference (which is an import) - * @param prevPrec the precedence of the reference (either NamedImport or WildImport) + * @param prevResult the previously found reference (which is an import), and + * the precedence of the reference (either NamedImport or WildImport) * @param prevCtx the context in which the reference was found * @param using_Context the outer context of `precCtx` */ - def checkImportAlternatives(previous: Type, prevPrec: BindingPrec, prevCtx: Context)(using Context): Type = + def checkImportAlternatives(prevResult: Result, prevCtx: Context)(using Context): Result = + val (previous, prevPrec) = prevResult def addAltImport(altImp: TermRef) = if !TypeComparer.isSameRef(previous, altImp) @@ -305,20 +308,20 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer if prevPrec == WildImport then // Discard all previously found references and continue with `altImp` altImports.clear() - checkImportAlternatives(altImp, NamedImport, ctx)(using ctx.outer) + checkImportAlternatives((altImp, NamedImport), ctx)(using ctx.outer) else addAltImport(altImp) - checkImportAlternatives(previous, prevPrec, prevCtx)(using ctx.outer) + checkImportAlternatives(prevResult, prevCtx)(using ctx.outer) case _ => if prevPrec == WildImport then wildImportRef(curImport) match case altImp: TermRef => addAltImport(altImp) case _ => - checkImportAlternatives(previous, prevPrec, prevCtx)(using ctx.outer) + checkImportAlternatives(prevResult, prevCtx)(using ctx.outer) else - val found = findRefRecur(previous, prevPrec, prevCtx) - if found eq previous then checkNewOrShadowed(found, prevPrec)(using prevCtx) - else found + val foundResult = findRefRecur(prevResult, prevCtx) + if foundResult._1 eq previous then checkNewOrShadowed(foundResult)(using prevCtx) + else foundResult end checkImportAlternatives def selection(imp: ImportInfo, name: Name, checkBounds: Boolean): Type = @@ -408,10 +411,10 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer !noImports && (prevPrec.ordinal < prec.ordinal || prevPrec == prec && (prevCtx.scope eq ctx.scope)) - @tailrec def loop(lastCtx: Context)(using Context): Type = - if (ctx.scope eq EmptyScope) previous + @tailrec def loop(lastCtx: Context)(using Context): Result = + if (ctx.scope eq EmptyScope) prevResult else { - var result: Type = NoType + var result: Result = NoResult val curOwner = ctx.owner /** Is curOwner a package object that should be skipped? @@ -510,7 +513,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer effectiveOwner.thisType.select(name, defDenot).makePackageObjPrefixExplicit } if !curOwner.is(Package) || isDefinedInCurrentUnit(defDenot) then - result = checkNewOrShadowed(found, Definition) // no need to go further out, we found highest prec entry + result = checkNewOrShadowed((found, Definition)) // no need to go further out, we found highest prec entry found match case found: NamedType if curOwner.isClass && isInherited(found.denot) && !ctx.compilationUnit.isJava => @@ -518,29 +521,28 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer case _ => else if migrateTo3 && !foundUnderScala2.exists then - foundUnderScala2 = checkNewOrShadowed(found, Definition, scala2pkg = true) + foundUnderScala2 = checkNewOrShadowed((found, Definition), scala2pkg = true)._1 if (defDenot.symbol.is(Package)) - result = checkNewOrShadowed(previous orElse found, PackageClause) + result = checkNewOrShadowed((previous orElse found, PackageClause)) else if (prevPrec.ordinal < PackageClause.ordinal) - result = findRefRecur(found, PackageClause, ctx)(using ctx.outer) + result = findRefRecur((found, PackageClause), ctx)(using ctx.outer) } - if result.exists then result + if result._1.exists then result else { // find import val outer = ctx.outer val curImport = ctx.importInfo - def updateUnimported() = - if (curImport.nn.unimported ne NoSymbol) unimported += curImport.nn.unimported if (curOwner.is(Package) && curImport != null && curImport.isRootImport && previous.exists) - previous // no more conflicts possible in this case - else if (isPossibleImport(NamedImport) && (curImport nen outer.importInfo)) { - val namedImp = namedImportRef(curImport.uncheckedNN) + prevResult // no more conflicts possible in this case + else if (isPossibleImport(NamedImport) && curImport != null && (curImport ne outer.importInfo)) { + def updateUnimported() = if curImport.unimported ne NoSymbol then unimported += curImport.unimported + val namedImp = namedImportRef(curImport) if (namedImp.exists) - checkImportAlternatives(namedImp, NamedImport, ctx)(using outer) - else if (isPossibleImport(WildImport) && !curImport.nn.importSym.isCompleting) { - val wildImp = wildImportRef(curImport.uncheckedNN) + checkImportAlternatives((namedImp, NamedImport), ctx)(using outer) + else if (isPossibleImport(WildImport) && !curImport.importSym.isCompleting) { + val wildImp = wildImportRef(curImport) if (wildImp.exists) - checkImportAlternatives(wildImp, WildImport, ctx)(using outer) + checkImportAlternatives((wildImp, WildImport), ctx)(using outer) else { updateUnimported() loop(ctx)(using outer) @@ -559,7 +561,8 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer loop(NoContext) } - findRefRecur(NoType, BindingPrec.NothingBound, NoContext) + val (foundRef, foundPrec) = findRefRecur(NoResult, NoContext) + foundRef } /** If `tree`'s type is a `TermRef` identified by flow typing to be non-null, then diff --git a/tests/pos/i18529/JCode1.java b/tests/pos/i18529/JCode1.java new file mode 100644 index 000000000000..e1f12f852c00 --- /dev/null +++ b/tests/pos/i18529/JCode1.java @@ -0,0 +1,9 @@ +package bug.code; + +import bug.util.List; +import bug.util.*; +import java.util.*; + +public class JCode1 { + public void m1(List xs) { return; } +} diff --git a/tests/pos/i18529/JCode2.java b/tests/pos/i18529/JCode2.java new file mode 100644 index 000000000000..2a1ec812852c --- /dev/null +++ b/tests/pos/i18529/JCode2.java @@ -0,0 +1,9 @@ +package bug.code; + +import bug.util.*; +import bug.util.List; +import java.util.*; + +public class JCode2 { + public void m1(List xs) { return; } +} diff --git a/tests/pos/i18529/List.java b/tests/pos/i18529/List.java new file mode 100644 index 000000000000..caf3c0b8036b --- /dev/null +++ b/tests/pos/i18529/List.java @@ -0,0 +1,3 @@ +package bug.util; + +public final class List {} diff --git a/tests/pos/i18529/SCode1.scala b/tests/pos/i18529/SCode1.scala new file mode 100644 index 000000000000..b6796b1540c6 --- /dev/null +++ b/tests/pos/i18529/SCode1.scala @@ -0,0 +1,9 @@ +package bug.code + +import bug.util.List +import bug.util.* +import java.util.* + +class SCode1 { + def work(xs: List[Int]): Unit = {} +} diff --git a/tests/pos/i18529/SCode2.scala b/tests/pos/i18529/SCode2.scala new file mode 100644 index 000000000000..30fc7d0e6f91 --- /dev/null +++ b/tests/pos/i18529/SCode2.scala @@ -0,0 +1,9 @@ +package bug.code + +import bug.util.* +import bug.util.List +import java.util.* + +class SCode2 { + def work(xs: List[Int]): Unit = {} +} diff --git a/tests/pos/i18529/Test.scala b/tests/pos/i18529/Test.scala new file mode 100644 index 000000000000..be7795442a7a --- /dev/null +++ b/tests/pos/i18529/Test.scala @@ -0,0 +1 @@ +class Test From 2d2b2ad4b27db3676389909c026791ea5f09d812 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Mon, 2 Dec 2024 16:16:47 +0000 Subject: [PATCH 2/2] Without boxing, just rerun that a named import is behind a wildcard one --- .../src/dotty/tools/dotc/typer/Typer.scala | 87 ++++++++++--------- 1 file changed, 45 insertions(+), 42 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index c133d70ff9c7..7cd8e3496cd3 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -241,40 +241,44 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer !owner.isEmptyPackage || ctx.owner.enclosingPackageClass.isEmptyPackage } - import BindingPrec.* - type Result = (Type, BindingPrec) - val NoResult = (NoType, NothingBound) - /** Find the denotation of enclosing `name` in given context `ctx`. - * @param prevResult A type that was found in a more deeply nested scope, - * and its precedence, or NoResult if nothing was found yet. + * @param previous A denotation that was found in a more deeply nested scope, + * or else `NoDenotation` if nothing was found yet. + * @param prevPrec The binding precedence of the previous denotation, + * or else `nothingBound` if nothing was found yet. * @param prevCtx The context of the previous denotation, * or else `NoContext` if nothing was found yet. */ - def findRefRecur(prevResult: Result, prevCtx: Context)(using Context): Result = { - val (previous, prevPrec) = prevResult + def findRefRecur(previous: Type, prevPrec: BindingPrec, prevCtx: Context)(using Context): Type = { + import BindingPrec.* /** Check that any previously found result from an inner context * does properly shadow the new one from an outer context. - * @param newResult The newly found type and its precedence. + * @param found The newly found result + * @param newPrec Its precedence * @param scala2pkg Special mode where we check members of the same package, but defined * in different compilation units under Scala2. If set, and the * previous and new contexts do not have the same scope, we select * the previous (inner) definition. This models what scalac does. */ - def checkNewOrShadowed(newResult: Result, scala2pkg: Boolean = false)(using Context): Result = - val (found, newPrec) = newResult + def checkNewOrShadowed(found: Type, newPrec: BindingPrec, scala2pkg: Boolean = false)(using Context): Type = if !previous.exists || TypeComparer.isSameRef(previous, found) then - newResult + found else if (prevCtx.scope eq ctx.scope) && newPrec.beats(prevPrec) then // special cases: definitions beat imports, and named imports beat // wildcard imports, provided both are in contexts with same scope - newResult + found + else if newPrec == WildImport && ctx.outersIterator.exists: ctx => + ctx.isImportContext && namedImportRef(ctx.importInfo.uncheckedNN).exists + then + // Don't let two ambiguous wildcard imports rule over + // a winning named import. See pos/i18529. + found else if !scala2pkg && !previous.isError && !found.isError then fail(AmbiguousReference(name, newPrec, prevPrec, prevCtx, isExtension = previous.termSymbol.is(ExtensionMethod) && found.termSymbol.is(ExtensionMethod))) - prevResult + previous /** Assemble and check alternatives to an imported reference. This implies: * - If we expand an extension method (i.e. altImports != null), @@ -287,13 +291,12 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer * shadowed. This order of checking is necessary since an outer package-level * definition might trump two conflicting inner imports, so no error should be * issued in that case. See i7876.scala. - * @param prevResult the previously found reference (which is an import), and - * the precedence of the reference (either NamedImport or WildImport) + * @param previous the previously found reference (which is an import) + * @param prevPrec the precedence of the reference (either NamedImport or WildImport) * @param prevCtx the context in which the reference was found * @param using_Context the outer context of `precCtx` */ - def checkImportAlternatives(prevResult: Result, prevCtx: Context)(using Context): Result = - val (previous, prevPrec) = prevResult + def checkImportAlternatives(previous: Type, prevPrec: BindingPrec, prevCtx: Context)(using Context): Type = def addAltImport(altImp: TermRef) = if !TypeComparer.isSameRef(previous, altImp) @@ -308,20 +311,20 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer if prevPrec == WildImport then // Discard all previously found references and continue with `altImp` altImports.clear() - checkImportAlternatives((altImp, NamedImport), ctx)(using ctx.outer) + checkImportAlternatives(altImp, NamedImport, ctx)(using ctx.outer) else addAltImport(altImp) - checkImportAlternatives(prevResult, prevCtx)(using ctx.outer) + checkImportAlternatives(previous, prevPrec, prevCtx)(using ctx.outer) case _ => if prevPrec == WildImport then wildImportRef(curImport) match case altImp: TermRef => addAltImport(altImp) case _ => - checkImportAlternatives(prevResult, prevCtx)(using ctx.outer) + checkImportAlternatives(previous, prevPrec, prevCtx)(using ctx.outer) else - val foundResult = findRefRecur(prevResult, prevCtx) - if foundResult._1 eq previous then checkNewOrShadowed(foundResult)(using prevCtx) - else foundResult + val found = findRefRecur(previous, prevPrec, prevCtx) + if found eq previous then checkNewOrShadowed(found, prevPrec)(using prevCtx) + else found end checkImportAlternatives def selection(imp: ImportInfo, name: Name, checkBounds: Boolean): Type = @@ -411,10 +414,10 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer !noImports && (prevPrec.ordinal < prec.ordinal || prevPrec == prec && (prevCtx.scope eq ctx.scope)) - @tailrec def loop(lastCtx: Context)(using Context): Result = - if (ctx.scope eq EmptyScope) prevResult + @tailrec def loop(lastCtx: Context)(using Context): Type = + if (ctx.scope eq EmptyScope) previous else { - var result: Result = NoResult + var result: Type = NoType val curOwner = ctx.owner /** Is curOwner a package object that should be skipped? @@ -513,7 +516,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer effectiveOwner.thisType.select(name, defDenot).makePackageObjPrefixExplicit } if !curOwner.is(Package) || isDefinedInCurrentUnit(defDenot) then - result = checkNewOrShadowed((found, Definition)) // no need to go further out, we found highest prec entry + result = checkNewOrShadowed(found, Definition) // no need to go further out, we found highest prec entry found match case found: NamedType if curOwner.isClass && isInherited(found.denot) && !ctx.compilationUnit.isJava => @@ -521,28 +524,29 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer case _ => else if migrateTo3 && !foundUnderScala2.exists then - foundUnderScala2 = checkNewOrShadowed((found, Definition), scala2pkg = true)._1 + foundUnderScala2 = checkNewOrShadowed(found, Definition, scala2pkg = true) if (defDenot.symbol.is(Package)) - result = checkNewOrShadowed((previous orElse found, PackageClause)) + result = checkNewOrShadowed(previous orElse found, PackageClause) else if (prevPrec.ordinal < PackageClause.ordinal) - result = findRefRecur((found, PackageClause), ctx)(using ctx.outer) + result = findRefRecur(found, PackageClause, ctx)(using ctx.outer) } - if result._1.exists then result + if result.exists then result else { // find import val outer = ctx.outer val curImport = ctx.importInfo + def updateUnimported() = + if (curImport.nn.unimported ne NoSymbol) unimported += curImport.nn.unimported if (curOwner.is(Package) && curImport != null && curImport.isRootImport && previous.exists) - prevResult // no more conflicts possible in this case - else if (isPossibleImport(NamedImport) && curImport != null && (curImport ne outer.importInfo)) { - def updateUnimported() = if curImport.unimported ne NoSymbol then unimported += curImport.unimported - val namedImp = namedImportRef(curImport) + previous // no more conflicts possible in this case + else if (isPossibleImport(NamedImport) && (curImport nen outer.importInfo)) { + val namedImp = namedImportRef(curImport.uncheckedNN) if (namedImp.exists) - checkImportAlternatives((namedImp, NamedImport), ctx)(using outer) - else if (isPossibleImport(WildImport) && !curImport.importSym.isCompleting) { - val wildImp = wildImportRef(curImport) + checkImportAlternatives(namedImp, NamedImport, ctx)(using outer) + else if (isPossibleImport(WildImport) && !curImport.nn.importSym.isCompleting) { + val wildImp = wildImportRef(curImport.uncheckedNN) if (wildImp.exists) - checkImportAlternatives((wildImp, WildImport), ctx)(using outer) + checkImportAlternatives(wildImp, WildImport, ctx)(using outer) else { updateUnimported() loop(ctx)(using outer) @@ -561,8 +565,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer loop(NoContext) } - val (foundRef, foundPrec) = findRefRecur(NoResult, NoContext) - foundRef + findRefRecur(NoType, BindingPrec.NothingBound, NoContext) } /** If `tree`'s type is a `TermRef` identified by flow typing to be non-null, then