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.

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

SI-2458 Import shadows def in other unit #5339

Closed
wants to merge 5 commits into
base: 2.12.x
from

Conversation

Projects
None yet
7 participants
@som-snytt
Copy link
Contributor

som-snytt commented Aug 14, 2016

Fixes the implementation of point 4. from SLS chapter 2:

Definitions made available by a package clause not in the
compilation unit where the reference occurs have lowest precedence.

We had code to deal with this:

if (defSym.exists && impSym.exists) {
if (isPackageOwnedInDifferentUnit(defSym))
defSym = NoSymbol

But, in the pos/t2458, no impSym was around, due to filtering
in depthOk. The depth of the imported symbol was the same as the
depth of the defined symbol, which didn't pass:

imp.depth > symbolDepth

depthOk already had special handling for Java sources: if the
import is non-wildcard and depths are equal, retain the import.

This commit does the same thing for Scala sources, using the
isPackageOwnedInDifferentUnit as the criterion. It also makes
the check for the "imported ... is permanently hidden" warning
to avoid the spurious warning.

Fix by retronym
Excavation and preparation of bones for mounting by community

Fixes scala/bug#2458
Fixes scala/bug#9552

@scala-jenkins scala-jenkins added this to the 2.12.1 milestone Aug 14, 2016

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 18, 2016

This doesn't fix SI-9552. The import should be higher precedence than definition in other file. Maybe the package object fools a depth check?

package object p {
  type Foo = Int 
} 

and separate unit

package p {
  object Dingo { type Foo = String }
  import Dingo._
  trait Bippy  {
    // import Dingo._  // this version compiles
    def z: Foo
    z: String
  }
}
@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 18, 2016

Fixes the Java check for explicit import. Scala lets wildcard import hide foreign definition.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 18, 2016

Suffers from the broken build.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 19, 2016

Some documentation notes the bug and should be updated:

http://docs.scala-lang.org/tutorials/FAQ/finding-implicits.html

@som-snytt som-snytt closed this Nov 7, 2016

@Blaisorblade

This comment has been minimized.

Copy link
Contributor

Blaisorblade commented Nov 29, 2016

@som-snytt what happened to this PR? Are the bugtracker entries up-to-date currently? (https://issues.scala-lang.org/browse/SI-2458 is closed, https://issues.scala-lang.org/browse/SI-9552 is open, and I'm confused).

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Nov 29, 2016

@Blaisorblade The old issue was closed prematurely when the spec was clarified; this PR was meant to be implementation which also nails down the newer issue (which also objects to the behavior as specified). I don't remember the build status, so I'll have to revisit it.

@som-snytt som-snytt reopened this Nov 29, 2016

@Blaisorblade

This comment has been minimized.

Copy link
Contributor

Blaisorblade commented Nov 29, 2016

@som-snytt Thanks — it sounds like all the open parts are tracked by the newer issue. I dread unfixed closed issues and forgotten bugs.

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch from 83fb60c to cdad662 Nov 29, 2016

@som-snytt som-snytt closed this Nov 30, 2016

@SethTisue SethTisue removed this from the 2.12.1 milestone Dec 1, 2016

@som-snytt som-snytt reopened this Dec 9, 2017

@scala-jenkins scala-jenkins added this to the 2.12.5 milestone Dec 9, 2017

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch 2 times, most recently from 6c49757 to db259cd Dec 9, 2017

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Dec 12, 2017

Previous failure was due to cycle in ProcessBuilder.scala with wildcard import from companion.

There was also an import in scaladoc that was not to spec.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Dec 13, 2017

This idiom now inoculates against accidental local dir named util:

package p

import scala._      // $ mkdir p/util
import util.Try     // was: error: object Try is not a member of package p.util

class C {
  def f = Try(42)
}

Too bad that's not more useful.

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch from 5553582 to 81f1366 Jan 31, 2018

build.sbt Outdated
@@ -102,6 +102,9 @@ lazy val commonSettings = clearSourceAndResourceDirectories ++ publishSettings +
autoScalaLibrary := false,
// Avoid circular dependencies for scalaInstance (see https://github.com/sbt/sbt/issues/1872)
managedScalaInstance := false,
//maxErrors := 500,
maxErrors := 5,

This comment has been minimized.

@som-snytt

som-snytt Jan 31, 2018

Contributor

Meant to set that to zero.

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch 2 times, most recently from 411dd5f to 930594a Jan 31, 2018

@@ -69,7 +70,7 @@ abstract class Reifier extends States
val rtree = pipeline(tree)

val tpe = typer.packedType(tree, NoSymbol)
val ReifiedType(_, _, tpeSymtab, _, rtpe, tpeReificationIsConcrete) = `package`.reifyType(global)(typer, universe, mirror, tpe, concrete = false)
val ReifiedType(_, _, tpeSymtab, _, rtpe, tpeReificationIsConcrete) = reifyTypeGlobal(global)(typer, universe, mirror, tpe, concrete = false)

This comment has been minimized.

@som-snytt

som-snytt Jan 31, 2018

Contributor

This was good, when you got packages and methods named reify.

/**
* This class implements a Reporter that stores erroneous files in a script for editing.
*/
class EditReporter(settings: Settings) extends Reporter {

This comment has been minimized.

@som-snytt

som-snytt Jan 31, 2018

Contributor

If @lrytz was up for a reporters module, I'd contribute.

// For that case, check if the root import dealiases to the definition.
private def resolveAmbiguity(name: Name, defSym: Symbol, impSym: Symbol, imp: ImportInfo): Boolean = {
defSym == impSym || imp.isRootImport && {
impSym.isAliasType && impSym.info.typeSymbol.fullName == defSym.fullName

This comment has been minimized.

@som-snytt

som-snytt Jan 31, 2018

Contributor

@retronym Help. I didn't have time to improve this yet.

This comment has been minimized.

@retronym

retronym Feb 1, 2018

Member

I'm having trouble paging in the context for this from my mental swap space... could you elaborate a little?

This comment has been minimized.

@som-snytt

som-snytt Feb 1, 2018

Contributor

If root import scala._ alias Seq collides with Seq defined in enclosing package, recognize that they are the same. There is related code for when two imports collide.

@SethTisue

This comment was marked as outdated.

Copy link
Member

SethTisue commented Feb 22, 2018

rebase?

@lrytz lrytz modified the milestones: 2.12.5, 2.12.6 Mar 12, 2018

@SethTisue SethTisue modified the milestones: 2.12.6, 2.12.7 Mar 23, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Apr 29, 2018

My plan is to re-do this on 2.13 and then add -Yimport to make preamble imports tractable. Not sure exactly how it will work, but similarly to the leading import Predef._ exclusion, a source file can opt out of the preamble; however, it seems to me that the syntax should align with ordinary binding.

If I compile with -Yimport:my.Predef._ then import my.Predef.{_ => _} turns it off with no special magic.

If I compile with -Yimport:my.Predef.any2stringadd, then import my.Predef.{_ => _} does not hide? Unless the rule is construed that "level 4" includes the preamble imports. I don't think anyone considered that option, i.e., the preamble is not just stuck at the front of your source; it has level 4 "binding force" like bindings from a package clause that is not in the current unit.

som-snytt added some commits Aug 14, 2016

SI-2458 Import shadows def in other unit
Fixes the implementation of point 4. from SLS chapter 2:

> Definitions made available by a package clause not in the
> compilation unit where the reference occurs have lowest precedence.

We had code to deal with this:

if (defSym.exists && impSym.exists) {
if (isPackageOwnedInDifferentUnit(defSym))
defSym = NoSymbol

But, in the `pos/t2458`, no `impSym` was around, due to filtering
in `depthOk`. The depth of the imported symbol was the same as the
depth of the defined symbol, which didn't pass:

imp.depth > symbolDepth

`depthOk` already had special handling for Java sources: if the
import is non-wildcard and depths are equal, retain the import.

This commit does the same thing for Scala sources, using the
`isPackageOwnedInDifferentUnit` as the criterion. It also makes
the check for the "imported ... is permanently hidden" warning
to avoid the spurious warning.

Fix by retronym
Excavation and preparation of bones for mounting by community
SI-9552 Wildcard import precedence vs foreign definition
The Java test for explicit import tripped up the Scala
test for foreign definition (i.e., in other compilation
unit). This commit refactors the boolean expression.

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch from 930594a to 1aba572 Apr 30, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Apr 30, 2018

Rebased with the fix for root imports aka preamble imports. No longer impacts the world like an asteroid.

som-snytt added some commits Apr 30, 2018

Preamble imports are lowest priority binding
This change allows the package-owned symbols clause
to respect nesting depth without breaking all
code. This corresponds to the intuition that
the implicit imports of the root context, which
are not written in source code, do not conflict
with user-written imports and definitions.
Fix imports in two tests
This has the form
```
import p._ ; package p { S }
```
where S is both made available in p and also explicitly
imported in enclosing scope.

The code for reconciling ambiguous imports is not
triggered here.

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch from 1aba572 to c957bcd Apr 30, 2018

@som-snytt

This comment was marked as outdated.

Copy link
Contributor

som-snytt commented Apr 30, 2018

/rebuild

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Apr 30, 2018

Gitter is down? so I'll ask here whether this existing behavior seems too loose:

scala> class C { type InputStream = java.io.InputStream }
defined class C

scala> def f(c: C) = { import java.io._, c._ ; null.asInstanceOf[InputStream] }
f: (c: C)c.InputStream

scala> def f(c: C) = { import c._, java.io._ ; null.asInstanceOf[InputStream] }
f: (c: C)java.io.InputStream

I was considering whether the ambiguity in a test class

import p._ ; package p { S } ; package q { S }

deserves a pass. (Update: Yes, it does, even though the fix (moving import inside packages that need it) is easy. If the import goes unused, the user can ask for a warning.)

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch from fef604b to c4c5c88 May 2, 2018

Yimports for preamble imports
Packages and objects can be specified.

A source file can exclude imports from
one or more objects with the usual mechanism,
formerly known as the `Predef` exclusion clause.

Handle overloaded foreign definitions:
If one alternative is foreign definition,
take the overload as foreign definition.

@som-snytt som-snytt force-pushed the som-snytt:issue/2458-precedence branch from c4c5c88 to c25bc57 May 2, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented May 9, 2018

I needed this today.

imported `Configurator' is permanently hidden by definition of object Configurator in package apps

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented May 13, 2018

I need this basically all the time.

[warn] /Users/andrew/workspace/scala/test/benchmarks/src/main/scala/scala/tools/reporters/ReportingBenchmark.scala:9: imported `Reporter' is permanently hidden by definition of class Reporter in package reporters

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented May 30, 2018

Moving this to the next 2.13 milestone.

@adriaanm adriaanm modified the milestones: 2.12.7, 2.13.0-M5 May 30, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented May 30, 2018

@adriaanm There's a forward port PR for 2.13; I have a -Xsource:2.13 commit to push here; would you consider that?

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented May 30, 2018

Sure, as a backport once it has landed in 2.13.x. I'm concerned with how thin our attention is spread across the PR queue these days, so I'd like to compact the queue. (It's nice to be so outnumbered, but also regret the delays this is causing.)

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented May 30, 2018

Superseded for 2.13 by #6589

@som-snytt som-snytt closed this May 30, 2018

@SethTisue SethTisue removed this from the 2.13.0-M5 milestone Aug 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment