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

Add flags -Ysysdef and -Ypredef to configure root imports [ci: last-only] #5350

Closed
wants to merge 1 commit into
base: 2.12.x
from

Conversation

Projects
None yet
@andyscott
Copy link

andyscott commented Aug 18, 2016

This adds a -Ysysdef and -Ypredef flag to give you full control over global imports for Scala compilation units.

Background information:

By default, scalac imports java.lang._, scala._, and scala.Predef._ into the namespace of Scala compilation units.

As it stands, we have two existing flags that adjust the behavior of predefined imports.

  • -Yno-imports disables all of these.
  • -Yno-predef disables scala.Predef._.

There are a few other nuances to the behavior of imports. The most important behavior is that scala.Predef._ is unimported/evicted if there are any direct leading imports for anything within scala.Predef. For example, an import to scala.Predef.DummyImplicit will cause everything else normally imported by predef to be unimported.

Changes

  • Introduces the notion of "sysdefs" and "predefs" for Scala compilation units. Sysdefs are global imports that are imported at the top of all Scala compilation units under all circumstances. Predefs are imported after sysdefs and can be masked by top level imports in the source file (to match the existing behavior of directly importing from scala.Predef).
  • Adds -Ysysdef which controls the global sysdef imports for all Scala compilation units. The default value is set to java.lang._; scala._.
  • Added -Ypredef which controls the predef imports for Scala compilation units. The default value is set to scala.Predef._.

I'd be great to get any feedback. I would also to back port this to 2.11.

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 18, 2016

It looks like it would be reasonable to implement support for importing individual symbols. For example, -Ypredef:java.lang._,scala.{ any2stringAdd => _, _},scala.Predef._. Should I push this a little further?

@tpolecat

This comment has been minimized.

Copy link
Contributor

tpolecat commented Aug 18, 2016

I would love to see this feature, with fine-grained support as described above.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 19, 2016

That was my thinking as well. There might be considerations about symbol init which makes it not work as simply as an import? The idiom would be -Ypredef:java.lang._,scala._,scala.Predef.{any2stringAdd => _, _}.

Does the refactor of that part work the same? I remember the import has to be at the top of the unit, because otherwise the semantics looks like unimporting. The REPL supports it at the top. The other part is don't import our own package.

There are postSetHooks for settings which you can use to wire -Ypredef to -Yno-imports and -Yno-predef. I guess -Yno-imports resets to empty, -Yno-predef strips Predef. (As you have it in the code.) noimports andThen (flag => if (flag) Ypredef.value = Nil). noassertions uses that.

@@ -33,7 +33,7 @@ trait AbsScalaSettings {
def ChoiceSetting(name: String, helpArg: String, descr: String, choices: List[String], default: String): ChoiceSetting
def ChoiceSettingForcedDefault(name: String, helpArg: String, descr: String, choices: List[String], default: String): ChoiceSetting
def IntSetting(name: String, descr: String, default: Int, range: Option[(Int, Int)], parser: String => Option[Int]): IntSetting
def MultiStringSetting(name: String, helpArg: String, descr: String): MultiStringSetting
def MultiStringSetting(name: String, helpArg: String, descr: String, default: List[String] = Nil): MultiStringSetting

This comment has been minimized.

@som-snytt

som-snytt Aug 19, 2016

Contributor

This doesn't look right to me. Compare MultiChoiceSetting where default arg is on MutableSettings.

val nopredef = BooleanSetting ("-Yno-predef", "Compile without importing Predef.")
val Ypredef = MultiStringSetting("-Ypredef", "package", "Supply a list of packages/objects to be imported into the global scope", List("java.lang", "scala", "scala.Predef"))
val noimports = BooleanSetting ("-Yno-imports", "Deprecated. Compile without importing scala.*, java.lang.*, or Predef.")
val nopredef = BooleanSetting ("-Yno-predef", "Deprecated. Compile without importing Predef.")

This comment has been minimized.

@som-snytt

som-snytt Aug 19, 2016

Contributor

Use withDeprecationMessage.

else settings.Ypredef.value

paths.map(path => rootMirror.getPackage(path))
}

This comment has been minimized.

@som-snytt

som-snytt Aug 19, 2016

Contributor

I think it's easier just to use Ypredef here. If you want to enforce exclusive use, you can move that into withPostSetHook.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Aug 19, 2016

