From 1a538af06323f7d6cd471ae0af39842f26e9e7be Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 23 Aug 2016 15:48:48 +0200 Subject: [PATCH 1/4] Fix #1457: Three incompatbilities with scalac Two of these are unavoidable. I moved the tests to diabled/not-representable and added in each case a comment to the main scala file detailing why there is a deviation. The last one (import-rewrite) is fixed. --- src/dotty/tools/dotc/typer/ImportInfo.scala | 1 + .../not-representable}/hkt/compiler.error | 0 .../not-representable}/hkt/hkt.scala | 3 +++ .../not-representable/naming-resolution/callsite.scala | 10 ++++++++++ .../not-representable/naming-resolution/compiler.error | 8 ++++++++ .../not-representable/naming-resolution/package.scala | 5 +++++ tests/pending/import-rewrite/compiler.error | 6 ------ tests/{pending => pos}/import-rewrite/file.scala | 0 tests/{pending => pos}/import-rewrite/rewrite.scala | 0 9 files changed, 27 insertions(+), 6 deletions(-) rename tests/{pending => disabled/not-representable}/hkt/compiler.error (100%) rename tests/{pending => disabled/not-representable}/hkt/hkt.scala (68%) create mode 100644 tests/disabled/not-representable/naming-resolution/callsite.scala create mode 100644 tests/disabled/not-representable/naming-resolution/compiler.error create mode 100644 tests/disabled/not-representable/naming-resolution/package.scala delete mode 100644 tests/pending/import-rewrite/compiler.error rename tests/{pending => pos}/import-rewrite/file.scala (100%) rename tests/{pending => pos}/import-rewrite/rewrite.scala (100%) diff --git a/src/dotty/tools/dotc/typer/ImportInfo.scala b/src/dotty/tools/dotc/typer/ImportInfo.scala index 2ca90311feba..2105d9ccc9de 100644 --- a/src/dotty/tools/dotc/typer/ImportInfo.scala +++ b/src/dotty/tools/dotc/typer/ImportInfo.scala @@ -64,6 +64,7 @@ class ImportInfo(symf: => Symbol, val selectors: List[untpd.Tree], val isRootImp myExcluded += name case Pair(Ident(from: TermName), Ident(to: TermName)) => myMapped = myMapped.updated(to, from) + myExcluded += from myOriginals += from case Ident(nme.WILDCARD) => myWildcardImport = true diff --git a/tests/pending/hkt/compiler.error b/tests/disabled/not-representable/hkt/compiler.error similarity index 100% rename from tests/pending/hkt/compiler.error rename to tests/disabled/not-representable/hkt/compiler.error diff --git a/tests/pending/hkt/hkt.scala b/tests/disabled/not-representable/hkt/hkt.scala similarity index 68% rename from tests/pending/hkt/hkt.scala rename to tests/disabled/not-representable/hkt/hkt.scala index 34858cd95009..1a9932d7348d 100644 --- a/tests/pending/hkt/hkt.scala +++ b/tests/disabled/not-representable/hkt/hkt.scala @@ -1,3 +1,6 @@ +// This one is unavoidable. Dotty does not allow several overloaded +// parameterless methods, so it picks the one in the subclass. + import scala.language.higherKinds // Minimal reproduction for: // scala.collection.mutable.ArrayStack.empty[Int] diff --git a/tests/disabled/not-representable/naming-resolution/callsite.scala b/tests/disabled/not-representable/naming-resolution/callsite.scala new file mode 100644 index 000000000000..036803a26930 --- /dev/null +++ b/tests/disabled/not-representable/naming-resolution/callsite.scala @@ -0,0 +1,10 @@ +// This one should be rejected according to spec. The import takes precedence +// over the type in the same package because the typeis declared in a +// different compilation unit. scalac does not conform to spec here. +package naming.resolution + +import java.nio.file._ // Imports `Files` + +object Resolution { + def gimmeFiles: Files = Files.list(Paths.get(".")) +} diff --git a/tests/disabled/not-representable/naming-resolution/compiler.error b/tests/disabled/not-representable/naming-resolution/compiler.error new file mode 100644 index 000000000000..81d6b3cfaf91 --- /dev/null +++ b/tests/disabled/not-representable/naming-resolution/compiler.error @@ -0,0 +1,8 @@ +$ scalac tests/pending/naming-resolution/*.scala +$ ./bin/dotc tests/pending/naming-resolution/*.scala +tests/pending/naming-resolution/callsite.scala:6: error: type mismatch: + found : java.util.stream.Stream[java.nio.file.Path] + required: java.nio.file.Files + def gimmeFiles: Files = Files.list(Paths.get(".")) + ^ +one error found diff --git a/tests/disabled/not-representable/naming-resolution/package.scala b/tests/disabled/not-representable/naming-resolution/package.scala new file mode 100644 index 000000000000..f0e26ee95ad7 --- /dev/null +++ b/tests/disabled/not-representable/naming-resolution/package.scala @@ -0,0 +1,5 @@ +package naming + +package object resolution { + type Files = java.util.stream.Stream[java.nio.file.Path] +} diff --git a/tests/pending/import-rewrite/compiler.error b/tests/pending/import-rewrite/compiler.error deleted file mode 100644 index 0832d33bb914..000000000000 --- a/tests/pending/import-rewrite/compiler.error +++ /dev/null @@ -1,6 +0,0 @@ -$ scalac tests/pending/import-rewrite/*.scala -$ ./bin/dotc tests/pending/import-rewrite/*.scala -tests/pending/import-rewrite/rewrite.scala:5: error: value apply is not a member of java.io.File.type - Seq("").map(File.apply) - ^ -one error found diff --git a/tests/pending/import-rewrite/file.scala b/tests/pos/import-rewrite/file.scala similarity index 100% rename from tests/pending/import-rewrite/file.scala rename to tests/pos/import-rewrite/file.scala diff --git a/tests/pending/import-rewrite/rewrite.scala b/tests/pos/import-rewrite/rewrite.scala similarity index 100% rename from tests/pending/import-rewrite/rewrite.scala rename to tests/pos/import-rewrite/rewrite.scala From 99afc892839ca6209786ccd0dbca2544633be2e8 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 26 Aug 2016 18:29:17 +0200 Subject: [PATCH 2/4] Accommodate Scala2 name resolution scheme Scala2 does not conform to spec Section 2, where it says: Bindings of different kinds have a precedence defined on them: 1. Definitions and declarations that are local, inherited, or made available by a package clause and also defined in the same compilation unit as the reference, have highest precedence. 2. Explicit imports have next highest precedence. 3. Wildcard imports have next highest precedence. 4. Definitions made available by a package clause, but not also defined in the same compilation unit as the reference, have lowest precedence. In fact Scala 2, merges (1) and (4) into highest precedence. This commit simulates the Scala2 behavior under -language:Scala2, but gives a migration warning. For the naming-resolution test case we get: dotc *.scala -language:Scala2 -migration callsite.scala:9: migration warning: Name resolution will change. currently selected : naming.resolution.Files in the future, without -language:Scala2: java.nio.file.Files' where Files is a type in package object package which is an alias of java.util.stream.Stream[java.nio.file.Path] Files' is a class in package file def gimmeFiles: Files = Files.list(Paths.get(".")) ^ one warning found --- src/dotty/tools/dotc/typer/Typer.scala | 31 ++++++++++++++++--- .../naming-resolution/callsite.scala | 10 ------ .../naming-resolution/compiler.error | 8 ----- .../naming-resolution/package.scala | 5 --- 4 files changed, 26 insertions(+), 28 deletions(-) delete mode 100644 tests/disabled/not-representable/naming-resolution/callsite.scala delete mode 100644 tests/disabled/not-representable/naming-resolution/compiler.error delete mode 100644 tests/disabled/not-representable/naming-resolution/package.scala diff --git a/src/dotty/tools/dotc/typer/Typer.scala b/src/dotty/tools/dotc/typer/Typer.scala index 52470ba879ec..5fbb395ba06d 100644 --- a/src/dotty/tools/dotc/typer/Typer.scala +++ b/src/dotty/tools/dotc/typer/Typer.scala @@ -72,6 +72,13 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit */ private var importedFromRoot: Set[Symbol] = Set() + /** Temporary data item for single call to typed ident: + * This symbol would be found under Scala2 mode, but is not + * in dotty (because dotty conforms to spec section 2 + * wrt to package member resolution but scalac doe not). + */ + private var foundUnderScala2: Type = _ + def newLikeThis: Typer = new Typer /** Attribute an identifier consisting of a simple name or wildcard @@ -228,10 +235,14 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit else curOwner.thisType.select(name, defDenot) if (!(curOwner is Package) || isDefinedInCurrentUnit(defDenot)) return checkNewOrShadowed(found, definition) // no need to go further out, we found highest prec entry - else if (defDenot.symbol is Package) - return checkNewOrShadowed(previous orElse found, packageClause) - else if (prevPrec < packageClause) - return findRef(found, packageClause, ctx)(outer) + else { + if (ctx.scala2Mode) + foundUnderScala2 = checkNewOrShadowed(found, definition) + if (defDenot.symbol is Package) + return checkNewOrShadowed(previous orElse found, packageClause) + else if (prevPrec < packageClause) + return findRef(found, packageClause, ctx)(outer) + } } } val curImport = ctx.importInfo @@ -267,10 +278,20 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit val saved = importedFromRoot importedFromRoot = Set.empty - val rawType = + foundUnderScala2 = NoType + + var rawType = try findRef(NoType, BindingPrec.nothingBound, NoContext) finally importedFromRoot = saved + if (foundUnderScala2.exists && (foundUnderScala2 ne rawType)) { + ctx.migrationWarning( + ex"""Name resolution will change. + | currently selected : $foundUnderScala2 + | in the future, without -language:Scala2: $rawType""", tree.pos) + rawType = foundUnderScala2 + } + val ownType = if (rawType.exists) ensureAccessible(rawType, superAccess = false, tree.pos) diff --git a/tests/disabled/not-representable/naming-resolution/callsite.scala b/tests/disabled/not-representable/naming-resolution/callsite.scala deleted file mode 100644 index 036803a26930..000000000000 --- a/tests/disabled/not-representable/naming-resolution/callsite.scala +++ /dev/null @@ -1,10 +0,0 @@ -// This one should be rejected according to spec. The import takes precedence -// over the type in the same package because the typeis declared in a -// different compilation unit. scalac does not conform to spec here. -package naming.resolution - -import java.nio.file._ // Imports `Files` - -object Resolution { - def gimmeFiles: Files = Files.list(Paths.get(".")) -} diff --git a/tests/disabled/not-representable/naming-resolution/compiler.error b/tests/disabled/not-representable/naming-resolution/compiler.error deleted file mode 100644 index 81d6b3cfaf91..000000000000 --- a/tests/disabled/not-representable/naming-resolution/compiler.error +++ /dev/null @@ -1,8 +0,0 @@ -$ scalac tests/pending/naming-resolution/*.scala -$ ./bin/dotc tests/pending/naming-resolution/*.scala -tests/pending/naming-resolution/callsite.scala:6: error: type mismatch: - found : java.util.stream.Stream[java.nio.file.Path] - required: java.nio.file.Files - def gimmeFiles: Files = Files.list(Paths.get(".")) - ^ -one error found diff --git a/tests/disabled/not-representable/naming-resolution/package.scala b/tests/disabled/not-representable/naming-resolution/package.scala deleted file mode 100644 index f0e26ee95ad7..000000000000 --- a/tests/disabled/not-representable/naming-resolution/package.scala +++ /dev/null @@ -1,5 +0,0 @@ -package naming - -package object resolution { - type Files = java.util.stream.Stream[java.nio.file.Path] -} From fcf3bcd7e7358f94846cd90a83efd476ef5023b1 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 26 Aug 2016 18:34:03 +0200 Subject: [PATCH 3/4] Add test file. --- tests/pos-scala2/naming-resolution/callsite.scala | 10 ++++++++++ tests/pos-scala2/naming-resolution/package.scala | 5 +++++ 2 files changed, 15 insertions(+) create mode 100644 tests/pos-scala2/naming-resolution/callsite.scala create mode 100644 tests/pos-scala2/naming-resolution/package.scala diff --git a/tests/pos-scala2/naming-resolution/callsite.scala b/tests/pos-scala2/naming-resolution/callsite.scala new file mode 100644 index 000000000000..036803a26930 --- /dev/null +++ b/tests/pos-scala2/naming-resolution/callsite.scala @@ -0,0 +1,10 @@ +// This one should be rejected according to spec. The import takes precedence +// over the type in the same package because the typeis declared in a +// different compilation unit. scalac does not conform to spec here. +package naming.resolution + +import java.nio.file._ // Imports `Files` + +object Resolution { + def gimmeFiles: Files = Files.list(Paths.get(".")) +} diff --git a/tests/pos-scala2/naming-resolution/package.scala b/tests/pos-scala2/naming-resolution/package.scala new file mode 100644 index 000000000000..f0e26ee95ad7 --- /dev/null +++ b/tests/pos-scala2/naming-resolution/package.scala @@ -0,0 +1,5 @@ +package naming + +package object resolution { + type Files = java.util.stream.Stream[java.nio.file.Path] +} From 062b4133db13bb77369cae81a5ec89e4b2bb6699 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Fri, 16 Sep 2016 18:46:08 +0200 Subject: [PATCH 4/4] Refactoring of findRef Three goals: 1. Fix crasher in compileStdLib by saving and restoring foundUnderScala2 analogous to iportedFromRoot. 2. Make behavior the same as scalac under Scala2 mode - ListBuffer behaved differently before. 3. Make findRef faster by making it tail-recursive as long as nothing was found. --- src/dotty/tools/dotc/typer/Typer.scala | 141 +++++++++++++++---------- 1 file changed, 83 insertions(+), 58 deletions(-) diff --git a/src/dotty/tools/dotc/typer/Typer.scala b/src/dotty/tools/dotc/typer/Typer.scala index 5fbb395ba06d..976f16289e4a 100644 --- a/src/dotty/tools/dotc/typer/Typer.scala +++ b/src/dotty/tools/dotc/typer/Typer.scala @@ -77,7 +77,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit * in dotty (because dotty conforms to spec section 2 * wrt to package member resolution but scalac doe not). */ - private var foundUnderScala2: Type = _ + private var foundUnderScala2: Type = NoType def newLikeThis: Typer = new Typer @@ -140,14 +140,20 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit * imported by * or defined in */ - def bindingString(prec: Int, whereFound: Context, qualifier: String = "") = + def bindingString(prec: Int, whereFound: Context, qualifier: String = "")(implicit ctx: Context) = if (prec == wildImport || prec == namedImport) ex"imported$qualifier by ${whereFound.importInfo}" else ex"defined$qualifier in ${whereFound.owner}" /** 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 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: Int): Type = + def checkNewOrShadowed(found: Type, newPrec: Int, scala2pkg: Boolean = false)(implicit ctx: Context): Type = if (!previous.exists || ctx.typeComparer.isSameRef(previous, found)) found else if ((prevCtx.scope eq ctx.scope) && (newPrec == definition || @@ -157,7 +163,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit found } else { - if (!previous.isError && !found.isError) { + if (!scala2pkg && !previous.isError && !found.isError) { error( ex"""reference to $name is ambiguous; |it is both ${bindingString(newPrec, ctx, "")} @@ -170,7 +176,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit /** The type representing a named import with enclosing name when imported * from given `site` and `selectors`. */ - def namedImportRef(site: Type, selectors: List[untpd.Tree]): Type = { + def namedImportRef(site: Type, selectors: List[untpd.Tree])(implicit ctx: Context): Type = { def checkUnambiguous(found: Type) = { val other = namedImportRef(site, selectors.tail) if (other.exists && found.exists && (found != other)) @@ -197,7 +203,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit /** The type representing a wildcard import with enclosing name when imported * from given import info */ - def wildImportRef(imp: ImportInfo): Type = { + def wildImportRef(imp: ImportInfo)(implicit ctx: Context): Type = { if (imp.isWildcardImport) { val pre = imp.site if (!isDisabled(imp, pre) && !(imp.excluded contains name.toTermName) && name != nme.CONSTRUCTOR) { @@ -211,58 +217,71 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit /** Is (some alternative of) the given predenotation `denot` * defined in current compilation unit? */ - def isDefinedInCurrentUnit(denot: Denotation): Boolean = denot match { + def isDefinedInCurrentUnit(denot: Denotation)(implicit ctx: Context): Boolean = denot match { case MultiDenotation(d1, d2) => isDefinedInCurrentUnit(d1) || isDefinedInCurrentUnit(d2) case denot: SingleDenotation => denot.symbol.sourceFile == ctx.source.file } /** Is `denot` the denotation of a self symbol? */ - def isSelfDenot(denot: Denotation) = denot match { + def isSelfDenot(denot: Denotation)(implicit ctx: Context) = denot match { case denot: SymDenotation => denot is SelfName case _ => false } - // begin findRef - if (ctx.scope == null) previous - else { - val outer = ctx.outer - if ((ctx.scope ne outer.scope) || (ctx.owner ne outer.owner)) { - val defDenot = ctx.denotNamed(name) - if (qualifies(defDenot)) { - val curOwner = ctx.owner - val found = - if (isSelfDenot(defDenot)) curOwner.enclosingClass.thisType - else curOwner.thisType.select(name, defDenot) - if (!(curOwner is Package) || isDefinedInCurrentUnit(defDenot)) - return checkNewOrShadowed(found, definition) // no need to go further out, we found highest prec entry - else { - if (ctx.scala2Mode) - foundUnderScala2 = checkNewOrShadowed(found, definition) - if (defDenot.symbol is Package) - return checkNewOrShadowed(previous orElse found, packageClause) - else if (prevPrec < packageClause) - return findRef(found, packageClause, ctx)(outer) + /** Would import of kind `prec` be not shadowed by a nested higher-precedence definition? */ + def isPossibleImport(prec: Int)(implicit ctx: Context) = + prevPrec < prec || prevPrec == prec && (prevCtx.scope eq ctx.scope) + + @tailrec def loop(implicit ctx: Context): Type = { + if (ctx.scope == null) previous + else { + val outer = ctx.outer + var result: Type = NoType + + // find definition + if ((ctx.scope ne outer.scope) || (ctx.owner ne outer.owner)) { + val defDenot = ctx.denotNamed(name) + if (qualifies(defDenot)) { + val curOwner = ctx.owner + val found = + if (isSelfDenot(defDenot)) curOwner.enclosingClass.thisType + else curOwner.thisType.select(name, defDenot) + if (!(curOwner is Package) || isDefinedInCurrentUnit(defDenot)) + result = checkNewOrShadowed(found, definition) // no need to go further out, we found highest prec entry + else { + if (ctx.scala2Mode && !foundUnderScala2.exists) + foundUnderScala2 = checkNewOrShadowed(found, definition, scala2pkg = true) + if (defDenot.symbol is Package) + result = checkNewOrShadowed(previous orElse found, packageClause) + else if (prevPrec < packageClause) + result = findRef(found, packageClause, ctx)(outer) + } } } - } - val curImport = ctx.importInfo - if (ctx.owner.is(Package) && curImport != null && curImport.isRootImport && previous.exists) - return previous // no more conflicts possible in this case - // would import of kind `prec` be not shadowed by a nested higher-precedence definition? - def isPossibleImport(prec: Int) = - prevPrec < prec || prevPrec == prec && (prevCtx.scope eq ctx.scope) - if (isPossibleImport(namedImport) && (curImport ne outer.importInfo) && !curImport.sym.isCompleting) { - val namedImp = namedImportRef(curImport.site, curImport.selectors) - if (namedImp.exists) - return findRef(checkNewOrShadowed(namedImp, namedImport), namedImport, ctx)(outer) - if (isPossibleImport(wildImport)) { - val wildImp = wildImportRef(curImport) - if (wildImp.exists) - return findRef(checkNewOrShadowed(wildImp, wildImport), wildImport, ctx)(outer) + + if (result.exists) result + else { // find import + val curImport = ctx.importInfo + if (ctx.owner.is(Package) && curImport != null && curImport.isRootImport && previous.exists) + previous // no more conflicts possible in this case + else if (isPossibleImport(namedImport) && (curImport ne outer.importInfo) && !curImport.sym.isCompleting) { + val namedImp = namedImportRef(curImport.site, curImport.selectors) + if (namedImp.exists) + findRef(checkNewOrShadowed(namedImp, namedImport), namedImport, ctx)(outer) + else if (isPossibleImport(wildImport)) { + val wildImp = wildImportRef(curImport) + if (wildImp.exists) + findRef(checkNewOrShadowed(wildImp, wildImport), wildImport, ctx)(outer) + else loop(outer) + } + else loop(outer) + } + else loop(outer) } } - findRef(previous, prevPrec, prevCtx)(outer) } + + loop } // begin typedIdent @@ -275,21 +294,27 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit return typed(desugar.patternVar(tree), pt) } - val saved = importedFromRoot - importedFromRoot = Set.empty - - foundUnderScala2 = NoType - - var rawType = - try findRef(NoType, BindingPrec.nothingBound, NoContext) - finally importedFromRoot = saved - if (foundUnderScala2.exists && (foundUnderScala2 ne rawType)) { - ctx.migrationWarning( - ex"""Name resolution will change. - | currently selected : $foundUnderScala2 - | in the future, without -language:Scala2: $rawType""", tree.pos) - rawType = foundUnderScala2 + val rawType = { + val saved1 = importedFromRoot + val saved2 = foundUnderScala2 + importedFromRoot = Set.empty + foundUnderScala2 = NoType + try { + var found = findRef(NoType, BindingPrec.nothingBound, NoContext) + if (foundUnderScala2.exists && !(foundUnderScala2 =:= found)) { + ctx.migrationWarning( + ex"""Name resolution will change. + | currently selected : $foundUnderScala2 + | in the future, without -language:Scala2: $found""", tree.pos) + found = foundUnderScala2 + } + found + } + finally { + importedFromRoot = saved1 + foundUnderScala2 = saved2 + } } val ownType =