uniconproject / unicon

http://www.unicon.org
Other
54 stars 28 forks source link

invoke method is not working #422

Closed IanTrudel closed 1 week ago

IanTrudel commented 1 month ago

The package lang provides the utility method invoke but it does not work. It was confirmed when I ran objecttests. Unicon version 13.3 on Linux.

Output from unicon/tests/lib/objecttests.icn:

Check to see if d is an instance of Class.

d is NOT an instance of Class!

Test string invocation of method names...should see '123'

List all the methods in class D...

DTest_write

Check the inheritance hierarchy

ATest is NOT an instance of Class
ATest is NOT an instance of BTest
BTest is NOT an instance of ATest
BTest is NOT an instance of BTest
CTest is NOT an instance of Class
CTest is NOT an instance of ATest
DTest is NOT an instance of Class
DTest is NOT an instance of ATest
DTest is NOT an instance of BTest
DTest is NOT an instance of CTest
ETest is NOT an instance of Class
ETest is NOT an instance of FTest
FTest is NOT an instance of ETest

d inherits from: DTest ATest CTest lang::Object

e inherits from: ETest FTest lang::Object ETest

g fields: one two three
h fields: four five six one two three
h.one -> 1
h.one -> One
Attempt to set non-existent field: ok - field doesn't exist
StephenWampler commented 1 month ago

Okay, there are lots of interesting things going wrong here. Starting with the simple and heading towards the complex:

1) The class "Object" no longer subclasses the class "Class", so objecttests.icn needs to change all references to the Class class into references to the Object class.

2) The procedure instanceof in uni/lib/predicat.icn currently calls the istype procedure. istype appears to be broken, possibly in several ways (for one thing it calls get_type but get_type doesn't traverse the inheritance hierarchy). A workaround would be to modify the instanceof method to call lang::Type instead of istype. The correct thing may be to fix get_type but who knows what that may break?

3) In uni/lib/object.icn, the method invoke calls hasMethod, which has the line:

return mName == genMethods()

however, genMethods now calls the builtin function methodnames to generate the names of methods. But methodnames returns the internal names of class methods, e.g. "DTestwrite". I think this is wrong but, again, what breaks if it's fixed? One would think that a workaround would be to modify genmethods to map the internal names back into external ones. That's not sufficient because invoke(), assuming that's fixed, will die because the table lookup it uses (self.m[mName]) will fail because the external name, e.g. "DTest::write" won't be found in __m - it needs to be just "write", so a 2nd workaround is needed to map the full external method name into its simple form. Note that fixing the 'wrong' methodnames function to return the simple form of method names would be sufficient unto itself (he claims).

IanTrudel commented 1 month ago

This is an amazingly insightful reply. Thank you! My current workaround is to use __m[mname] but it's not without its flaws. It requires the receiver object to be passed on as the first parameter, i.e. object.__m[mnane](object), otherwise it will send it to self. In the following code example, you will notice that the output is Child::print() instead of Parent::print(), which is not a typical behaviour for OOP. It would most likely require to call o.__m[mname](self) in the run method of the Parent class.

Output:

Anonymous::print()
Child::print()
class Parent(__objects)
   method add(o)
      put(__objects, o)
   end

   method print()
      write(classname(self) || "::print()")
   end

   method run()
      local mname

      every o := !__objects do {
         cname := classname(o)

         every m := !methodnames(o) do {
            m ? {
               tab(upto(cname || "_")) & move(*cname + 1)
               mname := tab(0)
            }
            o.__m[mname](o)
         }
      }
   end

initially
   __objects := []
end

class Child : Parent()
   method print()
      self.Parent.print()
   end
end

class Anonymous()
   method print()
      write(classname(self) || "::print()")
   end
end

procedure main()
   p := Parent()
   a := Anonymous()
   c := Child()
   p.add(a)
   p.add(c)
   p.run()
end
StephenWampler commented 1 month ago

