bugGNU Octave - Bugs: bug #59602, incorrect "not a direct...

 
 

bug #59602: incorrect "not a direct superclass" error

Submitter:  Ray Zimmerman <rdzman>
Submitted:  Wed 02 Dec 2020 08:31:28 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  7 - High Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 6.1.0 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 07 Dec 2020 06:54:25 PM UTC, comment #9: 

Alright, I pushed the changeset to stable.  We will see if any other programmers report an error.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Mon 07 Dec 2020 04:40:47 PM UTC, comment #8: 

Thanks. Unfortunately, I'm not currently set up to build my own Octave from source, so I can't try it out yet on my own code.

FWIW, I have since encountered what I believe is the same problem in Octave 5.2.0 as well, but in my experience it is less common than with 6.x.

Ray Zimmerman <rdzman>
Thu 03 Dec 2020 11:55:55 PM UTC, comment #7: 

Attached is a cset which implements what I suggested (call lookup_class only with a string argument) and it fixes the problem in this report.  I ran "make check" and there are no errors so it is probably safe.

(file #50396)

Rik <rik5>
Group administrator
Thu 03 Dec 2020 08:08:15 PM UTC, comment #6: 

This is really ugly, but I know a bit more about what is happening.  The root cause is that two different calls to lookup_class are returning different values when they should be returning the same thing.

The function lookup_class is overloaded and there are three versions.


  cdef_class
  lookup_class (const std::string& name, bool error_if_not_found,
                bool load_if_not_found)
  {
    cdef_manager& cdm = __get_cdef_manager__ ("lookup_class");

    return cdm.find_class (name, error_if_not_found, load_if_not_found);
  }

  cdef_class
  lookup_class (const cdef_class& cls)
  {
    // FIXME: placeholder for the time being, the purpose
    //        is to centralized any class update activity here.

    return cls;
  }

  cdef_class
  lookup_class (const octave_value& ov)
  {
    if (ov.is_string())
      return lookup_class (ov.string_value ());
    else
      {
        cdef_class cls (to_cdef (ov));

        return lookup_class (cls);
      }

    return cdef_class ();
  }


The first call to lookup_class uses a string as the argument.  The second call, however, does not.  The code first gets a Cell array by querying the SuperClasses field.  The individual entries of the Cell array are not the string names of the superclasses, but rather meta.class objects wrapped inside an octave_value.  This calls the third version of lookup_class where the input is an octave_value.

The third version determines that the octave_value is not a string (ov.is_string()) and therefore attempts a direct conversion from octave_value to cdef_object.  The code for that is


  cdef_object
  to_cdef (const octave_value& val)
  {
    if (val.type_name () != "object")
      error ("cannot convert '%s' into 'object'", val.type_name().c_str ());

    return dynamic_cast<octave_classdef *> (val.internal_rep ())->get_object ();
  }


This looks problematic.  Octave is trying to dynamically convert a meta.class object into a classB object and my guess is that the conversion doesn't work perfectly.

When execution returns to the third version of lookup_class it creates a new class from the object returned from to_cdef.  For whatever reason, the cdef_class constructor is then creating and installing a new class with cdef_manager such that they have different memory addresses.


        cdef_class cls (to_cdef (ov));


Long term, I think we need to untangle this code because it is so complex I can't guarantee there aren't many other subtle problems here.

In the short term, for this bug report, it would probably be enough to change is_superclass() to call lookup_class (std::string).  In order to do that, we would need to get the Name field from the meta.class object wrapped inside an octave_value.  I'm not sure how to do that, but it should be more straightforward than dynamic_casting.

Rik <rik5>
Group administrator
Thu 03 Dec 2020 06:19:54 PM UTC, comment #5: 

Marking as confirmed, and noting that this is a regression and hence of more importance than the average bug.  I'm adding jwe to the CC list as he may have some ideas about what is going on.

I have done a bunch of line-by-line debuggging with gdb and I've somewhat localized the problem, but it has been very tedious as the classdef code is not simple and not my area of expertise.

The error "'classB' is not a direct superclass of 'classA'" originates in ov-classdef.cc in this function which I quote.


octave_value_list
octave_classdef_superclass_ref::execute (octave::tree_evaluator& tw,
                                         int nargout,
                                         const octave_value_list& idx)
{
  octave_value_list retval;

  std::string meth_name;
  bool in_constructor;
  octave::cdef_class ctx;

  ctx = octave::get_class_context (meth_name, in_constructor);

  if (! ctx.ok ())
    error ("superclass calls can only occur in methods or constructors");

  std::string mname = m_method_name;
  std::string cname = m_class_name;

  // CLS is the superclass.  The lookup_class function handles
  // pkg.class names.

  octave::cdef_class cls = octave::lookup_class (cname);

  if (in_constructor)
    {
      if (! is_direct_superclass (cls, ctx))
        error ("'%s' is not a direct superclass of '%s'",
               cname.c_str (), ctx.get_name ().c_str ());


When I check the values of meth_name ("classA") and use ctx.get_name() ("classA") I can see that Octave is executing the constructor for classA.

If I check the values of m_method_name ("obj") and m_class_name ("classB") it is clear that the interpreter has reached the line


obj@classB();


The next bit of code finds the specified superclass


  octave::cdef_class cls = octave::lookup_class (cname);


I verified with cls.get_name() that this is "classB".  Finally, Octave goes to verify the superclass relationship with


      if (! is_direct_superclass (cls, ctx))


The next set of functions are in the file cdef-utils.cc.  The first function is


  bool
  is_direct_superclass (const cdef_class& clsa, const cdef_class& clsb)
  {
    return is_superclass (clsa, clsb, false, 1);
  }


and the function is_superclass is defined as


  bool
  is_superclass (const cdef_class& clsa, const cdef_class& clsb,
                 bool allow_equal, int max_depth)
  {
    bool retval = false;

    if (allow_equal && clsa == clsb)
      retval = true;
    else if (max_depth != 0)
      {
        Cell c = clsb.get ("SuperClasses").cell_value ();

        for (int i = 0; ! retval && i < c.numel (); i++)
          {
            cdef_class cls = lookup_class (c(i));

            retval = is_superclass (clsa, cls, true,
                                    max_depth < 0 ? max_depth : max_depth-1);
          }
      }

    return retval;
  }


What is supposed to happen is that is_direct_superclass should call is_superclass with a depth of 1 so that only direct parent classes are found.

The first time in to the function is_superclass allow_equal is false so execution goes to the else branch where it gets the list of all SuperClasses in a cell array of strings.  It then iterates over each SuperClass by recursively calling is_superclass().  I verified that there is only one SuperClass and using cls.get_name() that its name is "classB".

When the code works, the cdef_class variable from the lookup() call in ov-classdef.cc and the cdef_class variable from the lookup() call in cdef-utils.cc are the same.  They are the same as determined by the operator overload "==" for cdef_class objects which compares the internal rep pointers.

When it fails, the two cdef_class objects superficially look the same---I called obj.get_name() and they both return "classB"---but the rep pointers are to different areas of memory.

The function prototype for lookup_class is


  extern cdef_class
  lookup_class (const std::string& name, bool error_if_not_found = true,
                bool load_if_not_found = true);


For the moment, this is as far as I have been able to localize the problem.  If I had to make a stab in the dark, I think that having the property "a = @classB" might be parsed, but the new Octave behavior is to leave the function handle reference dangling until it is used, at which point a search of the symbol table is done and the correct function is run.  This "lazy" evaluation of function handles is done for Matlab compatibility, but it may be that this leaves the class or classdef object in a not fully constructed state.  I know a cdef_object has functions for is_constructed() and is_partially_constructed_for().  It is possible that the lookup_class() function is avoiding partially constructed classes and when it doesn't find classB, it creates a new instance in the class_manager because load_if_not_found = true is the default.  This might also explain the observation that the minimum working example succeeds if you create an object of classB before creating an object of classA.

Rik <rik5>
Group administrator
Thu 03 Dec 2020 12:41:15 AM UTC, comment #4: 

While I can confirm that initializing the property in the constructor rather than the properties section does work in this MWE, it does not resolve the issue in my actual code (which runs fine in Matlab and previous versions of Octave). It still give the "not a direct superclass" error.

Even eliminating the function handle and assigning a char array instead to the property (either in properties or the constructor) does not prevent the error in my actual code.

Ray Zimmerman <rdzman>
Wed 02 Dec 2020 10:18:48 PM UTC, comment #3: 

I'm guessing that this is related to function handles, lazy evaluation, and having the class constructor for the superclass as a function handle in the properties section.

As a workaround, can you try declaring property 'a' in the properties block, but initializing it in the constructor of classA?  This worked for me.

classA.m


classdef classA < classB
    properties
        a
    end
    methods
        function obj = classA()
            disp ("in ClassA() constructor");
            obj@classB();
            obj.a = @classB;
        end
    end
end


classB.m

classdef classB < handle
  methods
    function obj = classB ()
      disp ("in ClassB() constructor");
    end
  end
end


Finally, start a new Octave session and


octave:1> x = classA
in ClassA() constructor
in ClassB() constructor
x =

  classA object with properties:

      a: [1x1 function_handle]

octave:2> x.a
ans = @classB



Rik <rik5>
Group administrator
Wed 02 Dec 2020 09:36:21 PM UTC, comment #2: 

While the behavior in the MWE is reproducible, in the more complex context of my own code, I have not yet been able to find a reliable workaround. So for now, I'm restricted to 5.2.0 and earlier. :-(

Ray Zimmerman <rdzman>
Wed 02 Dec 2020 09:02:33 PM UTC, comment #1: 

FWIW, I was able to duplicate the behavior on 6.1.0 on Windows as well.

Ray Zimmerman <rdzman>
Wed 02 Dec 2020 08:31:28 PM UTC, original submission:  

Given the following two classes ...


classdef classA < classB
    properties
        a = @classB;
    end
    methods
        function obj = classA()
            obj@classB();
        end
    end
end



classdef classB < handle
end


... attempting to instantiate classA in a fresh Octave session results in the following error:


octave:1> a = classA
error: 'classB' is not a direct superclass of 'classA'
error: called from
    classA at line 7 column 13


While it produces this error consistently in a fresh Octave session, the code does sometimes work fine, depending on previous activity in the Octave session. For example, the following is also repeatable for me in a fresh Octave session:


octave:1> b = @classB
b = @classB
octave:2> a = classA
a =

  classA object with properties:

      a: [1x1 function_handle]


In my tests on previous versions (e.g. 5.2.0, 5.1.0, 4.4.1) I was not able to reproduce the error. In 6.0.09 the error seems to be related to the initialization of the property a in classA. Initializing it with a different value also eliminates the error.

Very strange ... this bug was causing nondeterministic results from the test suite for my own code, and it took me hours to boil it down to this MWE.

Ray Zimmerman <rdzman>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #50396:  bug59602.cset added by rik5 (1KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by rdzman (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-12-07 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2020-12-03 rik5 Attached File- Added bug59602.cset, #50396
        StatusConfirmed Patch Submitted
    2020-12-03 rik5 Priority5 - Normal 7 - High
        Item GroupUnexpected Error or Warning Regression
        StatusNeed Info Confirmed
        Carbon-Copy- Added jwe
    2020-12-02 rik5 StatusNone Need Info
        Release6.0.90 6.1.0
        Operating SystemMac OS Any

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code