Allow toplevel definitions #5754

odersky · 2019-01-20T18:23:49Z

With this PR, we now allow arbitrary definitions at the toplevel. This means package objects are now redundant, and will be phased out.

Glavo · 2019-01-22T06:00:06Z

Current implicit classes must be defined inside of another, but it can be defined in the package object, will lift this restriction in this pull requests?

odersky · 2019-01-23T20:27:40Z

Current implicit classes must be defined inside of another, but it can be defined in the package object, will lift this restriction in this pull requests?

Yes. Implicit classes and implicit objects can now be defined at the toplevel.

therealcisse · 2019-01-24T09:20:04Z

With this, will we be getting zero parameter implicit classes instead of having to write a class and a implicit function?

odersky · 2019-01-24T09:58:12Z

With this, will we be getting zero parameter implicit classes instead of having to write a class and a implicit function?

The plan is to drop implicit classes altogether. See #5458.

sjrd

I haven't looked too much at the implementation, as I don't think I'm qualified. For tests, overall I'm pleased. Here are a few more suggestions for things that could wrong:

Top-level val X/def X in one file, and a class X in another file
Same with an object X in another file
Top-level type X in one file, and a class X, object X or val x in another file.

I'd like to know what would happen if we have a top-level def main(Array[String]): Unit. Can that work as a JVM main method? If yes, what's the incantation to invoke it? If not, at least we should emit a warning at the definition, because users are bound to try that. In either case there should be an appropriate test.

The documentation should clearly highlight that the enclosing file name becomes significant at the binary level. Changing a file's name is now a binary incompatible change if it contains any top-level definition. We should not lie by omission of saying that.

I also believe the documentation should at least talk about the trade-off with the alternative implementation scheme, which is that top-level definitions are put in a class file named after themselves, e.g., val x goes into x$toplevel.class or something like that (overloaded methods would go to the same class file).

tests/run/toplevel-overloads/defs_1.scala

tests/run/toplevel-implicits/defs_1.scala

drdozer · 2019-01-24T11:02:24Z

Will we also be able to put opaque types and their companion objects at top-level?

sjrd · 2019-01-24T12:07:20Z

Will we also be able to put opaque types and their companion objects at top-level?

https://github.com/lampepfl/dotty/pull/5754/files#diff-6368aeb78c7b36b5919b14e40a804b34

odersky · 2019-01-24T15:12:15Z

I added the suggested tests.

Regarding main methods, it follows directly from the spec. I put a note in the docs stating this, together with a note on binary compatibility.

I am not really sure we need a warning. What would such a warning say, anyway? It feels like too much hand-holding to me.

odersky · 2019-01-24T15:34:19Z

Regarding the implementation scheme, it felt most straightforward to do it this way.

I considered the alternative of naming toplevel files after definitions. Some challenges with this scheme would be:

how to handle overloading? Mangle overloaded signatures (unappealing)? Or combine all overloaded variants in one class file? But then we'd have to put in place and check restrictions that overloaded variants must all come from the same file.
how to handle type/term name matches? Encode type/term-ness in the name of the file? If we do not do that we face again the problem that different sources might define parts of the same logical output file.
how to handle side effects in initializations? This one is the killer, IMO. In the current scheme, you know what objects things get wrapped in. The first time you access a toplevel member of a source file, all definitions in that file get initialized, which might have side effects. If there is an elaborate scheme to put each definition group into its separate file, we have to explain precisely how that works so that side effects are predictable. So the implementation scheme cannot be hidden, it becomes the spec. And that spec might be quite surprising for instance in

    var a: Int = 1
    val b = { a += 1; a }
    val c = a

it looks like c would have the value 2, but since the implementation scheme would make all
val's effectively lazy it could also be 1.

Overall, it seemed more straightforward to just pick the name of the source. It also means we can re-use almost all of the existing package object infrastructure, so the added implementation effort was quite low.

docs/docs/reference/dropped-features/package-objects.md

smarter · 2019-01-24T23:05:05Z

