Proposal to disallow class shadowing


#21

AFAIU your main concern was that disabling class shadowing will force you to use extra prefixes/ suffixes:

What I shown is that you do not need that even with class shadowing disabled.

I’m just curious whether your argument (i.e. disabling class shadowing forces to use “magic names”) is valid. IMO you just need some refactoring (that I shown) and class/ trait/ etc names will stay the same.


#22

Sorry, but you have not shown it.
Your proposal not to use inner traits is very doubtful. Are you really trying to prove that inner traits are useless?
Your proposal to use objects is as usful for us as prefixes


#23

What are the deficiencies?

Not at all. OTOH, I think that if you change code generator a bit then you don’t need to come up with “magical names” nor do you need substantial changes in client code.

Well, use whatever you want. I still don’t think there’s any serious problem with disallowing class shadowing. However, I would be glad if you prove me otherwise, i.e. pinpoint a problem with my scheme that I’m not aware of.


#24

So we have agreed that we use inner traits. So your example is not completely correct.

My example is a client code, for businesses logic and customization layers.

Ok, I have said already if there were not shadowing at all it would not be a real problem. But it would be a real headache somtimes at least for me.


#25

I think the lack of shadowing leads us to “global variables”. It is not a real problems of course , but it is a bad thing.


#26

By “client code” I meant code that uses the generated classes.

I think you’re confusing shadowing with overriding. Class members in Scala can and will be able to be overridden.

What shadowing gives you except for reusing the same name for a potentially completely different thing? Remember that shadowing doesn’t care about types, so for a compiler it doesn’t matter if you shadow a parent class with a child class. You don’t gain extra flexibility from that, AFAIU.


#27

Yes, we have 2 layers which use generated code.

It just reduce coupling.

For example, reduced coupling ensures that we will not have conflicts between layers which are written by different teams.


#28

Hmm, I haven’t seen any mention of shadowing in the referenced Wikipedia article. Maybe you meant having three traits (List, Card and Lookup) vs one means coupling reduction? Sure, but I haven’t reduced the number of traits.

I see a lot of class shadowing in Slick codebase. See for example here: https://github.com/slick/slick/blob/master/slick/src/main/scala/slick/jdbc/PostgresProfile.scala :

trait PostgresProfile extends JdbcProfile {
  class ModelBuilder(mTables: Seq[MTable], ignoreInvalidDefaults: Boolean)(implicit ec: ExecutionContext) extends JdbcModelBuilder(mTables, ignoreInvalidDefaults) {
    class TableNamer(mTable: MTable) extends super.TableNamer(mTable){
    }
    class ColumnBuilder(tableBuilder: TableBuilder, meta: MColumn) extends super.ColumnBuilder(tableBuilder, meta) {
    }
    class IndexBuilder(tableBuilder: TableBuilder, meta: Seq[MIndexInfo]) extends super.IndexBuilder(tableBuilder, meta) {
    }
  }

  class QueryBuilder(tree: Node, state: CompilerState) extends super.QueryBuilder(tree, state) {
  }

  class UpsertBuilder(ins: Insert) extends super.UpsertBuilder(ins) {
  }

  class TableDDLBuilder(table: Table[_]) extends super.TableDDLBuilder(table) {
  }

  class ColumnDDLBuilder(column: FieldSymbol) extends super.ColumnDDLBuilder(column) {
  }

  class JdbcTypes extends super.JdbcTypes {
    class ByteArrayJdbcType extends super.ByteArrayJdbcType {
    }

    class LocalDateJdbcType extends super.LocalDateJdbcType with PostgreTimeJdbcType[LocalDate] {
    }

    class LocalTimeJdbcType extends super.LocalTimeJdbcType with PostgreTimeJdbcType[LocalTime] {
    }

    class OffsetTimeJdbcType extends super.OffsetTimeJdbcType with PostgreTimeJdbcType[OffsetTime] {
    }

    class InstantJdbcType extends super.InstantJdbcType with PostgreTimeJdbcType[Instant] {
    }

    class LocalDateTimeJdbcType extends super.LocalDateTimeJdbcType with PostgreTimeJdbcType[LocalDateTime] {
    }

    class UUIDJdbcType extends super.UUIDJdbcType {
    }
  }
}

object PostgresProfile extends PostgresProfile

That complex mess actually caused me problems many times as it leads to lots of path dependent types that increase coupling drastically. I have to manually loosen the types to make a database layer that can work with more than one database type. For example I have to widen types to JdbcProfile instead of using precise profile type, but even that doesn’t always cut it as at times I need to do explicit casting circumventing unnecessary path dependent types. IMO moving all those inner classes (in Slick profiles) to companion objects would make using Slick a lot smoother.


#29

If you do not think that name clashes increase coupling it means we are in different camps. We solve diffrent tasks and have different priorities. It happens.


#30

A child class is already fully coupled to a parent class. It is even mentioned in the article you’ve linked:

https://en.wikipedia.org/wiki/Coupling_(computer_programming)#Object-oriented_programming

Object-oriented programming

Subclass coupling
Describes the relationship between a child and its parent. The child is connected to its parent, but the parent is not connected to the child.

Deep inheritance hierarchies mean deep coupling. If disallowing class shadowing will reduce temptation for creating deep inheritance hierarchies then I think it’s a step in right direction. Same as with implicit conversions - they are powerful, flexible, terse, etc but they can be easily abused, so removing them will force people to come up with cleaner designs.


#31

You are right, but in our case the coupling between classes “very cheap” because it contains code that change rarely excepts traits which can be added “randomly”.


#32

Read this discussion once again, really interesting. Briefly saying, I am rather found my self in that @soronpo, @AMatveev, @Adowrath “camp”, and it seams that I have to disagree with @odersky’s opinion (related to hypothetical “virtual classes”)

My first impression about that idea was that protected should/could be enough in this case (like: no need isolated, just treat protected in the way similar to private, but impose strict overriding constrain, which anyway all ‘shadowers’(authors who use that) follow by their own will)
At least it covers @odersky’s concern about

in case if p is not this-expression and not super[...]-expression protected members will not violate that universal ruler of p.T (since they will be simply invisible though that notation, same as private ones)
Hoverer other @odersky’s concern about

will remains partially valid - one will not be able to refer parents (or even worse - “grand-parents”) hidden/shadowed nested classes (while it looks entirely “OK” for extends-case , it will become NOT OK if someone use that hidden class in some non-private method definition, in parent class)


Hoverer eventually (anyway) some form of “virtual classes” looks for me more attractive here.

I am not sure, whether I understood that point correctly, as for now it looks that I should disagree with that position.
As for my understanding “virtual classes” are currently (already) available in Scala, that could be easily expressed through abstract-types + self+types + abstract factory methods (the only basic thing that is missing - some syntax sugar for that construct - and that’s it (IMHO) …)
So from my understanding, here is valid definition of virtual nested/inner trait: complete snippet here

    trait FooLike {

      // main part - name definition (via abstract type)
      type VirtualInner <: VirtualInnerPart
      // `new VirtualInner(...)` replacement - any kind of constructors cold not be called directly,
      // so they basically should be explicitly replaced with some number of abstract factory methods on outer level
      def VirtualInner(): VirtualInner

      // virtual trait body - self-type need to be used to achieve strict overriding constraint
      trait VirtualInnerPart {
        this: VirtualInner =>
      }
    }

- if this could not be called “virtual class/trait” definition then I will appreciate any explanations “why” ?
If I understood correctly @adriaanm’s words

he refers to something similar to that code snippet.