I'm very much in favour of this. However if deprecation warnings are added for existing compiler flags I'd like to see some mechanism to disable them. Maybe flag deprecation warnings could also be covered by -deprecation?

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Aug 19, 2016

@milessabin

I'm very much in favour of this. However if deprecation warnings are added for existing compiler flags I'd like to see some mechanism to disable them. Maybe flag deprecation warnings could also be covered by -deprecation?

If that's for cross-building sbt reasons, you can make compiler flags conditional to the scala version. I had to do that with, for example, 2.12 renaming -Yopt to -opt.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Aug 19, 2016

@dwijnand no, not really. If a warning really shouldn't be ignored it should be an error. If it is permissible to ignore it, at least temporarily, then it should be possible to note the warning, disable it and return a build uncluttered with noise. People don't ignore warnings for fun ... typically there's some constraint that prevents them from fixing them immediately ... that being so we should avoid imposing additional burdens on them.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 19, 2016

TIL

$ scalam -Ymacro-no-expand
Welcome to Scala 2.12.0-M5 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_92).
Type in expressions for evaluation. Or try :help.

scala> [init] warning: there was one deprecation warning; re-run with -deprecation for details
warning: there was one deprecation warning; re-run with -deprecation for details
warning: there was one deprecation warning; re-run with -deprecation for details


scala> 42
warning: there was one deprecation warning; re-run with -deprecation for details
res0: Int = 42

scala> :replay -Ymacro-no-expand -deprecation
Replaying: 42
[init] warning: -Ymacro-no-expand is deprecated: Use -Ymacro-expand:none
warning: -Ymacro-no-expand is deprecated: Use -Ymacro-expand:none
res0: Int = 42


scala> :replay -Ymacro-no-expand -deprecation -nowarn
Replaying: 42
res0: Int = 42

I think there's a proposal for warnings to be typed, so it's easier to filter them (in a reporter). Then maybe they'll do warning suppression of a sort.

if (peer.isSetByUser)
errorFn(s"Flag ${s.name} can not be used in conjunction with flag ${peer.name}")
}

This comment has been minimized.

@som-snytt

som-snytt Aug 19, 2016

Contributor

This is a bit odd. Is there a word for currying in this way? I think this is the same:

def exclusively(peer: Setting)(setting: Setting): Unit = if (peer.isSet) fail(setting)
if (settings.noimports) error("Deprecated flag -Yno-imports isn't supported with -Ypredef")
settings.Ypredef.value
}
if (settings.Ypredef.isSetByUser) settings.Ypredef.value
else if (settings.noimports) Nil
else if (settings.nopredef) settings.Ypredef.value.filterNot(_ == "scala.Predef")
else settings.Ypredef.value

This comment has been minimized.

@som-snytt

som-snytt Aug 19, 2016

Contributor

You may not want to go this way, but I was thinking this could be val paths = settings.Ypredef.value if setting noimports or nopredef updated Ypredef in the hook. Then the real code doesn't care how Ypredef was composed.

This comment has been minimized.

