Skip to content
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

Drop package objects with inheritance #441

Closed
adriaanm opened this issue Nov 21, 2017 · 41 comments · Fixed by scala/scala#7662
Closed

Drop package objects with inheritance #441

adriaanm opened this issue Nov 21, 2017 · 41 comments · Fixed by scala/scala#7662

Comments

@adriaanm
Copy link
Contributor

Replace with support for top-level definitions?

@adriaanm adriaanm added this to the 2.14 milestone Nov 21, 2017
@olafurpg
Copy link

Can you elaborate on the issues with supporting package objects?

This would be a fairly breaking change for libraries like Scalameta that use package objects + inheritance to expose the same api through multiple namespaces https://github.com/scalameta/scalameta/blob/23de1ca839e53ad6387981bd7cced35122a2cf76/scalameta/scalameta/shared/src/main/scala/scala/meta/package.scala#L3-L14
For example, import scala.meta._ imports all scalameta modules while if you can depend only on the parsers package and import scala.meta.parsers._.

For migration, we could require users to add an explicit import on the package object

import scala.meta.`package`._
import scala.meta._

Would it be possible to simplify the implementation of package objects in the compiler to perform a similar desugaring?

@dwijnand
Copy link
Member

In sbt we also use package object inheritance to compose the namespace that import sbt._ gives you:

https://github.com/sbt/sbt/blob/v1.1.0-M1/sbt/src/main/scala/package.scala#L6-L20

So I think namespace composition is a feature that should be preserved in some fashion.

On the other hand I'm also very much in favour of the ability to define top-level definitions! 💯

@nafg
Copy link

nafg commented Nov 22, 2017

If someone revives the long-ago proposed notion of "exports" (dual of imports), it might solve some of the above use cases

@adriaanm
Copy link
Contributor Author

I was thinking about exports too, as an alternative, but it occurred to me they're likely worse then package objects that can inherit definitions when it comes to having at least some phase distinction between constructing the outline of the symbol table and full on type checking. With exports, you'd have to do full type checking (unless you restrict them to fully qualified references to packages), just to determine an outline of the packages and their top-level definitions.

That's also the main problem of inheritance for package objects: what does it even mean for a package object to inherit from a class that's inside the package that we're defining?

/cc @retronym, could you expand on this? Feel free to edit the issue description

@dwijnand
Copy link
Member

If it's useful, here's a ticket where @gkossakowski fixed a package object + inheritance issue in sbt 0.13's zinc: sbt/sbt#2326.

@szeiger
Copy link

szeiger commented Nov 28, 2017

Removing package object inheritance also prevents you from scoping implicits with different priorities into a package.

@megri
Copy link

megri commented Dec 12, 2017

This is "kinda" (add quotes to taste) like a suggestion I made a while ago: scala/scala3#1882

I never felt like package objects were ergonomical but felt like they were a crutch to get top level definitions into a namespace.

The comment about import priorities is interesting but that kinda feels like a crutch too..

@dwijnand
Copy link
Member

@adriaanm Is this ticket about dropping the ability to use inheritance in package objects, or dropping package objects entirely - because of issues with inheritance?

I first understood this to mean the former, but I see the latest SIP meeting agenda and minutes refer to this topic as "Remove package objects": http://docs.scala-lang.org/sips/minutes/2017-12-06-sip-minutes.html#to-be-discussed, and you crossing out package objects in https://adriaanm.github.io/reveal.js/scala-2.13-beyond.html#/7/3.

Thanks.

@megri
Copy link

megri commented Mar 9, 2018

I've given my proposal from a year ago some more thought. Not sure if it's the proper place but as my original issue is now closed I created a gist: https://gist.github.com/megri/22bf37fb6406ff104de18c58ff0a6404

It deals with top level definitions and restrictions of implicits in a somewhat controlled way.

@SethTisue
Copy link
Member