Remarkable here, that VirtualInnerPart is in fact basically “useless” anywhere in code except extends statement.
In particular, there are no reasons to use VirtualInnerPart in methods/members definitions, since VirtualInner will be “better” anyway
VirtualInner-type provides everything that VirtualInnerPart can, because of VirtualInner <: VirtualInnerPart, and eventually any
VirtualInnerPart instances should be anyway “up-castable” to VirtualInner because of Self-type constraint this: VirtualInner =>
So what I am trying to say is that under perfect conditions I would prefer that VirtualInnerPart become anonymous at all, and the only way to deal with it - should be using VirtualInner type
(this also assumes that at some point type VirtualInner should become concrete - in that place VirtualInner name should “materialize” itself and become name/alias of some “non-overridable” class/trait - so that name VirtualInner become “frozen” from that time)

Maybe it could be not fully clear here, how it relates to highlighted problem, so I will try to explain that by rewriting of few examples.
(But briefly, main idea here that inner class “name abstractness” and “body abstractness” should be separated and decoupled)
So here is “raw” rewrite of @soronpo’s snippet (it does not contain any syntax novelties, only prepares for them)

    abstract class Foo {
      type __Dev <: __Dev_SyntheticPart0001
      trait __Dev_SyntheticPart0001 {
        // use self-type constraint to enforce proper overriding instead of "shadowing"
        this: __Dev =>
        def devFunc : Unit
      }
      lazy val __dev : __Dev = ???
      def userFunc : Unit ={}
    }

    abstract class Bar extends Foo {
      // noticeable drawback here - that it remain overridable until name is abstract, but once name become concrete, "name" override-ability is gone
      type __Dev <: __Dev_SyntheticPart0002
      trait __Dev_SyntheticPart0002 extends __Dev_SyntheticPart0001 {
        // use self-type constraint to enforce proper overriding instead of "shadowing"
        this: __Dev =>
        def devFunc : Unit = println("only for devs")
      }
      override lazy val __dev : __Dev = ???
    }

    class BarImpl extends Bar {
      type __Dev = __Dev_SyntheticPart0003
      trait __Dev_SyntheticPart0003 extends __Dev_SyntheticPart0002
      override lazy val __dev : __Dev = new __Dev {}
    }

Here it is clearly seen, that SyntheticPartXXXX-names are in fact useless (and could be hidden under some “compiler sugar”)
And then (using some hypothetical @virtual_abstract_type modifier and @virtual_concrete_type modifier) snippet about should be rewritten to following equivalent snippet:

    abstract class Foo {
      @virtual_abstract_type trait __Dev {
        // proposed construction should implicitly use self-type constraint to enforce proper overriding instead of "shadowing"
        // this: __Dev =>
        def devFunc : Unit
      }
      lazy val __dev : __Dev = ???
      def userFunc : Unit ={}
    }

    class Bar extends Foo {
      // noticeable drawback here - that it remain overridable until name is abstract, once name become concrete, "name" override-ability is gone
      @virtual_abstract_type override trait __Dev extends super[Foo].__Dev {
        // proposed construction should implicitly use self-type constraint to enforce proper overriding instead of "shadowing"
        // this: __Dev =>
        def devFunc : Unit = println("only for devs")
      }
      override lazy val __dev : __Dev = ???
    }

    class BarImpl extends Bar {
      @virtual_concrete_type override trait __Dev extends super[Bar].__Dev
      override lazy val __dev : __Dev = new __Dev {}
    }