I'm not sure what's causing the "Child::print()" output or even it's the "Right Thing To Do" but one suggestion for your code: in place of upto(cname||""), use find(cname||"") the upto function converts its first argument to a cset and goes up to the first appearance of any character in that cset. You're actually lucky that the class names you've got work with upto.

I assume also that this is a test program to explore behavior. Ordinarily, omitting the print method from Child would default a call to print to the Parent's print() [but still output "Child::print()", of course]. However, the run() method won't find the Parent's print method in that case. I suspect you know that and are just experimenting.

StephenWampler commented 1 month ago

Ah, the reason run() doesn't find the print() method in Parent when the subclass doesn't have it is because methodnames() is only returning the 'local' methods, not any derived from ancestors. This is, he claims, another problem with the methodnames implementation. The __m list has all the methods (included inherited ones) in it but methodnames() only provides the local ones. I ;think fixing methodnames() and invoke() may be non-trivial.

StephenWampler commented 1 month ago

I played around a bit with your test program and made some changes. The big ones are that the run() method:

(a) now takes as a argument the name of the method (e.g. "print") to run. This version doesn't accept any arguments to pass to that method - that should be fixed.

(b) now can find a method that exists back in the class hierarchy (it avoids using the broken methodnames)

(c) only involkes the first instance of a method from the hierarchy, starting from the 'local' and going up into superclasses

Assuming the issue stated in (a) is addressed, I think the run() method points to a viable replacement for the invoke() method.

Anyway, the amended code:

class Parent(objects) method add(o) put(objects, o) end

method print() write(classname(self) || "::print()") end

method run(s) # Pass in name of method to run, should also pass in args but doesn't yet... local mname

  every o := !__objects do {
     cname := classname(o)

     # If we find that method, then call it and stop looking for it!
     #   Note: this is prelim version that doesn't pass any args to method.
     every m := image(!o.__m) do {
        m ? {
           mname := (="procedure ", tab(0))  | next
           if mname ? (tab(-(*s+1)),="_",match(s)) then break mname(o)
        }
     }
  }

end

initially __objects := [] end

class Child : Parent() end

class Anonymous() method print() write(classname(self) || "::print()") end end

procedure main() p := Parent() a := Anonymous() c := Child() p.add(a) p.add(c) p.run("print") end

brucerennie commented 1 month ago

Good morning Ian,

For my purposes, I built a class hierarchy to get around some of the problems within the library Class/Object that I found. My particular solution is found in the two files in lib called classobject.icn and errorsystem.icn.

This is used in utf8.icn (also in lib).

Fundamentally, the problem is that there is no direct correspondence between the user visible package/class/method names and the internal names. I am looking at a possible change here, but only after I finish the new keyword additions.

regards and blessings

Bruce Rennie

On 17/4/24 21:37, Ian Trudel wrote:

This is an amazingly insightful reply. Thank you! My current workaround is to use |m[mname]| but it's not without its flaws. It requires the receiver object to be passed on as the first parameter, i.e. |object.mnmane|, otherwise it will send it to self. In the following code example, you will notice that the output is |Child::print()| instead of |Parent::print()|, which is not a typical behaviour for OOP. It would most likely require to call |o.__mmname| in the run method of the Parent class.

Output:

|Anonymous::print() Child::print() | |class Parent(objects) method add(o) put(objects, o) end method print() write(classname(self) || "::print()") end method run() local mname every o := !_objects do { cname := classname(o) every m := !methodnames(o) do { m ? { tab(upto(cname || "")) & move(*cname + 1) mname := tab(0) } o.mmname } } end initially objects := [] end class Child : Parent() method print() self.Parent.print() end end class Anonymous() method print() write(classname(self) || "::print()") end end procedure main() p := Parent() a := Anonymous() c := Child() p.add(a) p.add(c) p.run() end |