(this was discussed by the SIP committee at the SIP meeting this week, but the notes haven't been published yet)

@soronpo
Copy link

soronpo commented Mar 10, 2018

I think the main use-case for inheritance in package objects is to save the user from multiple import statements, yet still maintain proper code distribution across files. So I'm proposing a new language feature which is fairly simple, I think.
Lets introduce an import alias (or bundle). Meaning, if I have import theEntireLib = import myLib.{core._, implicits._, otherStuff._}, and I use import theEntireLib then that's just syntactic sugar for import myLib.{core._, implicits._, otherStuff._}.
You can limit the alias to the scope of the package it resides in, so people can't just import several libraries in one import alias (creating a huge binary as the sum of all those libraries).

@fanf
Copy link

fanf commented Apr 5, 2018

I encourage everybody to read the above link (kentuckymule: "inch toward the finish line"): @gkossakowski explains the impact of package objects on compilation and semantic problems that come with them.

gkossakowski/kentuckymule#6 (comment)

eed3si9n added a commit to eed3si9n/scala that referenced this issue May 2, 2018
This deprecates package objects with inheritance, and removes it under -Xsource:2.14.

Ref scala/scala-dev#488
Ref scala/scala-dev#441
eed3si9n added a commit to eed3si9n/scala that referenced this issue May 2, 2018
This deprecates package objects with inheritance, and removes it under -Xsource:2.14.

Ref scala/scala-dev#488
Ref scala/scala-dev#441
@SethTisue
Copy link
Member

It came up today on scala/collection-strawman Gitter that having a package object inherit from something Scala-version-specific is a useful technique for doing 2.11/2.12/2.13 cross-building /cc @milessabin

@milessabin
Copy link

See the shapeless package object here, extending this for 2.13- and this for 2.13+.

@sjrd
Copy link
Member

sjrd commented Jun 20, 2018

We had a similar use case, but we opted for a different solution: in one of our package.scala, we import type aliases and helper methods from a Compat object (for 2.12- and for 2.13+), and we realias and bridge them in the package object. That way we keep a unique package.scala file, without duplicating it, which transitively re-exports stuff from Compat which is version-dependent. And all of that without having our package object extend anything.

@milessabin
Copy link

Aside from the above, I realized it's probably not super-smart to pollute the shapeless._ namespace with a bunch aliases.

@dwijnand
Copy link
Member

You can mark them private[shapeless].

(h/t @mpilquist scala/scala#5135 (comment))

@olafurpg
Copy link

I still struggle to understand why package objects with inheritance are more problematic than regular objects that are explicitly imported in all compilation units.

That's also the main problem of inheritance for package objects: what does it even mean for a package object to inherit from a class that's inside the package that we're defining?

@adriaanm how is this a different problem from the follow scenario?

// Dogs.scala
object Dogs extends Animals // psuedo package object

// Animals.scala
import Dogs._
trait Animals {
  def sound(): Unit = println("woff")
}
object Animals {
  sound() // Dogs.sound()
}

If the complications come from package clause bindings having lower precedence than explicit imports (see specification), can we avoid the problem by making the precedence for package objects bindings the same as from explicit imports?

@dragos
Copy link

dragos commented Jul 10, 2018

@adriaanm how is this a different problem from the follow scenario?

Not him, but I've seen this failing in practice often enough that I have an opinion on it :)

The basic idea is that a package object definition should not see the members of the package it defines, but it does, and that's used in 99% of the uses in the wild Your examples side-steps it by extending Animals that's defined outside of Dogs. To be the same, you'd need something like this (that doens't work, of course):

object Dogs extends Dogs.Animal {
  class Animal
}

If package objects can't extend classes from inside themselves (conceptually), this would work fine (at least, I think it would work in a lot more cases than not).

@adriaanm
Copy link
Contributor Author

adriaanm commented Aug 3, 2018

Thanks, @dragos. I agree it would conceptually be ok for package objects to extend classes from other packages. The problem is that I'm not sure how to implement this restricted form, or even if it's still useful in practice.

@dwijnand
Copy link
Member

dwijnand commented Jan 17, 2019

Right. Yeah, this is like every other use case that's being dropped, as mentioned in the message in the PR:

Losing inheritance for package objects means that some code duplication
will be required (you'll have to add val/type aliases for members,
instead of inheriting the members directly). You also can't use inheritance
to prioritize implicits, but, implicits defined in packages are also
being phased out (i.e., package prefixe don't contribute to the implicit scope).

So for extension methods: implicit def MyIntOps(i: Int): MyIntOps = new MyIntOps(i)

@adriaanm
Copy link
Contributor Author

See also #446, where packages in the prefix of a type will no longer contribute to the implicit scope.

I don't have a ticket handy, but there are multiple discussion about the downsides of uncontrolled contribution of implicit definitions to the implicit scope, leading e.g. to the implicit import idea.

I am strongly in favor of a being more explicit about the implicits you have/want in scope (e.g. by importing them, rather than getting them for "free" / unexpectedly from packages you have lying around).

@ryan-williams
Copy link

I love using package-obj inheritance to work around the lack of a way to customize Predef. For example, I do this frequently:

package object foo
     with cats.   syntax.AllSyntax
     with cats.instances.AllInstances

to save O(N) import cats.implicits._ (one per file in package). Sometimes there are a lot of imports that I normalize this way; losing that ability altogether would be really inconvenient.

Can I replace my package-obj inheritance with predef-customizations today? Is removing the former contingent on supporting the latter?

Specifying "always in scope" imports in a more direct way than the package-obj-inheritance work-around would be great, but it sounds like maybe I misunderstood, and the plan is to just remove the work-around without a replacement?

@adriaanm
Copy link
Contributor Author

yes, you can customize predef -- @som-snytt to the rescue (scala/scala#6589, #474)

@dwijnand
Copy link
Member

Customised predefs are a little invisible though, being defined in the build rather than in the imports. Being able to compose things together and then refer to the composition is very convenient. It needn't be by inheritance but losing it entirely kind of sucks.

@adriaanm
Copy link
Contributor Author

adriaanm commented Jan 17, 2019

True, but no one is taking away the alternative

object foo
     extends cats.   syntax.AllSyntax
     with cats.instances.AllInstances

import foo._

@dwijnand
Copy link
Member

You're right. 👍

@ryan-williams
Copy link

Thanks for the links to the custom predef work, I'd seen those but couldn't find them earlier.

The object foo + import foo._ pattern still requires one import for that purpose in every file; I guess that's not so bad.

@adriaanm
Copy link
Contributor Author

I think it's actually desirable to have a least an explicit entry point (an import in this case) to understanding where implicits come from, rather than having them in scope automatically in your whole package. The core principle behind implicits is great, but it's implementation details like this that make them hard to grok for many developers.

@SethTisue
Copy link
Member

scala/scala#7857 delays the deprecation until 2.14. Toplevel definitions remain in the works for Scala 3.

@Jasper-M
Copy link

Actually toplevel definitions can replace package objects without inheritance. There is no replacement for package objects with inheritance. Probably stating the obvious here, but I felt like someone should...

@ryan-williams
Copy link

Yea, I'd also like to flag that the "you should have to import package-level implicits explicitly, for readability" reasoning (here and on the PR) seems iffy (@adriaanm):

I think it's actually desirable to have a least an explicit entry point (an import in this case) to understanding where implicits come from, rather than having them in scope automatically in your whole package.

This change isn't preventing people from putting implicits in (non-inherited) package-objects (analogous implicit top-level definitions will also be supported, IIUC).

it is just removing the current ability to compose and reuse existing implicits as package-scoped.

Maybe that simplifies some compiler internals so significantly that it's worth it, but it's not really being framed that way afaict. To a user it just looks like needlessly adding a special-case that hamstrings some existing functionality.

I always found it delightful that we got composable, reusable package-scoped implicits as a side effect of making a half-baked Java concept (packages) 1st-class. It seems like a good example of the "few, orthogonal language features which compose well" principle.

So this plan seems naively like adding complexity while removing functionality (and regularity).

@megri
Copy link

megri commented Mar 13, 2019

I agree with @ryan-williams. Getting implicits with your package context is a feature. If I want implicit imports to be optional I put them under a separate package and have the use-site import everything from that package explicitly. Now that I think of it, the import implied change forces me to either put my implicit instances right next to their type definition, or do something silly like import implied foo.bar.implicits—but maybe that's actually a good thing… 🤔

Explicitly importing implicits dodges an issue of conflicting implicit instances but this seems more like a priority resolution issue. Seeing how "nested" implicits work in Dotty, wouldn't this be solved if the resolution chain went something like scope > class > file > package > companion object ?

xuwei-k added a commit to scalaz/scalaz that referenced this issue Jul 12, 2020
```
[warn] /home/travis/build/scalaz/scalaz/core/src/main/scala/scalaz/syntax/package.scala:9:23: package object inheritance is deprecated (scala/scala-dev#441);
[warn] drop the `extends` clause or use a regular object instead
[warn] package object syntax extends Syntaxes
[warn]                       ^
```
xuwei-k added a commit to scalaz/scalaz that referenced this issue Jul 12, 2020
```
[warn] /home/travis/build/scalaz/scalaz/effect/src/main/scala/scalaz/syntax/effect/package.scala:9:23: package object inheritance is deprecated (scala/scala-dev#441);
[warn] drop the `extends` clause or use a regular object instead
[warn] package object effect extends EffectSyntaxes
[warn]                       ^
[warn] one warning found
```
@eed3si9n
Copy link
Member

eed3si9n commented Oct 6, 2023

Hello from 2023. I am leaving this note because currently there's a compiler error under -Xsource:3 that links to this issue:

$ scalac -Xsource:3 -d /tmp/sandbox po.scala
po.scala:2: error: package object inheritance is deprecated (https://github.com/scala/scala-dev/issues/441);
drop the `extends` clause or use a regular object instead
Scala 3 migration messages are errors under -Xsource:3. Use -Wconf / @nowarn to filter them or add -Xmigration to demote them to warnings.
Applicable -Wconf / @nowarn filters for this fatal warning: msg=<part of the message>, cat=scala3-migration
package object p extends Protocols {
                 ^

We have Scala 3.3.0, and we can discuss Scala 3 migration more concretely.

  • As of Scala 3.3.0, a package object can inherit (extend) traits like we can in 2.13.
  • However, in Scala 3, "Package prefixes no longer contribute to the implicit search scope of a type" according to Changes in Implicit Resolution.

This means that in Scala 3, without -source:3.0-migration, Implicits without the import tax technique would no longer work for package objects, and defining implicits or givens would still require imports at the call site. Companion objects would be ok. So if you are using package object inheritance for the purpose of typeclass instances, consider changing your code.

@lrytz
Copy link
Member

lrytz commented Nov 21, 2023

I removed the deprecation / warning in scala/scala#10573

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.