Allow base classes for data classes #31

Closed
udalov opened this Issue Jun 23, 2016 · 8 comments

Comments

6 participants
@udalov
Contributor

udalov commented Jun 23, 2016

Proposal

@udalov udalov added the language label Jun 23, 2016

@udalov udalov self-assigned this Jun 23, 2016

@abreslav abreslav added this to the 1.1 milestone Oct 10, 2016

@stangls

This comment has been minimized.

Show comment
Hide comment
@stangls

stangls Apr 7, 2017

👍 👍 👍

stangls commented Apr 7, 2017

👍 👍 👍

@dzharkov

This comment has been minimized.

Show comment
Hide comment
@dzharkov

dzharkov Feb 14, 2018

Collaborator

The feature has been released in Kotlin 1.1, thus I'm closing the KEEP. Do not hesitate to open a YouTrack issue for any additional suggestions.

Collaborator

dzharkov commented Feb 14, 2018

The feature has been released in Kotlin 1.1, thus I'm closing the KEEP. Do not hesitate to open a YouTrack issue for any additional suggestions.

@dzharkov dzharkov closed this Feb 14, 2018

@grodzickir

This comment has been minimized.

Show comment
Hide comment
@grodzickir

grodzickir Jul 12, 2018

Probably this has been said somewhere, but why exactly can't a data class inherit from another data class?
Let's say I have
data class Base(val a: Int)

and

data class Inherited(val b: Int):Base

I'd expect it to be generated that for Inherited class 'a' is component1 and 'b' is component2 and all the additional methods to be generated with this in mind.

It would be super useful to create hierarchies like Resource and Book or Info and DetailedInfo or multitude of other use cases where I need to add variables to a class without repeating the ones from the base class.

Probably this has been said somewhere, but why exactly can't a data class inherit from another data class?
Let's say I have
data class Base(val a: Int)

and

data class Inherited(val b: Int):Base

I'd expect it to be generated that for Inherited class 'a' is component1 and 'b' is component2 and all the additional methods to be generated with this in mind.

It would be super useful to create hierarchies like Resource and Book or Info and DetailedInfo or multitude of other use cases where I need to add variables to a class without repeating the ones from the base class.

@gildor

This comment has been minimized.

Show comment
Hide comment
@gildor

gildor Jul 12, 2018

Contributor

@grodzickir There is no way to implement hashCode and equals correctly for such hierarchy. It's not something Kotlin only, check for example AutoValue, they have exactly the same approach, you can extend abstract classes and interfaces, but cannot extend other value classes.
This is just a general limitation. For more details you check Effective Java "Obey the general contract when overriding equals" chapter

Contributor

gildor commented Jul 12, 2018

@grodzickir There is no way to implement hashCode and equals correctly for such hierarchy. It's not something Kotlin only, check for example AutoValue, they have exactly the same approach, you can extend abstract classes and interfaces, but cannot extend other value classes.
This is just a general limitation. For more details you check Effective Java "Obey the general contract when overriding equals" chapter

@grodzickir

This comment has been minimized.

Show comment
Hide comment
@grodzickir

grodzickir Jul 12, 2018

OK, I've read a bit about this and I've found that there is no way of doing it only if we want to comply to Liskov Substitution Principle, that is "you can use a subclass everywhere where you use a superclass".

I may be missing something here, but I think that generating proper equals() and hashCode() methods is perfectly possible, as we don't need to comply to LSP with them (heck, we can't even make a subclass right now).

The equals() and hashCode() functions are meant to be used especially in HashMaps and other structures where an instance of Base class should never be equal to an instance of Inherited class.

That said, even IntelliJ can generate a proper equals() function using getClass() method for comparison instead of 'instanceof' comparison which breaks the symmetry (according to mentioned Effective Java chapter). It even gives an option to accept subclasses but that makes equals() method not compliant.

So not accepting the subclasses is the right behaviour in generated functions. A subclass instance never should equal to the superclass instance.

OK, I've read a bit about this and I've found that there is no way of doing it only if we want to comply to Liskov Substitution Principle, that is "you can use a subclass everywhere where you use a superclass".

I may be missing something here, but I think that generating proper equals() and hashCode() methods is perfectly possible, as we don't need to comply to LSP with them (heck, we can't even make a subclass right now).

The equals() and hashCode() functions are meant to be used especially in HashMaps and other structures where an instance of Base class should never be equal to an instance of Inherited class.

That said, even IntelliJ can generate a proper equals() function using getClass() method for comparison instead of 'instanceof' comparison which breaks the symmetry (according to mentioned Effective Java chapter). It even gives an option to accept subclasses but that makes equals() method not compliant.

So not accepting the subclasses is the right behaviour in generated functions. A subclass instance never should equal to the superclass instance.

@stangls

This comment has been minimized.

Show comment
Hide comment
@stangls

stangls Jul 12, 2018

@grodzickir: can you provide an example for equals and hashcode? I think you are right but want to make sure we understand the same thing.

stangls commented Jul 12, 2018

@grodzickir: can you provide an example for equals and hashcode? I think you are right but want to make sure we understand the same thing.

@grodzickir

This comment has been minimized.

Show comment
Hide comment
@grodzickir

grodzickir Jul 13, 2018

@stangls I'll post the example in Java as I'm more sure about it's correctness.
(Equals methods generated in IntelliJ with "IntelliJ Default" setting and without accepting subclasses checkbox checked.)

public class Base {
    private int a;
   //getters, setters

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        Base base = (Base) o;
        return a == base.a;
    }
}
public class Inherited extends Base {
    private int b;
    //getters, setters
    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        if (!super.equals(o)) return false;
        Inherited inherited = (Inherited) o;
        return b == inherited.b;
    }
}

Checking for classes equality instead of 'instanceof' makes sure we are getting truly transitive and symmetrical methods. (I didn't include the hashCode implementation for brevity as it's straightforward)

grodzickir commented Jul 13, 2018

@stangls I'll post the example in Java as I'm more sure about it's correctness.
(Equals methods generated in IntelliJ with "IntelliJ Default" setting and without accepting subclasses checkbox checked.)

public class Base {
    private int a;
   //getters, setters

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        Base base = (Base) o;
        return a == base.a;
    }
}
public class Inherited extends Base {
    private int b;
    //getters, setters
    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        if (!super.equals(o)) return false;
        Inherited inherited = (Inherited) o;
        return b == inherited.b;
    }
}

Checking for classes equality instead of 'instanceof' makes sure we are getting truly transitive and symmetrical methods. (I didn't include the hashCode implementation for brevity as it's straightforward)

@gildor

This comment has been minimized.

Show comment
Hide comment
@gildor

gildor Jul 13, 2018

Contributor

@grodzickir Even if you don't care about LSP, there is another important thing.
To inherit data class you should make it open, so you lose SmartCast for most of cases.
Check this discussion https://discuss.kotlinlang.org/t/classes-final-by-default/166

And if you need a Base class, do you really want to make it data class? Why not just open class or abstract class

Contributor

gildor commented Jul 13, 2018

@grodzickir Even if you don't care about LSP, there is another important thing.
To inherit data class you should make it open, so you lose SmartCast for most of cases.
Check this discussion https://discuss.kotlinlang.org/t/classes-final-by-default/166

And if you need a Base class, do you really want to make it data class? Why not just open class or abstract class

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