(Here also need to be noted that under extends super[Foo].__Dev I see only some formal notation, in any other places (except extends) super[Foo].__Dev should NOT be used and/or should generally reference to this.__Dev as it does now in Dotty)
Basically that hypothetical construction should solve problem of “diverse” names like __Dev_SyntheticPartXXX availability in scope - they should be simply “invisible” (names itself should be internally assigned by compiler in any reasonable way), however it will introduce some other problem - transition form “abstract name” to “concrete/frozen name” now need to be explicit (and probably require introduction of yet an other class in outer classes hierarchy - here it is BarImpl)
Going even further - one may see that @soronpo’s snippet is in fact some way of declaring some “lazy abstract virtual object”.
In particular, for that sort of use-cases I would imagine even more specific form of “syntax sugar”

    /*
      it is not valid/compile-abe Scala code - since it uses hypothetical abstract/lazy objects notation
    */
    abstract class Foo {
      abstract lazy object __dev {
        // such construction should implicitly use self-type constraint bound to `__dev.type`, at least it looks that Dotty behalves this way
        // this: __dev.type =>
        def devFunc : Unit
      }
      def userFunc : Unit ={}
    }

    class Bar extends Foo {
      // note drawback here - that it remain overridable until name is abstract, once name become concrete, "name" override-ability is gone
      override abstract lazy object __dev extends super[Foo].__dev {
        // such construction should implicitly use self-type constraint bound to `__dev.type`, at least it looks that Dotty behalves this way
        // this: __dev.type =>
        def devFunc : Unit = println("only for devs")
      }
    }

    class BarImpl extends Bar {
      // non `abstract` objects names should automatically become `final`
      override final lazy object __dev extends super[Bar].__dev
    }

- this suggestion was in fact inspired by internally generated Dotty code (which I’ve spotted while observing dotty crashes - in fact in Dotty objects are represented as val-s, and their bodies internally defined as a classes with self-type constraints of form this: __dev.type =>)


Actually this approach could be mechanically extent to @AMatveev’s case, however in that situation it looks much more tricky.
And by saying “tricky” I mean “mixing of virtual classes” (assuming that “virtual classes construct” understanding matches to that approach mentioned above).
Generally it looks simple - all Default, List, Card, Lookup need to be internally represented as abstract types, and their body should be treated as some “anonymous”(with generated names) traits with appropriate self-tye constraints. At some point all that names should be “frozen” (abstract types should become simple type aliases) from that point (in outer types hierarchy) they will be capable to participate in new clause.
Interesting here that even if we only “refine” (“override”) Default “virtual trait” generated self-types constraints on List,Card, Lookup bodies will force us to keep List,Card,Lookup bodies consistent with up-to-date Default in that place where List,Card, Lookup names become frozen (and immediate implication here that names List,Card,Lookup could NOT become frozen “earlier” then Default name become “frozen” - because List,Card,Lookup has dependency on Default).
But anyway, I could not see any “soundness” issue (even in this case)
So here is that suggested “raw rewrite” (this is how generated code might look like after hypothetical “de-sugaring” of “abstract virtual traits”)

    class Bfb_InventoryOrderAvi extends Bfb_InventoryOrderDvi {

/*
      // could not be migrated "directly" since `new List` will not be available until `List` name is resolved to abstract type (and not simple type alias)
      // this sort of methods should be defined as abstract factories, and could be implemented only at that point in class hierarchy where name `List` become "frozen"
      override def list(): List = {
        new List {
          override def meta = this
        }
      }
*/
      type Default <: Default_SyntheticPart0002
      trait Default_SyntheticPart0002 extends super.Default_SyntheticPart0001 {
        this: Default =>
        @Setter
        override def setidObj(event: SetterEvent): Unit = {
          super.setidObj(event)
          selection.refreshItem()
        }
      }
      type List <: Default with List_SyntheticPart0002
      trait List_SyntheticPart0002 extends Default_SyntheticPart0002 with super.List_SyntheticPart0001 {
        this: List =>
      }
      type Card <: Default with List_SyntheticPart0002
      trait Card_SyntheticPart0002 extends Default_SyntheticPart0002 with super.Card_SyntheticPart0001 {
        this: Card =>
      }
      type Lookup <: Default with List_SyntheticPart0002
      trait Lookup_SyntheticPart0002 extends Default_SyntheticPart0002 with super.Lookup_SyntheticPart0001 {
        this: Lookup =>
      }
    }

Here also (probably) could be seen what was hinted by @odersky

But it looks that that statement has limited “scope” - that.Default is in fact valid anywhere in outside code, where we have that: Bfb_InventoryOrderAvi reference, but yes, if we are talking about usages of that.Default in extends statement (somewhere outside Bfb_InventoryOrderAvi body), then definitely such code will not be “OK” (but I think nobody of those “nested abstract-virtual classes” real users ever want or need such construct)
So in particular if one have code like

    trait Outer {
      trait Inner
    }

    trait Other {
      val s: Outer

      trait Strange extends s.Inner
    }