— Reply to this email directly, view it on GitHub https://github.com/uniconproject/unicon/issues/422#issuecomment-2061056158, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAUUHFKSU7MMIKPSQRVYGBTY5ZNF5AVCNFSM6AAAAABGGMZF4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRRGA2TMMJVHA. You are receiving this because you are subscribed to this thread.Message ID: @.***>

StephenWampler commented 1 month ago

Here's a potential replacement for the invoke() method in uni/lib/object.icn. It avoids the problem of mapping between internal/external/simple method names. I've only tested it a little, so caveat emptor!

method invoke(s, A[]) local mname

  every image(!self.__m) ? {
     mname := (="procedure ", tab(0)) | next
     if mname ? (tab(-(*s+1),="_",match(s)) then
           suspend mname ! ([self]|||A) | fail
     }

end

Jafaral commented 1 month ago

Oops. Typo. The change would be to uni/lib/object.icn, not uni/lib/class.icn. Sorry!

You can edit a previous post by pressing the three dots at the top right corner of post.

IanTrudel commented 4 weeks ago

RE: upto() vs find()

Thank you for the tip!

RE: methodnames() only listing the class methods (no parent).

I believe this is acceptable considering that Unicon implements multiple inheritance. One could end up with several methods with the same name. The way one would list the parent's methods would perhaps be something like methodnames(classname(self.Parent)).

RE: alternative to __m invocation.

I tried something like that but couldn't get it to work. Calling mname(o) and the likes is certainly less convoluted.

RE: self in Child vs self in Parent

Smalltalk has defined the behaviour of self to be bound by the class it is used in. In Child, self should be the current instance of Child, and, in Parent, self should be the current instance of Parent. For some reason, Unicon seems to flatten the concept of self where there is no class hierarchy. This is the point I was illustrating in the code example.

RE: classobject.icn, errorsystem.icn, utf8.icn

@brucerennie this is how I figured out how to properly use __m. I would still be stuck without your work.

IanTrudel commented 4 weeks ago

I have been writing a unit test framework based on Kent Beck's original SUnit. The idea is to fully embrace Unicon OOP and also better integrate with your CI, where failing tests would result in failed build — then you don't get to wait years before someone shows up with issues like those in objecttests.

This is work in progress. I am working on assertions and then a TestReporter. The TestReporter will be a class on its own because someone may someday want to integrate it to the Unicon IDE, they can write their own TestReporter accordingly.

Hope this clarifies a few things.

link ximage

import lang

invocable all

class TestCase : Object()
   method setupClass()
      write(classname(self), "::setupClass()")
   end

   method teardownClass()
      write(classname(self), "::teardownClass()")
   end

   method setup()
      write(classname(self), "::setup()")
   end

   method teardown()
      write(classname(self), "::teardown()")
   end

   method testNoTest()
      write("shouldn't be here")
   end

   method assertEqual(n, m)
      local retval

      retval := success

      write("assertEqual")

      # are n, m strings? integers? reals?
      # case type(n) of {
      #  "string": { }
      #  "integer": { }
      # }
      if n ~= m then {
         write("Assertion failed: Expected ", m, ", but got ", n)
         retval := failure
      }

      return retval
   end

   method print()
      write("print has been called.")
   end
end

class TestSuite(__tests, __testReporter)
   method add(testCase)
      cname := classname(testCase)
      if (\cname & (not (cname == "TestCase"))) then {
         if \testCase.__m["Type"] then {
            every ctype := testCase.Type() do {
               if ctype == "TestCase" then {
                  put(__tests, testCase)
               }
            }
         }
      }
   end

   method run()
      local passed, failed, methodName

      passed := 0
      failed := 0

      every test := !__tests do {
         cname := classname(test)

         test.setupClass()
         every m := !methodnames(test) do {
            m ? {
               tab(find(cname || "_")) & move(*cname + 1)
               methodName := tab(0)
            }
            if methodName ? match("test") then {
               write("\t" || classname(test) || "::" || methodName)
               test.setup()
               test.__m[methodName](test)
               test.teardown()
            }
         }
         test.teardownClass()
      }

      self.output()
   end

   method output()
      write(ximage(__tests))
   end

   method print()
      write("print from TestSuite.")
   end

initially
   __tests := []
   /__testReporter := "ReporterClass"
end

class NoClass()
end

class NormalTest : TestCase()
   method this_is_not_a_test()
      write("shouldn't happen.")
   end

   method testSomething()
      write("100101010010")
   end

   method test_me_one_time()
      assertEqual(2, 5)
   end

   method testOneMore()
      #assertEqual("a", "b")
   end

   method testNormal()
      write("o_O")
   end
end

class AnotherTest : TestCase()
   method setupClass()
      write("AnotherTest class setup.")
   end

   method setup()
      write("Another test, another day. Setup.")
   end

   method testAnother()
      write("Running another test.")
   end

   method teardown()
      write("Another test, another day. Teardown.")
   end

   method teardownClass()
      write("AnotherTest class teardown.")
   end
end

procedure main()
   ts := TestSuite()
   tc := TestCase()
   nc := NoClass()
   nt := NormalTest()
   at := AnotherTest()
   ts.add(&null)
   ts.add("hello")
   ts.add(tc)
   ts.add("o_O")
   ts.add(nc)
   ts.add(nt)
   ts.add(at)
   ts.run()
end
Jafaral commented 4 weeks ago

@IanTrudel tests are very welcome, thank you for doing the work.

FYI, we do have tests run as part of the CI (see Test here). We are close to changing that so that any failed test would fail the CI. We are in the process of fixing the remaining failing tests or at least isolating them.

IanTrudel commented 4 weeks ago

@IanTrudel tests are very welcome, thank you for doing the work.

My pleasure! tests/unicon/tester.icn is very good but it's rather minimalist. With a bit of guidance from you guys, I can come up with a framework that offers convenience, high cohesion and extensibility.

FYI, we do have tests run as part of the CI (see Test here). We are close to changing that so that any failed test would fail the CI. We are in the process of fixing the remaining failing tests or at least isolating them.

Great work on the CI, by the way! I was surprised to see support for so many targets.

IanTrudel commented 4 weeks ago

Here's a potential replacement for the invoke() method in uni/lib/object.icn. It avoids the problem of mapping between internal/external/simple method names. I've only tested it a little, so caveat emptor!

method invoke(s, A[]) local mname

  every image(!self.__m) ? {
     mname := (="procedure ", tab(0)) | next
     if mname ? (tab(-(*s+1),="_",match(s)) then
           suspend mname ! ([self]|||A) | fail
     }

end

@StephenWampler Your solution turned out pretty well. It's clean and straightforward.

      every test := !__tests do {
         cname := classname(test)

         test.setupClass()
         every image(!test.__m) ? {
            mname := (="procedure ", tab(0)) | next
            if mname ? (tab((*cname+1)), ="_", match("test")) then {
               test.setup()
               mname(test)
               test.teardown()
            }
         }
         test.teardownClass()
      }
Jafaral commented 3 weeks ago

@StephenWampler, wanna go after some of the issues you identified? We should attempt to fix those. If they break any existing applications we can discuss.

StephenWampler commented 3 weeks ago

Invoke should be fixed with issue #441. Also, despite multiple inheritance, only one of any duplicated method is ever visible to the subclass. There's a fix in #441 so all of the visible methods available to a subclass are produced by genMethods(). objecttests.icn now produces the expected output. Note that the function (i.e. "builtin") methodnames is still broken. My C is so far in the past the I'm absolutely the wrong person to try and fix it. I know. I looked. It's pretty much greek to me...

StephenWampler commented 1 week ago

I'm going to close this issue since invoke() now works as expected. I'm also going to open a new issue on the fact that methodnames() doesn't generate inherited methods visible to the subclass.