jcoplien / trygve

The trygve language project - Building a DCI-centric language from the ground up
GNU General Public License v2.0
103 stars 15 forks source link

Liskov substitution principle #112

Closed mbrowne closed 8 years ago

mbrowne commented 8 years ago

Should trygve be enforcing the Liskov substitution principle? I had heard this principle referenced before, but just today actually learned about it...so I may not understand it fully yet, but it seems that this code should not compile, but it does:

class A {
    public void foo(int x, int y) {}
}

class B extends A {
    public void foo(int x) {}
}

{
    //this should compile
    new A().foo(1, 2);
    //but this should not, right?
    new B().foo(1, 2);
}

Additionally, I get the following error when running the code:

Error: User process died because of internal trygve error: java.lang.AssertionError
popping stack: type is RTIntegerObject
popping stack: type is RTIntegerObject
popping stack: type is RTObjectCommon
    In script `A.foo'

java.lang.AssertionError
    at info.fulloo.trygve.run_time.RTMessageDispatcher.genericPolymorphicMethodLookup(RTMessageDispatcher.java:609)
    at info.fulloo.trygve.run_time.RTMessageDispatcher.genericMethodDeclLookup(RTMessageDispatcher.java:687)
    at info.fulloo.trygve.run_time.RTMessageDispatcher$RTNonContextToNonContext.getMethodDecl(RTMessageDispatcher.java:854)
    at info.fulloo.trygve.run_time.RTMessageDispatcher.commonWrapup(RTMessageDispatcher.java:392)
    at info.fulloo.trygve.run_time.RTMessageDispatcher$RTNonContextToNonContext.<init>(RTMessageDispatcher.java:840)
    at info.fulloo.trygve.run_time.RTMessageDispatcher.makeDispatcher(RTMessageDispatcher.java:76)
    at info.fulloo.trygve.run_time.RTExpression$RTMessage.run(RTExpression.java:708)
    at info.fulloo.trygve.run_time.RunTimeEnvironment.runner(RunTimeEnvironment.java:445)
    at info.fulloo.trygve.run_time.RunTimeEnvironment.run(RunTimeEnvironment.java:227)
    at info.fulloo.trygve.editor.TextEditorGUI.simpleRun(TextEditorGUI.java:994)
    at info.fulloo.trygve.editor.TextEditorGUI$32.doInBackground(TextEditorGUI.java:1060)
    at info.fulloo.trygve.editor.TextEditorGUI$32.doInBackground(TextEditorGUI.java:1053)
    at javax.swing.SwingWorker$1.call(SwingWorker.java:296)
    at java.util.concurrent.FutureTask.run(FutureTask.java:262)
    at javax.swing.SwingWorker.run(SwingWorker.java:335)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
    at java.lang.Thread.run(Thread.java:745)
jcoplien commented 8 years ago

Hej, Matt,

Thanks for the report. The compiler is correct: both calls are legal calls. There was a bug in the virtual machine that was causing the message dispatcher not to be able to find the script. It’s been fixed in the most recent release. And also in the newest release I’ve tightened up the rules for covariance and contravariance checking.

But since there seems to be some confusion about the LSP I thought I’d post this to the list along with a quick refresher course on some OO basics.

An old version of the LSP is: “If for each object o₁ of type S there is an object o₂ of type T such that for all programs P defined in terms of T, the behavior of P is unchanged when o₁ is substituted for o₂, them S is a subtype of T.” The "behavior of P is unchanged” is kind of squishy. The LSP is actually much more than substitutability, and is more properly stated as "if properties provable for objects of one type T1 imply that the same properties are provable for another type T2, then T2 is a subtype of T1.”

“Properties” include pre- and post-conditions for given functions that are applied to objects of the type. A function, as in the mathematical sense, is a mapping from a domain to a range. In the example of your question, there are two separate functions: their domains are distinct. The LSP doesn’t apply to the properties related to the functions of your example, so the LSP is irrelevant with respect to this bug.

Preconditions often tell us what we are allowed to assume about the properties of the method arguments (comparing methods of two objects pairwise) whereas postconditions tell us what we are allowed to assume about the properties of the object(s) returned by the methods. So if a base class method requires a short integer for its first arguments, it is perfectly valid for the derived class to override that method with a method whose first argument is a long integer. The invariants for the derived class arguments can be no less strong than those on the base class arguments: i.e., derived class arguments are more restrictive. One of the tacit arguments is of course self, which naturally follows this pattern. Other arguments follow the rule structure for self. This is called covariance.

Return values work the other way. A client using a derived class object through a base class identifier expects the returned objects to at least have the properties of the type returned by the base class method. It may have additional properties (e.g., it may have even more methods in its interface that the corresponding base class return value features). This goes the opposite way of the parameters and is called contravariance.

So a Liskov-compliant inheritance graph features methods that are covariant in their argument properties and contravariant in their return values.

Each of these classes gives rise to a different form of object, so many forms of objects can stand in where the code is originally written for one kind of object. “Many forms” = “poly morphos” in Greek, from which we get the term polymorphism. The kind of polymorphism supported by inheritance and its related dynamic techniques is called inclusion polymorphism. However, it’s important to recognize that the ability to say both “1 + 2” and “1.0 + 2.0” makes “+” a polymorphic operator, though its polymorphism falls into a class called overloading. (There are two other common forms of polymorphism, as we find in trygve templates, and as some languages allow as casting. The trygve language does automatic casting and issues a warning if it does so.)

Your example is close to overloading: you are overloading the name foo to mean two different things, to represent two entirely separate and incompatible mappings. The selection is completely resolvable at compile-time. The reason that it’s not overloading in trygve (and in most other languages) is that both signatures do not appear in the same scope. So they’re just two completely unrelated scripts. Java has some kind of weird treatment of such situations which has always baffled me and which leads me to believe that the Java people don’t understand even old-fashioned OO.

Most polymorphism in DCI takes place in the ability of many different (ordered) sets of objects to play a(n ordered) set of Roles. So the polymorphic mapping in DCI is not base class to derived classes, but rather a set of Roles onto a set of classes whose objects can play those Roles. This subtyping structure in trygve is a DAG rather than a tree, but because Roles and classes are different things, we never have to concern ourselves with the structure of the DAG, nor express the DAG in code. Of course there is ordinary class polymorphism, too, and class derivation is still useful, but it’s not the focus of DCI.

So here’s a puzzle for you. Which of these script declarations are illegal?

class Base { }

class Derived extends Base { }

class Base2 { public Derived func(int i, Base j) { return null } public Base func2(int i, Derived j) { return null } public Base func3(int i, Base j) { return null } public Derived func4(int i, Derived j) { return null } }

class Derived2 extends Base2 { public Derived func(int i, Base j) { return null } public Base func2(int i, Derived j) { return null } public Derived func3(int i, Derived j) { return null } public Base func4(int i, Base j) { return null } public void test() { } }

class Derived2b extends Base2 { public Base func(int i, Base j) { return null } public Derived func2(int i, Derived j) { return null } public Base func3(int i, Derived j) { return null } public Derived func4(int i, Base j) { return null } }

class Derived2c extends Base2 { public Base func(int i, Derived j) { return null } public Derived func2(int i, Base j) { return null } public Base func3(int i, Base j) { return null } public Derived func4(int i, Derived j) { return null } }

class Derived2d extends Base2 { public Derived func(int i, Derived j) { return null } public Base func2(int i, Base j) { return null } public Derived func3(int i, Base j) { return null } public Base func4(int i, Derived j) { return null } }

On 06 May 2016, at 23:56, Matt Browne notifications@github.com wrote:

Should trygve be enforcing the Liskov substitution principle? I had heard this principle referenced before, but just today actually learned about it...so I may not understand it fully yet, but it seems that this code should not compile, but it does:

class A { public void foo(int x, int y) {} }

class B extends A { public void foo(int x) {} }

{ //this should compile new A().foo(1, 2); //but this should not, right? new B().foo(1, 2); } Additionally, I get the following error when running the code:

Error: User process died because of internal trygve error: java.lang.AssertionError

jcoplien commented 8 years ago

First time (in a good way) I've seen LSP converted to contra/covariance, thanks for that.

jcoplien commented 8 years ago

First time (in a good way) I've seen LSP converted to contra/covariance, thanks for that.

Reading the Wikipedia entry (have I not done that before? Probably. I have become addled in the memory. What was I talking about?) I see it is in there via mentioning DbC. I gotta start taking supplements, what stems dementia?

ciscoheat commented 8 years ago

Hmm, it seems that github fooled me with the quoting (again), now that I see in the DCI group that Raoul made those two last posts, not Cope. Cross-posting to github issues creates a little security/identity hole.

jcoplien commented 8 years ago

The crash no longer happens and the compiler is correct in its lack of reporting. I believe this can be closed.

mbrowne commented 8 years ago

I'm closing this since it seems that the bugs I mentioned have been fixed. But as I was verifying one of them, I accidentally copied and pasted only part of one of my examples and encountered a new issue...I copied just the test code and clicked the Parse button before I noticed that I had forgotten to include the declarations of Base and Derived:

class Base2 {
    public Derived func5() { return null }
}

class Derived2 extends Base2 {
    public Base func5() { return null }
}

class SomeClass {
    public void test(Base2 o) {
        Derived x = o.func5();
    }
}

{
    new SomeClass().test(new Derived2());
}

The compiler correctly complains that the return types are not declared, but there's also a NullPointerException:

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException
    at info.fulloo.trygve.parser.Pass2Listener.checkMethodAccess(Pass2Listener.java:280)
    at info.fulloo.trygve.parser.Pass2Listener.exitMethod_decl(Pass2Listener.java:239)
    at info.fulloo.trygve.parser.KantParser$Method_declContext.exitRule(KantParser.java:2301)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.exitRule(ParseTreeWalker.java:71)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:54)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:52)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:52)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:52)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:52)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:52)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:52)
    at org.antlr.v4.runtime.tree.ParseTreeWalker.walk(ParseTreeWalker.java:52)
    at info.fulloo.trygve.parser.ParseRun.pass2(ParseRun.java:100)
    at info.fulloo.trygve.parser.ParseRun.commonInit(ParseRun.java:74)
    at info.fulloo.trygve.parser.ParseRun$GuiParseRun.<init>(ParseRun.java:133)
    at info.fulloo.trygve.editor.TextEditorGUI.parseButtonActionPerformed(TextEditorGUI.java:1122)
    at info.fulloo.trygve.editor.TextEditorGUI$12.actionPerformed(TextEditorGUI.java:563)
    at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2018)
    at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2341)
    at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:402)
    at javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:259)
    at javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:252)
    at java.awt.Component.processMouseEvent(Component.java:6516)
    at javax.swing.JComponent.processMouseEvent(JComponent.java:3321)
    at java.awt.Component.processEvent(Component.java:6281)
    at java.awt.Container.processEvent(Container.java:2229)
    at java.awt.Component.dispatchEventImpl(Component.java:4872)
    at java.awt.Container.dispatchEventImpl(Container.java:2287)
    at java.awt.Component.dispatchEvent(Component.java:4698)
    at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4832)
    at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4492)
    at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4422)
    at java.awt.Container.dispatchEventImpl(Container.java:2273)
    at java.awt.Window.dispatchEventImpl(Window.java:2719)
    at java.awt.Component.dispatchEvent(Component.java:4698)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:740)
    at java.awt.EventQueue.access$300(EventQueue.java:103)
    at java.awt.EventQueue$3.run(EventQueue.java:699)
    at java.awt.EventQueue$3.run(EventQueue.java:697)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
    at java.awt.EventQueue$4.run(EventQueue.java:713)
    at java.awt.EventQueue$4.run(EventQueue.java:711)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:710)
    at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
    at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
    at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:150)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:146)
    at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:138)
    at java.awt.EventDispatchThread.run(EventDispatchThread.java:91)

If this turns out to be more than a simple issue with a quick fix I can report this again as a new issue report.

jcoplien commented 8 years ago

On 09 May 2016, at 23:44, Matt Browne <notifications@github.com mailto:notifications@github.com> wrote:

I'm closing this since it seems that the bugs I mentioned have been fixed. But as I was verifying one of them, I accidentally copied and pasted only part of one of my examples and encountered a new issue...I copied just the test code and clicked the Parse button before I noticed that I had forgotten to include the declarations of Base and Derived:

Fixed — a classical error-stumbling error. It will be in the next release I push to GitHub, which will probably wait for the next major feature.