Of-course trait Strange extends s.Inner will not work if Outer#Inner will be defined as abstract virtual trait (since s.Inner will refer to abstract type in that case and that entire construct trait Strange extends s.Inner will be defiantly invalid)
But in any reasonable cases of nested classes overriding (mentioned in this discussion) that kind of constructs was never referred to.


#33

In C# they have partial classes. It is very close to prototype patching in dynamic languages (but in contrast to those - this feature is solely safe and accomplished on compile time). Also it is close to type classes (but not in generic/parametrized form). That feature works in some very simple way - it just allows to “spread” some class definition between multiple source files - resulting class definition will be retrieved by simple “merging” of all of those parts defined in different source files (if different parts contains same method definition (with impl) it will be treated as compile time error - same as if that situation happen in single file with single regular class definition). Basically this feature is used to integrate human written code with automatically generated one (in human written code generated methods will be available as extra members of this, naturally that extra members could be private as well).


#34

That’s not virtual classes. You also have to take into account that you can inherit from these things. That aspect cannot be modelled though abstract types and abstract factory methods and that’s where the complexity lies.


#35

I’ve just mentioned that - that if that inheritance appears to be inside “same container” it is relatively essy to express/implement

Essentially if few (lets name it) “semi-virtual traits” extends one an other (being in the same container class), inheritance just need to be expressed twice - once on the level of abstract types, where that extends clause could be translated strait forward, and secondly on “body part” level.

Again could not see any difficulties here (if extending/dependent “semi-virtual traits” lays in the same container)
For example in aforementioned example noone forces to define abstract factories for List or Card etc, but if one decide to do so, he/she will be able to implement that only in proper “container subclass” where type Card (for example) defined as type Card = Card_SyntheticPartXXXX (and not as type Card <: Card_SyntheticPartXXXX)
Regarding that cases of “cross container” inheritance, I’ve already mention that such kind of inheritance will not work with this interpretation, but again I don’t think that it is practicably useful (and demanded) for those people who are looking for possibility of basic nested/inner classes overriding

Probably I need not to be so lazy, and provide complete example to demonstrate what I mean without any ambiguities.

But before that I would like to highlight again that emphasize here was not on that “what could be called (semi)virtual-trait and what is not”, but rather on that fact that in aforementioned construct bodies itself (traits that defines them) could be made anonymous, and if so - no names clashes will appear, the only name that should remain - is name of abstract type (and optionally abstract factory), which could be overridden easily (and that overriding constraints are well defined)


#36

So here is example of hypothetical semi-virtual trites system with overriding and inheritance inside particular container hierarchy (no cross-container inheritance suggested). The result of “de-sugaring” of such code should look like this snippet

    trait ContainerLevel0 {

      @virtual_abstract_type
      trait Default {
        // this: Default =>
        def doSmth0(): Unit
      }

      def NestedEntry(): NestedEntry
      @virtual_abstract_type
      trait NestedEntry extends Default {
        // this: NestedEntry =>
        def getSmthFromEntry(): String
      }

      def NestedSpecificEntry(): NestedSpecificEntry
      @virtual_abstract_type
      trait NestedSpecificEntry extends Default with NestedEntry {
        // this: NestedSpecificEntry =>
        def getSmthFromSpecificEntry(): String
      }
    }

    trait ContainerLevel1 extends ContainerLevel0 {

      @virtual_abstract_type
      override
      trait Default extends super[ContainerLevel0].Default {
        // this: Default =>
        def doSmth1(): Unit
      }

      @virtual_abstract_type
      override
      trait NestedEntry extends Default with super[ContainerLevel0].NestedEntry {
        // this: NestedEntry =>
        def getSmthFromL1Entry(): String
      }

      @virtual_abstract_type
      override
      trait NestedSpecificEntry extends Default with NestedEntry with super[ContainerLevel0].NestedSpecificEntry {
        //this: NestedSpecificEntry =>
        def getSmthFromL1SpecificEntry(): String
      }
    }

    trait ContainerExtensionFrozen extends ContainerLevel1 {

      @virtual_concrete_type
      override
      trait Default extends super[ContainerLevel1].Default
      @virtual_concrete_type
      override
      trait NestedEntry extends Default with super[ContainerLevel1].NestedEntry
      @virtual_concrete_type
      override
      trait NestedSpecificEntry extends Default with NestedEntry with super[ContainerLevel0].NestedSpecificEntry
    }


