SI-2458 Import shadows def in other unit #5339

som-snytt · 2016-08-14T07:59:10Z

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

som-snytt · 2016-08-18T00:07:15Z

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 · 2016-08-18T21:00:33Z

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

som-snytt · 2016-08-18T22:20:46Z

Suffers from the broken build.

som-snytt · 2016-08-19T17:10:17Z

Some documentation notes the bug and should be updated:

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

Blaisorblade · 2016-11-29T20:48:12Z

@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 · 2016-11-29T21:07:20Z

@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.

Blaisorblade · 2016-11-29T21:09:47Z

@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 · 2017-12-12T06:56:58Z

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 · 2017-12-13T16:21:22Z

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 · 2018-01-31T02:22:36Z

build.sbt

  // Avoid circular dependencies for scalaInstance (see https://github.com/sbt/sbt/issues/1872)
  managedScalaInstance := false,
+  //maxErrors := 500,
+  maxErrors := 5,


Meant to set that to zero.

som-snytt · 2018-01-31T14:09:08Z

src/compiler/scala/reflect/reify/Reifier.scala


          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 was good, when you got packages and methods named reify.

som-snytt · 2018-01-31T14:10:18Z

src/compiler/scala/tools/nsc/reporters/EditReporter.scala

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


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

som-snytt · 2018-01-31T14:11:46Z

src/compiler/scala/tools/nsc/typechecker/Contexts.scala

+    // 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


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

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

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.

som-snytt · 2018-04-29T21:17:06Z

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.

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

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 · 2018-04-30T18:30:16Z

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

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.

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.

som-snytt · 2018-04-30T23:29:40Z

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.)

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 · 2018-05-09T22:07:54Z

I needed this today.

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

som-snytt · 2018-05-13T00:03:08Z

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 · 2018-05-30T07:53:45Z

Moving this to the next 2.13 milestone.

som-snytt · 2018-05-30T14:08:21Z

@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 · 2018-05-30T14:23:08Z

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 · 2018-05-30T15:49:39Z

Superseded for 2.13 by #6589

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

som-snytt mentioned this pull request Aug 14, 2016

SI-2458 Clarify spec for package syms #5151

Merged

som-snytt closed this Nov 7, 2016

som-snytt reopened this Nov 29, 2016

som-snytt force-pushed the issue/2458-precedence branch from 83fb60c to cdad662 Compare November 29, 2016 21:23

adriaanm assigned retronym Nov 30, 2016

som-snytt closed this Nov 30, 2016

SethTisue removed this from the 2.12.1 milestone Dec 1, 2016

This was referenced Apr 7, 2017

Clarify spec for package syms scala/bug#2458

Closed

identical code compiles in separate source files, fails in single source file scala/bug#9552

Closed

som-snytt reopened this Dec 9, 2017

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

som-snytt force-pushed the issue/2458-precedence branch 2 times, most recently from 6c49757 to db259cd Compare December 12, 2017 06:51

som-snytt force-pushed the issue/2458-precedence branch from 5553582 to 81f1366 Compare January 31, 2018 02:19

som-snytt commented Jan 31, 2018

View reviewed changes

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

som-snytt commented Jan 31, 2018

View reviewed changes

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

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

som-snytt added 2 commits April 29, 2018 15:00

SI-9552 Wildcard import precedence vs foreign definition

ec4693c

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 force-pushed the issue/2458-precedence branch from 930594a to 1aba572 Compare April 30, 2018 18:27

som-snytt added 2 commits April 30, 2018 13:48

Fix imports in two tests

b0a7c6c

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 force-pushed the issue/2458-precedence branch from 1aba572 to c957bcd Compare April 30, 2018 20:48

This comment has been minimized.

Sign in to view

som-snytt mentioned this pull request May 2, 2018

Allow customizing root imports scala/scala-dev#474

Closed

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

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

som-snytt mentioned this pull request May 2, 2018

Explicit imports now shadow locally defined identifiers #6589

Merged

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

som-snytt closed this May 30, 2018

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

som-snytt deleted the issue/2458-precedence branch December 20, 2020 00:55

SI-2458 Import shadows def in other unit #5339

SI-2458 Import shadows def in other unit #5339

Uh oh!

Conversation

som-snytt commented Aug 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Aug 18, 2016

Uh oh!

som-snytt commented Aug 18, 2016

Uh oh!

som-snytt commented Aug 18, 2016

Uh oh!

som-snytt commented Aug 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Blaisorblade commented Nov 29, 2016

Uh oh!

som-snytt commented Nov 29, 2016

Uh oh!

Blaisorblade commented Nov 29, 2016

Uh oh!

som-snytt commented Dec 12, 2017

Uh oh!

som-snytt commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

som-snytt Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

som-snytt Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

som-snytt Jan 31, 2018

Choose a reason for hiding this comment

Uh oh!

retronym Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

som-snytt Feb 1, 2018

Choose a reason for hiding this comment

Uh oh!

som-snytt commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented Apr 30, 2018

Uh oh!

This comment has been minimized.

som-snytt commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

som-snytt commented May 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriaanm commented May 30, 2018

Uh oh!

som-snytt commented May 30, 2018

Uh oh!

adriaanm commented May 30, 2018

Uh oh!

som-snytt commented May 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

som-snytt commented Aug 14, 2016 •

edited

Loading

som-snytt commented Aug 19, 2016 •

edited

Loading

som-snytt commented Dec 13, 2017 •

edited

Loading

som-snytt commented Apr 29, 2018 •

edited

Loading

som-snytt commented Apr 30, 2018 •

edited

Loading

som-snytt commented May 9, 2018 •

edited

Loading

som-snytt commented May 13, 2018 •

edited

Loading