@andyscott
/** Cleanup import `imp` such that any selectors covered by
* any imports in `parents` are removed.
*/
def dedupe(imp: Import, parents: List[Import]): Option[Import] = {

This comment has been minimized.

@andyscott

andyscott Aug 20, 2016

I am unsure of what the specific behavior should be for skipping predef imports. Also, what's the appropriate way to test this logic?

.collect { case imp: Import =>
// I can't seem to figure out how to use the freshly parsed tree...
// Instead we get to recreate it
gen.mkImportFromSelector(

This comment has been minimized.

@andyscott

andyscott Aug 20, 2016

See the inline code comments. The parsed tree seems to be lacking symbols on the import's expr tree. I couldn't figure out how to assign them without throwing errors.

@andyscott andyscott force-pushed the andyscott:as-Ypredef branch from 6cde54b to ecc7de2 Aug 20, 2016

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 20, 2016

I made an attempt at handling import selectors-- some help needed for this one, so I've added a few comments to my own code.

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 20, 2016

/rebuild

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 21, 2016

Getting the neg/predef-masking test to pass is going to be "fun" (read: challenging), regardless of support for import selectors.

For this test to pass, hard coded logic for Predef needs to be generalized to handle all predefs.

  // Belongs in TreeInfo but then I can't reach it from Printers.
  def isReferenceToScalaMember(t: Tree, Id: Name) = t match {
    case Ident(Id)                                          => true
    case Select(Ident(nme.scala_), Id)                      => true
    case Select(Select(Ident(nme.ROOTPKG), nme.scala_), Id) => true
    case _                                                  => false
  }

To make this easier, I may put a constraint that all predef imports would automatically be attached to _root_. This means -Ypredef:scala._;collection._;immutable._; wouldn't behave but -Ypredef:scala._;scala.collection._;scala.collection.immutable._ would be fine.

@soc

This comment has been minimized.

Copy link
Member

soc commented Aug 21, 2016

@andyscott While you are on it, can you add something to treat named imports as wildcard imports?

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 21, 2016

@soc I'm not familiar with what you mean by start trees. Can you explain?

@soc

This comment has been minimized.

Copy link
Member

soc commented Aug 21, 2016

@andyscott Sorry, I was still editing => wildcard imports.

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 22, 2016

One issue I am still having: Many of the tests, such as test/files/run/dynamic-updateDynamic.scala are failing with a diff error due to the following warning output:

-newSettings: allArgs = List(-d, dynamic-updateDynamic-run.obj, -usejavacp, -Xprint-pos, -Xprint:typer, -Yrangepos, -Ystop-after:typer, -d, dynamic-updateDynamic-run.obj)

I noticed a few existing tests extending scala.tools.partest.DirectTest override the debug flag to suppress this warning. Do I need to do this for all of the failing tests or is there a better solution for this?

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 22, 2016

I guess you're talking about your local test output, not the jenkins failure

scala.reflect.internal.MissingRequirementError: object <0:9>java.lang in compiler mirror not found.

That log output is when partest.debug, which is not on for builds, right?

I haven't followed the last changes, so I don't understand the semicolon separator change.

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 22, 2016

I made the delimiter configurable for multi string settings, since commas might be present in the import selector syntax. -Ypredef:scala._;scala.Predef.{any2stringadd,println}.

I believe I fixed that particular error in one of the last commits. It had to do with calling toString on an Import's expr. It looks like it was attaching some range information in certain cases.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 22, 2016

Ah. I wonder if it isn't better just to make the option parser aware of brackets.

Needless to add, if we're imitating import syntax, import concurrent._, ExecCtx.Implicits.global and so on. It's commas all the way down.

Or, it's not a multistring at all, but a string. When you receive it, prepend "import" and parse it and typecheck it.

@andyscott

This comment has been minimized.

Copy link

andyscott commented Aug 22, 2016

I'm okay with any of those approaches. The custom delimiter seems to work fine, but if you'd prefer that I go a different route let me know and I can switch it over.

@adriaanm adriaanm changed the title Add flag -Ypredef to configure predef objects Add flag -Ypredef to configure predef objects [ci: last-only] Aug 23, 2016

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 27, 2016

Just noticed -language:experimental._ doesn't actually work. I remember the options are hardcoded (because the package isn't loaded yet) but it would be nice if all these options could be handled uniformly. Then -language:foo would be -Ypredef:language.foo and so on.

@SethTisue SethTisue modified the milestones: 2.12.5, 2.12.6 Feb 22, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Feb 24, 2018

closing for inactivity. can always be reopened

@SethTisue SethTisue closed this Feb 24, 2018

@andyscott

This comment has been minimized.

Copy link

andyscott commented Feb 24, 2018

sad kitten

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Feb 24, 2018

@andyscott if you'd still like to work on this — and/or if someone else wants to take it up — we'd be happy to reopen

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Feb 24, 2018

(P.S. if having some help might help this move towards the finish line, you might post about it on https://contributors.scala-lang.org and see if you can generate some interest/assistance that way)

@fommil

This comment has been minimized.

Copy link
Contributor

fommil commented Feb 25, 2018

to summarise my feedback from above:

  1. I personally don't care about Predef. I think everything can be achieved with custom imports.
  2. The system imports are triggering unused import warnings, they should be special cased and the warning not emitted.

other than that, I can live with all the other glitches because I'd really like system imports to behave as if I wrote them explicitly at the top of the file.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Feb 25, 2018

I'm interested in helping, because I think fixing name binding in another PR will make more folks (common folk) consider controlling the implicit imports in projects, even if they are a handy default for beginners and casual REPLing. I'll also look at whether they warn. How can they warn if they are not in code?

@andyscott

This comment has been minimized.

Copy link

andyscott commented Feb 25, 2018

If we can come up with a reasonable path forward I'd be very happy to allocate some time to wrap this up (or something similar in functionality).

Is this PR the best place to continue the discussion?

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Feb 26, 2018

Yes please! I spinned of a scala-dev ticket to continue the discussion: scala/scala-dev#474.

@SethTisue SethTisue modified the milestones: 2.12.6, WIP Mar 6, 2018

@SethTisue SethTisue reopened this Mar 6, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented May 2, 2018

I left a note on scala-dev that I've proposed a simplified version of this feature, as part of a PR that has been hanging around about as long as this one.

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Jun 1, 2018

Heads up: if this is going to make it into 2.13, it will need to be ready to merge by M5 (due Aug 10).

@adriaanm adriaanm modified the milestones: WIP, 2.13.0-M5 Jun 1, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Jun 1, 2018

My simpler -Yimports is at 1bae6f4

part of https://github.com/scala/scala/pull/6589/commits

private[this] def collectLeadingImports(body: Tree): List[Import] = {
@tailrec def loop(rem: List[Tree], acc: List[Import]): List[Import] = rem match {
case tree :: tail => tree match {
case PackageDef(_, stats) => loop(stats ::: tail, acc)

This comment has been minimized.

@som-snytt

som-snytt Jun 8, 2018

Contributor

This drills down through package statements, different from current check.

This comment has been minimized.

@som-snytt

som-snytt Jun 8, 2018

Contributor

Sorry, I retract that! It does in fact enter nested packages looking for the exclusionary import.

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Jun 9, 2018

Looking this over again for ideas and LOC for my -Yimports.

Thinking about this during unbearably long Bay Area commutes, I wanted to minimize scans of unit trees, and also minimize context lookups, in part to avoid forcing infos. I was also thinking that you only need to look at previous imports (including renames).

But the approach taken here doesn't work for this case where a definition aliases a name in the full name of the predef object hello.world.minidef (from a test):

import hello.world._

object minidef {
  val somethingElse = 17
}

// does this look like predef import?
import minidef._

In a normal context lookup, the definition shadows the import.

The problem is partly that the existing implementation doesn't just look at "leading" imports. But switching the order of the object and the import from it shouldn't change semantics.

The revised words at this puzzler are still not right, but the gist is that the import can be at the bottom of the file, like vi modelines. Possibly we should just disallow that, for the sake of efficiency.

Here is the working example, where INT and Magic come from -Ypredef hello.world.minidef._. Uncommenting the import breaks it as shown. selectively import was to show that the nested import doesn't disqualify the predef.

//import hello.world._

// does this look like predef import?
import minidef._

object minidef {
  val somethingElse = 17
}

object Test {
  // selectively import
  //import hello.world.minidef.INT
  val a: INT = Magic
  println("this should be OK")
  def f = somethingElse
}

Edit: there are couple of heuristics or limitations that we could apply, such as taking only leading imports, up to a def, and only at top-level or top-level package; and either requiring fully-qualified import (with the exception of scala.Predef) or warning for the case shown here, which is easy? to check for.

By fully-qualified, I wasn't even thinking "absolute", _root_-rooted.

Proposed warning, torn from the headlines of The Onion:

import minidef._ permanently shadows import of -Ypredef

Or is there a fifth level of name binding that ignores definitions in imports?

@SethTisue SethTisue removed the help-wanted label Jul 10, 2018

@SethTisue SethTisue modified the milestones: 2.13.0-M5, 2.13.0-RC1 Aug 7, 2018

@adriaanm adriaanm modified the milestones: 2.13.0-RC1, 2.13.x Aug 7, 2018

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Aug 10, 2018

This work was brought forward in a slightly different form in #6764 which supersedes this PR.

The merged PR is one flag, and distinguishes "sysdef" as packages and "predef" as objects that are not packages. You get wildcard imports from either sort, and the order is respected, where later shadows earlier, as though nested. The way predefs are excluded is similar but arguably more workable.

"Fine-grained" control is not supported. Root contexts work more like nested packages than like imports.

I commented that a true -Ypreamble could be added separately, where a preamble is just code prepended to source text. That preamble could be an arbitrary import, for example.

@som-snytt som-snytt closed this Aug 10, 2018

@SethTisue SethTisue removed this from the 2.13.1 milestone Sep 20, 2018

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