#37

We did that more than 10 years ago, mostly driven by Sean McDirmid, who was a post doc in my group at the time. It was an interesting experiment, but not something I am inclined to revisit.


#38

Could it be that you expect from that construct (aka “virtual classes”) “to much” ?
I mean if you want that it support full range of operations - then it looks like you are right - it probably could never be archived (with enough level of simplicity). But in practice even some very limited construction could be useful.
And probably even if you don’t like idea to add some syntax/compiler support to facilitate aforementioned construct directly, what about some modifier/keyword that unlocks “shadowing like overriding”(for protected trites) but also dis-allows that marked trait to participate in any places except abstract types boundaries and extends clauses of child containers in same-name trait definition ?

    class ContainerLevel0 {
      type __Dev <: __DevBody

      @very_limited_type_allowed_only_as_type_boundary_and_in_extends_closure_in_sub_container_but_could_be_overridden
      protected trait __DevBody {
        this: __Dev =>
        // this `__DevBody` name should serve as some for of builder for `_Dev` actual body
        // "build process" should end when is some sub container it will be overridden with some trite without "magic modifier"
        def member0() = "member0"
      }
    }

    class ContainerLevel1 extends ContainerLevel0 {
      type __Dev <: __DevBody

      @very_limited_type_allowed_only_as_type_boundary_and_in_extends_closure_in_sub_container_but_could_be_overridden
      protected trait __DevBody extends super[ContainerLevel0].__DevBody {
        this: __Dev =>
        // this `__DevBody` should serve as some for of builder for `_Dev` actual body
        // "build process" should end when is some sub container it will be overridden with some trite without "magic modifier"
        def member1() = "member2"
      }
    }

    class ContainerExtensionFrozen extends ContainerLevel1 {
      type __Dev = __DevBody

      // no "magic modifier" is present, so body "build process" should be assumed finished - name `__DevBody` become "frozen"
      // no - `@very_limited_type_allowed_only_as_type_boundary_and_in_extends_closure_in_sub_container_but_could_be_overridden
      trait __DevBody extends super[ContainerLevel1].__DevBody
    }
  }

Or in general, did you consider some less “total” but still possibly useful overriding-like constructs ?


#39

In Scala we have self-types and mixins. Look at this:

trait CollatzPart1 { self: CollatzPart2 with CollatzPart3 =>
  def apply(x: Int): Unit = {
    print(s"$x, ")
    if (x == 1) {
      println("done")
    } else if (x % 2 == 1) {
      methodOdd(x)
    } else {
      methodEven(x)
    }
  }
}
trait CollatzPart2 { self: CollatzPart1 =>
  def methodOdd(x: Int): Unit =
    apply(x * 3 + 1)
}
trait CollatzPart3 { self: CollatzPart1 =>
  def methodEven(x: Int): Unit =
    apply(x / 2)
}
object Collatz extends CollatzPart1 with CollatzPart2 with CollatzPart3

object Main extends App {
  Collatz(5)
  Collatz(8)
  Collatz(345)
}

#40

Yes, it looks reasonable, but take into account that in C# they are just simply merging together thous parts and then compiled them - it just provides more sooth integration of human written code with automatically generated one (no run time overhead introduced by that construct, neither it support any flexibility to introduce “unknown” extension (at the mix/use site))