/cc @lihaoyi, would be interesting to know if this covers all the usecases for the top-level definitions support in https://ammonite.io/#ScalaScripts currently.

lihaoyi · 2019-01-24T23:25:43Z

@smarter some, but not all.

The main thing that Ammonite scripts provide on top of this is configurable wrapping modes, e.g. :

The default REPL uses more-or-less this wrapping scheme, but with additional methods being injected into the wrapper for pretty-printing & stuff.
Class-based wrapping is necessary sometimes to make Futures, parallel collections, or Spark work top level IIRC and is used by the Ammonite-Jupyter integration Almond. Notably, class-based wrapping also breaks value classes (which cannot be nested) and so cannot be the default.
Mill uses a slightly different wrapping scheme to make sure the top-level object inherits from the relevant mill.Module trait so it can be treated as one. Mill also injects a few implicits into the wrapper object

Another thing that Ammonite scripts provide is automatic package inference: the JVM package path of the script's compilation unit is inferred based on the path of the script relative to the user's working directory or current script, more or less. This isn't fully specced out, and definitely has some holes as currently implemented, but it can be cleaned up with a bit of effort and is of great convenience. The equivalent here would be to have .scala files live in a source-root, rather than being passed to the compiler individually, so their path relative to source-root would be relevant. This isn't how Java or Scala traditionally worked, but other languages like Python, Go, etc. have used this scheme forever and it works quite well. In fact, IntelliJ already is aware of the concept of source-roots, and will highlight any file whose package declaration does not match the relative path from the relevant source root.

It's not clear to me that we want the default Scala compiler to have the flexibility to do all this stuff, but I would definitely be happy to drop Ammonite's custom logic in favor of upstream support in the Scala compiler where-ever possible

lihaoyi · 2019-01-24T23:27:05Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -954,6 +954,36 @@ object desugar {
    else Apply(ref(tupleTypeRef.classSymbol.companionModule.termRef), ts)
  }

+  /** Group all definitions that can't be at the toplebel in


toplebel -> toplevel

Suggested change

/** Group all definitions that can't be at the toplebel in

/** Group all definitions that can't be at the toplevel in

soronpo · 2019-01-25T01:28:01Z

I think top-level final values should be tested as well.

odersky · 2019-01-25T08:54:47Z

The work on private definitions includes a simplification step: private for members of a package p is now translated to private[p]. There should be no way to distinguish the two, so it's better to have only one representation. The previous attempt to treat package private more like Java's default visibility #3735 has been reverted. 731c55a explains why.

odersky · 2019-01-25T10:22:34Z

That's all from my side.

odersky · 2019-01-25T10:42:06Z

test performance please

odersky · 2019-02-01T08:30:58Z

Rebased on top of #5825.

odersky · 2019-02-01T16:17:39Z

@smarter Do you still plan to review this? I want to merge this over the weekend.

compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

smarter · 2019-02-01T18:09:21Z

compiler/src/dotty/tools/dotc/ast/Desugar.scala

@@ -954,6 +954,36 @@ object desugar {
    else Apply(ref(tupleTypeRef.classSymbol.companionModule.termRef), ts)
  }

+  /** Group all definitions that can't be at the toplebel in


Suggested change

/** Group all definitions that can't be at the toplebel in

/** Group all definitions that can't be at the toplevel in

compiler/src/dotty/tools/dotc/ast/Desugar.scala

compiler/src/dotty/tools/dotc/typer/Typer.scala

smarter

The revert of #3735 reintroduces the issue described in #3392 (comment) where code that works in join compilation fails with an error in separate compilation, I think the only way to avoid this and keep the behavior of this PR would be to emit all classes as public like scalac does.

odersky · 2019-02-02T08:52:31Z

The revert of #3735 reintroduces the issue described in #3392 (comment) where code that works in join compilation fails with an error in separate compilation, I think the only way to avoid this and keep the behavior of this PR would be to emit all classes as public like scalac does.

private for package members is widened to private[this], which is erased to public.

smarter · 2019-02-02T09:42:15Z

private for package members is widened to private[this], which is erased to public.

Ah indeed, but I can still reproduce the separate compilation issue described in #3392 (comment), when compiling separately B.scala fails with not found: type A

compiler/src/dotty/tools/dotc/ast/Desugar.scala

Change scheme how sourcenames are computed from filenames to account for filenames that have additional `.`s in them. Fix comments.

odersky · 2019-02-02T12:20:29Z

Ah indeed, but I can still reproduce the separate compilation issue described in #3392 (comment), when compiling separately B.scala fails with not found: type A

I could not reproduce that. I added a test (i3339/*.scala) to run.

smarter · 2019-02-02T13:01:18Z

Ah it does work indeed, good to have the test.

smarter

LGTM, though this PR depends on #5825 which hasn't been reviewed yet.

agilesteel · 2019-03-05T22:41:15Z

It is currently possible to mix a trait into a package object. How would one accomplish something similar when package objects are dropped? Will it no longer be possible?

smarter · 2019-03-05T22:44:49Z

@agilesteel It won't be possible indeed, but this is unrelated to this PR, for discussing dropping package objects see scala/scala-dev#441 and scala/scala#7662

Adding `// LEGACY` to 061. It seems like TopLevelCantBeImplicit is no longer the case thanks to scala/scala3#5754 This is actually confirmed in https://github.com/lampepfl/dotty/blob/93fc41fcb624df73cc12d52b79d518a30a778a7c/tests/run/toplevel-implicits/a.b.scala#L19-L21

It seems like TopLevelCantBeImplicit is no longer the case as of scala#5754 And it is actually confirmed in https://github.com/lampepfl/dotty/blob/93fc41fcb624df73cc12d52b79d518a30a778a7c/tests/run/toplevel-implicits/a.b.scala#L19-L21 This commit removes the unnecessary check in from Checking.scala and deleted the `ErrorMessage` definition for `TopLevelCantBeImplicit`. I'm leaving the `TopLevelCantBeImplicitID` in `ErrorMessageID.scala` so we don't screw up the error number.

It seems like TopLevelCantBeImplicit is no longer the case as of scala#5754 And it is actually confirmed in https://github.com/lampepfl/dotty/blob/93fc41fcb624df73cc12d52b79d518a30a778a7c/tests/run/toplevel-implicits/a.b.scala#L19-L21 This commit replace the unnecessary check in from Checking.scala to assertion and deleted the `ErrorMessage` definition for `TopLevelCantBeImplicit`. I'm leaving the `TopLevelCantBeImplicitID` in `ErrorMessageID.scala` so we don't screw up the error number.

odersky added stat:wip and removed stat:wip labels Jan 20, 2019

biboudis requested review from smarter and sjrd January 23, 2019 15:17

odersky self-assigned this Jan 23, 2019

biboudis assigned smarter and unassigned smarter Jan 23, 2019

odersky force-pushed the add-toplevel branch from ed59a5b to c0aca2c Compare January 23, 2019 21:03

odersky assigned smarter and sjrd and unassigned odersky Jan 23, 2019

sjrd requested changes Jan 24, 2019

View reviewed changes

tests/run/toplevel-overloads/defs_1.scala Show resolved Hide resolved

tests/run/toplevel-implicits/defs_1.scala Outdated Show resolved Hide resolved

odersky force-pushed the add-toplevel branch from 34d4e80 to 7d1754b Compare January 24, 2019 15:40

sjrd reviewed Jan 24, 2019

View reviewed changes

docs/docs/reference/dropped-features/package-objects.md Outdated Show resolved Hide resolved

lihaoyi reviewed Jan 24, 2019

View reviewed changes

odersky force-pushed the add-toplevel branch from 23b3839 to 81ab201 Compare January 25, 2019 09:46

Try with a final modifier

d874964

odersky force-pushed the add-toplevel branch from e875d52 to d874964 Compare February 1, 2019 08:30

smarter requested changes Feb 1, 2019

View reviewed changes

smarter reviewed Feb 2, 2019

View reviewed changes

compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved

Address review comments

ed5304e

Change scheme how sourcenames are computed from filenames to account for filenames that have additional `.`s in them. Fix comments.

odersky force-pushed the add-toplevel branch from fc22d63 to ed5304e Compare February 2, 2019 10:15

odersky added 2 commits February 2, 2019 12:29

Avoid illegal characters in package objects

db36559

Add separate compilation test case for scala#3339

537f80d

smarter approved these changes Feb 2, 2019

View reviewed changes

adriaanm mentioned this pull request Feb 4, 2019

top-level definitions scala/scala-dev#603

Closed

odersky force-pushed the add-toplevel branch from 7db38bf to 537f80d Compare February 4, 2019 15:48

odersky merged commit cd3b1ed into scala:master Feb 4, 2019

allanrenucci deleted the add-toplevel branch February 4, 2019 22:13

tanishiking mentioned this pull request Apr 14, 2022

Add in 061_TopLevelCantBeImplicit ckipp01/dotty-error-index#9

Merged

tanishiking mentioned this pull request Apr 14, 2022

refactor: TopLevel implicit is now allowed, remove unnecessary checking for toplevel implicits #14935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow toplevel definitions #5754

Allow toplevel definitions #5754

odersky commented Jan 20, 2019 •

edited

Loading

Glavo commented Jan 22, 2019

odersky commented Jan 23, 2019

therealcisse commented Jan 24, 2019

odersky commented Jan 24, 2019

sjrd left a comment

drdozer commented Jan 24, 2019

sjrd commented Jan 24, 2019

odersky commented Jan 24, 2019

odersky commented Jan 24, 2019 •

edited

Loading

smarter commented Jan 24, 2019

lihaoyi commented Jan 24, 2019 •

edited

Loading

lihaoyi Jan 24, 2019

smarter Feb 1, 2019

soronpo commented Jan 25, 2019

odersky commented Jan 25, 2019 •

edited

Loading

odersky commented Jan 25, 2019

odersky commented Jan 25, 2019

odersky commented Feb 1, 2019

odersky commented Feb 1, 2019

smarter Feb 1, 2019

smarter left a comment

odersky commented Feb 2, 2019

smarter commented Feb 2, 2019

odersky commented Feb 2, 2019

smarter commented Feb 2, 2019

smarter left a comment

agilesteel commented Mar 5, 2019

smarter commented Mar 5, 2019

	/** Group all definitions that can't be at the toplebel in
	/** Group all definitions that can't be at the toplevel in

Allow toplevel definitions #5754

Allow toplevel definitions #5754

Conversation

odersky commented Jan 20, 2019 • edited Loading

Glavo commented Jan 22, 2019

odersky commented Jan 23, 2019

therealcisse commented Jan 24, 2019

odersky commented Jan 24, 2019

sjrd left a comment

Choose a reason for hiding this comment

drdozer commented Jan 24, 2019

sjrd commented Jan 24, 2019

odersky commented Jan 24, 2019

odersky commented Jan 24, 2019 • edited Loading

smarter commented Jan 24, 2019

lihaoyi commented Jan 24, 2019 • edited Loading

lihaoyi Jan 24, 2019

Choose a reason for hiding this comment

smarter Feb 1, 2019

Choose a reason for hiding this comment

soronpo commented Jan 25, 2019

odersky commented Jan 25, 2019 • edited Loading

odersky commented Jan 25, 2019

odersky commented Jan 25, 2019

odersky commented Feb 1, 2019

odersky commented Feb 1, 2019

smarter Feb 1, 2019

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

odersky commented Feb 2, 2019

smarter commented Feb 2, 2019

odersky commented Feb 2, 2019

smarter commented Feb 2, 2019

smarter left a comment

Choose a reason for hiding this comment

agilesteel commented Mar 5, 2019

smarter commented Mar 5, 2019

odersky commented Jan 20, 2019 •

edited

Loading

odersky commented Jan 24, 2019 •

edited

Loading

lihaoyi commented Jan 24, 2019 •

edited

Loading

odersky commented Jan 25, 2019 •

edited

Loading