bugGNU Octave - Bugs: bug #58953, Error with 'end' in index...

 
 

bug #58953: Error with 'end' in index expression with nested function calls

Submitter:  Fernando <tutissanalio>
Submitted:  Fri 14 Aug 2020 02:52:27 PM UTC
   
 
Category:  Interpreter Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 6.0.90 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 08 Oct 2020 07:55:00 AM UTC, comment #68: 

Great. Thank you, jwe.

Fernando <tutissanalio>
Mon 05 Oct 2020 07:37:02 PM UTC, comment #67: 

Fernando: I applied your changes from comment #66 with a few minor style fixes on stable and merged with default.  Marking this bug as fixed and closing the report.  If there are other issues, please open a new bug report.

John W. Eaton <jwe>
Group administrator
Thu 01 Oct 2020 05:18:52 PM UTC, comment #66: 

Here is a patch with proposed changes for the tests. It's more code than I would have liked. Basically, the changes are:

- Add tests for END in subsasgn expressions (previous tests only considered subsref expressions).
- Do not call subsref or subsasgn with an empty index structure. I noticed that this is not valid in Matlab.

(file #49901)

Fernando <tutissanalio>
Thu 01 Oct 2020 09:12:38 AM UTC, comment #65: 

I would like to propose a minor change for the tests. I will send it later today.

Fernando <tutissanalio>
Thu 01 Oct 2020 03:06:07 AM UTC, comment #64: 

Is this ready to close now?

Rik <rik5>
Group administrator
Mon 28 Sep 2020 04:34:21 PM UTC, comment #63: 

OK, I pushed the change on stable and merged with default.  I also pushed a second change that removes the has_magic_end and includes_magic_end functions from the parse tree classes since they are not needed now:

http://hg.savannah.gnu.org/hgweb/octave/rev/3719f5d452d4
http://hg.savannah.gnu.org/hgweb/octave/rev/2219b0ed8754

Marking as ready for test.

John W. Eaton <jwe>
Group administrator
Mon 28 Sep 2020 04:00:10 PM UTC, comment #62: 

I think stable is fine as well.

Rik <rik5>
Group administrator
Mon 28 Sep 2020 12:32:10 PM UTC, comment #61: 

If I remember correctly, the dispatch_class is set so that function handles defined in class methods will work properly.  But that may be left over from before my recent refactoring of the function handle implementation.  If it is not needed here, then we should remove it.  I suppose we can do that now and see whether any problems show up.

What should we do with this patch?  My preference is to apply it to stable.

John W. Eaton <jwe>
Group administrator
Thu 24 Sep 2020 05:02:33 PM UTC, comment #60: 

jwe, I wonder why it is necessary to call the method tree_evaluator::set_dispatch_class in tree_evaluator::evaluate_end_expression, just before calling a user-defined "end" method. The fragment of code is:


        if (meth.is_defined ())
          {
            unwind_action
              act ([this] (const std::string& cls)
                   {
                     set_dispatch_class (cls);
                   }, get_dispatch_class ());

            set_dispatch_class (dispatch_class);

            return m_interpreter.feval
              (meth, ovl (expr_result, m_index_position+1, m_num_indices), 1);
          }


Everything seems to work fine if I remove the call to set_dispatch_class.

Fernando <tutissanalio>
Wed 23 Sep 2020 05:12:13 PM UTC, comment #59: 

jwe, I tried the patch and it works perfectly. Thank you.

Fernando <tutissanalio>
Wed 23 Sep 2020 12:52:26 PM UTC, comment #58: 

I will see about fixing the warning.

I executed the command


/bin/bash run-octave -g --norc --silent --no-history --path ../src/doc/interpreter --eval "genpropdoc ('root');"


on the stable-gcc-lto build and generated the following stack trace:


Thread 1 "octave-cli" received signal SIGSEGV, Segmentation fault.
0x00007ffff7ad0e72 in octave::gtk_manager::unload_all_toolkits() ()
   from /scratch/buildbot/workers/jwe-debian-x86_64-2/stable-gcc-lto-debian/build/libinterp/.libs/liboctinterp.so.8
(gdb) where
#0  0x00007ffff7ad0e72 in octave::gtk_manager::unload_all_toolkits() ()
   from /scratch/buildbot/workers/jwe-debian-x86_64-2/stable-gcc-lto-debian/build/libinterp/.libs/liboctinterp.so.8
#1  0x00007ffff7aeb7ac in octave::interpreter::shutdown() ()
   from /scratch/buildbot/workers/jwe-debian-x86_64-2/stable-gcc-lto-debian/build/libinterp/.libs/liboctinterp.so.8
#2  0x00007ffff72db8ca in octave::cli_application::execute() ()
   from /scratch/buildbot/workers/jwe-debian-x86_64-2/stable-gcc-lto-debian/build/libinterp/.libs/liboctinterp.so.8
#3  0x00005555555553ad in main ()


I'm building with debugging enabled to see whether I can get better info.

John W. Eaton <jwe>
Group administrator
Wed 23 Sep 2020 12:12:36 PM UTC, comment #57: 

clang issues a warning during compilation. It is probably unrelated. But nevertheless:

In file included from ../src/libinterp/corefcn/interpreter.cc:36:
In file included from ../src/liboctave/util/cmd-edit.h:36:
In file included from ../src/liboctave/util/str-vec.h:35:
In file included from ../src/liboctave/array/Array.h:39:
In file included from ../src/liboctave/array/idx-vector.h:36:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/memory:84:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr.h:52:
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:632:8: warning: delete called on non-final 'octave::push_parser' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
              delete __p;
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:639:4: note: in instantiation of function template specialization 'std::__shared_count<__gnu_cxx::_S_atomic>::__shared_count<octave::push_parser *>' requested here
        : __shared_count(__p)
          ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:1128:17: note: in instantiation of function template specialization 'std::__shared_count<__gnu_cxx::_S_atomic>::__shared_count<octave::push_parser *>' requested here
        : _M_ptr(__p), _M_refcount(__p, typename is_array<_Tp>::type())
                       ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr.h:159:25: note: in instantiation of function template specialization 'std::__shared_ptr<octave::base_parser, __gnu_cxx::_S_atomic>::__shared_ptr<octave::push_parser, void>' requested here
        shared_ptr(_Yp* __p) : __shared_ptr<_Tp>(__p) { }
                               ^
../src/libinterp/corefcn/interpreter.cc:1239:27: note: in instantiation of function template specialization 'std::shared_ptr<octave::base_parser>::shared_ptr<octave::push_parser, void>' requested here
            repl_parser = std::shared_ptr<base_parser> (pp);
                          ^


Markus Mützel <mmuetzel>
Group administrator
Wed 23 Sep 2020 06:00:11 AM UTC, comment #56: 

Since hg id 2893db10b8d2 some of the build bots are segfaulting while building the manual. E.g.: http://buildbot.octave.org:8010/#/builders/32/builds/74/steps/6/logs/stdio on default
http://buildbot.octave.org:8010/#/builders/36/builds/39/steps/6/logs/stdio on stable

Looks like the crash is 100% reproducible on affected systems.

That's a blocking issue imho. Should I open a separate report?

Markus Mützel <mmuetzel>
Group administrator
Wed 23 Sep 2020 02:47:14 AM UTC, comment #55: 

Try the attached revision of the patch.  It relies on this changeset as well, that I have pushed to stable:

http://hg.savannah.gnu.org/hgweb/octave/rev/2893db10b8d2

(file #49846)

John W. Eaton <jwe>
Group administrator
Wed 23 Sep 2020 12:49:11 AM UTC, comment #54: 

I think the issue is about the Octave implementation, and whether the proposed change (calling tree_evaluator::evaluate_end_expression in tree_identifier::evaluate_n when the identifier is "end") is correct in all situations.

I understand your concerns, jwe.

As you know, the reason for calling tree_evaluator::evaluate_end_expression directly, instead of doing it indirectly by calling the builtin "end" function, is because "end" might need to evaluate an expresion like obj.X, where obj is an object from a class that redefines the subsref method. In that case, Octave needs to know whether that happens in the context of a method from the class of obj or not, to call the suitable subsref (as pointed out by Guillaume). In order to know the class context, octave obtains the class of the current function, but that function should not be the "end" function, but the function where the "end" expression happens.

Is that correct in all cases? I think it is, but I am not 100% sure. At the point where tree_evaluator::evaluate_end_expression is called in tree_identifier::evaluate_n, the unpatched code would call the function "end" without arguments. If that function has not been redefined, Octave would call octave_builtin::execute, which would call the builtin "end", which would call tree_evaluator::evaluate_end_expression. Ok, octave_builtin::execute also sets up the profiler and does some processing with the returned values. I don't know if that is important. On the other hand, the "end" function could have been redefined, but I don't think it is wise to redefine the general "end" function. I tried it in Matlab and it behaves unpredictably.

Anyway, there is another possibility that you might feel more comfortable with. It would be something like (in tree_identifier::evaluate_n):


if (name () == "end")
  obtain the arguments to be passed to the end function
else
  args = empty list

retval = fcn->call (tw, nargout,args);


Fernando <tutissanalio>
Tue 22 Sep 2020 08:55:29 PM UTC, comment #53: 

Fernando seems to be in a better position than me to help here but is the remaining issue about clarifying how Matlab handles "end" in all circumstances or is it about the corresponding Octave implementation or both?

Guillaume <gyom>
Mon 21 Sep 2020 06:21:48 PM UTC, comment #52: 

In any case, I attach two patches.

The first is the latest version of the end refactoring patch that includes the new tests.

The second eliminates the has_magic_end and includes_magic_end functions from the parse tree classes as they should no longer be needed.

(file #49832, file #49833)

John W. Eaton <jwe>
Group administrator
Mon 21 Sep 2020 06:17:12 PM UTC, comment #51: 

Thanks for the additional patch and tests.  I also see that it now appears to work, but I'm having trouble understanding why it is appropriate to call tree_evaluator::evaluate_end_expression in tree_identifier::evaluate_n when the identifier is "end".  Is that always true?  Why?  Are there any other cases that could result in this code path but not be the correct thing to do?  I'm afraid that without understanding or being explicit here about what it happening, that some future change will again result in introducing a bug or at least additional confusion.

John W. Eaton <jwe>
Group administrator
Sun 20 Sep 2020 11:51:51 PM UTC, comment #50: 

I attach some tests for this bug.

(file #49828)

Fernando <tutissanalio>
Sun 20 Sep 2020 09:34:21 AM UTC, comment #49: 

Thanks Fernando, I also applied your patch and the examples from comment #45 now work fine (as well as bug #55856). And, so far, I don't see any side effect so it looks all good to me. Thank you both.

Guillaume <gyom>
Sat 19 Sep 2020 10:48:21 PM UTC, comment #48: 

Oops! I forgot the patch. Here it is.

(file #49823)

Fernando <tutissanalio>
Sat 19 Sep 2020 10:34:58 PM UTC, comment #47: 

Great! Thank you Guillaume.

I think I have it. I attach a patch to be applied on top of the patch sent by jwe from comment #40.

Guillaume, can you confirm that it works for your example? I tried it with myclass4 and myclass5 and it works.

I also tested it with this class:


lassdef myclass6 < handle
  properties
    data_
  end
  methods
    function obj = myclass6(data)
      obj.data_ = data;
    end
    function r = subsref(obj,S)
      fprintf('-----------------\n')
      S.type
      S.subs
      fprintf('-----------------\n')
      if strcmp(S(1).type,'.') && strcmp(S(1).subs,'data')
        % Transform: obj.data --> obj.data_
        S(1).subs='data_';
      elseif strcmp(S(1).type,'()')
        % Transform: obj(index) --> obj.data_(index)
        S = [struct('type','.','subs','data_'), S(1:end)];
      end
      r = builtin('subsref',obj,S);
    end
    function r = end(obj,k,n)
      fprintf('''end'' called with index=%d of %d\n',k,n);
      % We subtract 1 from the "real" end of obj.data_
      r = builtin('end',obj.data_,k,n)-1;
    end
  end
end


The tests were:


octave:6> obj.data(end)
-----------------
ans = .
ans = data
-----------------
-----------------
ans = .
ans = ()
ans = data
ans =
{
  [1,1] = 5
}
-----------------
ans = 5
octave:7> obj(end)
'end' called with index=1 of 1
-----------------
ans = ()
ans =
{
  [1,1] = 4
}
-----------------
ans = 4


The results are the same in Matlab.

Fernando <tutissanalio>
Sat 19 Sep 2020 10:45:55 AM UTC, comment #46: 
Guillaume <gyom>
Sat 19 Sep 2020 10:28:59 AM UTC, comment #45: 

jwe, I'm sorry I cannot be more helpful here. You seem to be very close to fixing a number of issues though.

I tried to simplify my example to the maximum and I notice the issue happens with old-style class but not classdef and the issue does not come up before applying the patch.


classdef myclass5
  properties
    data
  end
  methods
    function obj = myclass5
      obj.data = 1;
    end
    function r = subsref(obj,s)
      switch s(1).type
        case '.'
          r = obj.data(end);
      end
    end
  end
end



# @myclass4/myclass4.m
function this = myclass4

  this = class (struct ('data', 1), 'myclass4');

end

# @myclass4/subsref.m
function r = subsref (obj, s)

  switch (s(1).type)
    case '.'
      r = obj.data (end);
  end

end



octave> obj = myclass4(); obj.data
error: max_recursion_depth exceeded
error: called from
    subsref
    subsref at line 5 column 9
octave> obj = myclass5(); obj.data
ans = 1


This means that unpatched Octave and Matlab use builtin when evaluating obj.data in subsref whether there is an 'end' indexing or not. I am trying to find in the Matlab documentation a description of when subsref is called from within methods but couldn't so far.

Guillaume <gyom>
Sat 19 Sep 2020 09:57:23 AM UTC, comment #44: 

Don't worry John, we will work it out with a bit of patience.

Fernando <tutissanalio>
Sat 19 Sep 2020 05:43:57 AM UTC, comment #43: 

I think at this point I just don't understand how the subsref and subsasgn methods are supposed to work.  I'm beginning to wonder whether something is fundamentally wrong with the way function overloading and dispatch is currently implemented for variable indexing and indexed assignment (if not other operator and function dispatching).

John W. Eaton <jwe>
Group administrator
Fri 18 Sep 2020 04:44:42 PM UTC, comment #42: 

I still see a problem with the latest patch - perhaps the same issue than the one reported by Fernando in comment 41. I attach a small class to reproduce the issue: check(myclass4).

(file #49810)

Guillaume <gyom>
Fri 18 Sep 2020 04:16:52 PM UTC, comment #41: 

We are very close. You have solved two problems:

- Incorrect dispatch for 'end' in cases like obj.data(end).
- Builtin 'end' ignores arguments when called from user code

There is still a third problem, which I had not provided a test for

- Incorrect dispatch for 'subsref' when used to obtain the object on which 'end' is to be applied.

To show the third problem, I defined the following class, where subsref transforms obj.data to obj.data_


classdef myclass3 < handle
  properties
    data_
  end
  methods
    function obj = myclass3(data)
      obj.data_ = data;
    end
    function r = subsref(obj,S)
      fprintf('-----------------\n')
      S.type
      S.subs
      fprintf('-----------------\n')
      if strcmp(S(1).type,'.') && strcmp(S(1).subs,'data')
        S(1).subs='data_';
      end
      r = builtin('subsref',obj,S);
    end
    function r = end(obj,k,n)
      fprintf('''end'' called with index=%d of %d\n',k,n);
      r = builtin('end',obj.data,k,n)-1;
    end
  end
end


then in Matlab:


>> obj=myclass3(1:5)
obj =
  myclass3 with properties:

    data_: [1 2 3 4 5]
>> obj.data(end)
-----------------
ans =
    '.'
ans =
    'data'
-----------------
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'data'
ans =
  1×1 cell array
    {[5]}
-----------------
ans =
     5


and in Octave:


octave:33> obj.data(end)
error: subsref: unknown method or property: data


What is happening is that in order to evaluate 'end', we must first evaluate 'obj.data', which should be done using the custom subsref, not the builtin subsref.

Sure, I can provide help writing some tests for problems like those three.

Fernando <tutissanalio>
Fri 18 Sep 2020 02:39:39 PM UTC, comment #40: 

Thanks for testing.

I'm attaching a new patch file.  Please try it instead of the earlier version.  I think it fixes the issue with calling end with arguments from builtin.  And with dispatching to subsref correctly when evaluating end in cases like obj.data(end).

Guillaume, if there is still a problem, then yes, please submit a test case.

I'm running out of energy to do much more work on this issue, so I hope we are about done with it.

If this feature is working correctly now, then it would be great to have some tests that could be used in the test suite.  Any help with that would be most appreciated.



(file #49808)

John W. Eaton <jwe>
Group administrator
Fri 18 Sep 2020 12:03:20 PM UTC, comment #39: 

With the patch, I also observe an infinite recursion in a private method (old-style class) at a line like this:


obj(1).data{i}.attr(2:end)


This class has a custom 'subsref' (but not 'end') which calls builtin subsref. I can try to write a small example if it ends up being a different issue than comment #38.

Guillaume <gyom>
Fri 18 Sep 2020 08:00:21 AM UTC, comment #38: 

It seems that you are almost there. However, there is some confusion between builtin and custom methods for subsref and END.

I defined yet another class, which redefines both subsref and END methods:


classdef myclass2 < handle
  properties
    data
  end
  methods
    function obj = myclass2(data)
      obj.data = data;
    end
    function r = subsref(obj,S)
      fprintf('-----------------\n')
      S.type
      S.subs
      fprintf('-----------------\n')
      % You can use () directly on obj to access data property
      if strcmp(S(1).type,'()')
        S = [struct('type','.','subs','data'), S(1:end)];
      end
      r = builtin('subsref',obj,S);
    end
    function r = end(obj,k,n)
      fprintf('''end'' called with index=%d of %d\n',k,n);
      r = builtin('end',obj.data,k,n)-1;
    end
  end
end


In Matlab, I got:


>> obj=myclass2(1:5)
obj =
  myclass2 with properties:
    data: [1 2 3 4 5]
>> a=obj.data(end)
-----------------
ans =
    '.'
ans =
    'data'
-----------------
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'data'
ans =
  1×1 cell array
    {[5]}
-----------------
a =
     5
>> b=obj(end)
'end' called with index=1 of 1
-----------------
ans =
    '()'
ans =
  1×1 cell array
    {[4]}
-----------------
b =
     4


but in Octave I got:


octave:1> obj=myclass2(1:5)
obj =
  myclass2 object with properties:
      data: [1x5 double]
octave:2> a=obj.data(end)
'end' called with index=1 of 1
-----------------
ans = .
ans = ()
ans = data
ans =
{
  [1,1] = 4
}
-----------------
a = 4
octave:3> b=obj(end)
'end' called with index=1 of 1
-----------------
ans = ()
ans =
{
  [1,1] = 0
}
-----------------
error: index (0): subscripts must be either integers 1 to (2^63)-1 or logicals
error: called from
    subsref at line 17 column 9


When evaluating the instruction a=obj.data(end), the END method should be evaluated for obj.data, which is an array and not an object, so the builtin END should be used.

When evaluating b=obj(end), the redefined END method calls the builtin END method for obj.data, but it seems that Octave's builtin END just ignores the arguments of that call.

Fernando <tutissanalio>
Fri 18 Sep 2020 02:20:43 AM UTC, comment #37: 

Can you try the attached patch?  I think it fixes this bug as well as bug #55856 and bug #59126.  The diffs are relative to stable, but I think they will also apply without conflicts on default.


(file #49806)

John W. Eaton <jwe>
Group administrator
Fri 18 Sep 2020 12:22:32 AM UTC, comment #36: 

I was experimenting with this bug when I found a related issue. I opened bug #59126 for it. Maybe they could be solved together.

Fernando <tutissanalio>
Wed 16 Sep 2020 04:06:20 PM UTC, comment #35: 

It sounds like the right approach.

Fernando <tutissanalio>
Wed 16 Sep 2020 03:40:34 PM UTC, comment #34: 

OK, thanks.  In Octave, we try to do the evaluation in advance, but that causes a lot of trouble and I see no reasonable way to distinguish


obj.data (variable (end))  ## end applies to variable
obj.data (function (end))  ## end applies to obj.data


But if we instead just cache the info for the expression we have processed so far (without evaluating it) and make that available to the "end" function, then I think it all falls out naturally in a compatible way.  So we would store the parse tree expression info for "obj.data", then when needing to compute the argument list for that, we can evaluate it, calling subsref if needed.  If the argument list also contains an expression, we perform this job again, recursively, so we just push another expression on the stack of cached info for "end" and pop it off when the argument list evaluation is complete.  But at that point, we are in the natural place to determine whether the indexed object is a variable (push new expression info) or a function (don't push).

I think we would also no longer have to store info about whether the argument list contains a "magic end" token, we just process it when it appears.

I'll see whether I can turn that into some code.  The same approach should also apply to bug #55856.

John W. Eaton <jwe>
Group administrator
Wed 16 Sep 2020 02:56:47 PM UTC, comment #33: 

I was about to send you what Octave does... Here is Matlab instead:


>> obj = myclass (rand (3, 4))
obj =
  myclass with properties:
    data: [3x4 double]
>> obj.data(end,end)
-----------------
ans =
    '.'
ans =
    'data'
-----------------
-----------------
ans =
    '.'
ans =
    'data'
-----------------
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'data'
ans =
  1x2 cell array
    {[1]}    {[3]}
-----------------
ans =
     1     1     1


subsref is indeed called twice while only once currently in Octave.

Guillaume <gyom>
Wed 16 Sep 2020 02:21:04 PM UTC, comment #32: 

Another question (not that it will be the last, I'd guess)...

With the class from comment #20, what happens for


obj = myclass (rand (3, 4))
obj.data(end,end)


Is subsref called for each time that "end" appears or just once?

John W. Eaton <jwe>
Group administrator
Mon 14 Sep 2020 07:27:01 PM UTC, comment #31: 

The more I look at this, the more complicated it appears to be, which leads me to think that I'm missing something that could make the implementation much simpler.  For example, what happens for expressions like


obj{idx}(end)
obj.a.b(end)
obj.a{idx}.b(end)


etc.  Are these (conceptually) equivalent to


obj{idx}(feval('end',obj{idx},1,1))
obj.a.b(feval('end',obj.a.b,1,1))
obj.a{idx}.b(feval('end',obj.a{idx}.b,1,1))


?  If I understand correctly, evaluation of the partial expression for the call to feval would result in one subsref call and the outer expression would be another, with all arguments passed to that subsref evaluation.

I think that is all easy to fix now, but the thing that escapes me is how to handle the following two cases differently:


obj.X(some_function(end))
obj.X(some_array(end))


I'm guessing there is some "natural" way of making this happen (especially if TMW is basically admitting that this "feature" is an accident of their implementation) but I'm not seeing it.

Given that any solution to this problem is likely to be disruptive, I think it is best to abandon any attempt to fix it on stable and instead try to fix for the next major release from the default branch.


John W. Eaton <jwe>
Group administrator
Sat 12 Sep 2020 12:24:24 PM UTC, comment #30: 

Summarizing, If I'm not wrong, we have that an expression like

obj.X(end)

where X can be either an object property or method, is equivalent to

obj.X(feval('end',obj.X,1,1))

thus, subsref is called twice.

On the other hand, the following two expressions:

obj.X(some_function(end))
obj.X(some_array(end))

are respectively equivalent to:

obj.X(some_function(feval('end',obj.X,1,1)))
obj.X(some_array(feval('end',some_array,1,1)))


Fernando <tutissanalio>
Sat 12 Sep 2020 12:06:38 PM UTC, comment #29: 

My previous message was truncated for some reason. The final part was:


>> obj=myclass(1:5)
obj =
  myclass with properties:
    data: [1 2 3 4 5]
>> obj.methodA
Not enough input arguments.
Error in myclass/methodA (line 10)
      r = val;
>> obj.methodA(end)
Not enough input arguments.
Error in myclass/methodA (line 10)
      r = val;



Fernando <tutissanalio>
Fri 11 Sep 2020 03:55:10 PM UTC, comment #28: 

More info. If I remove the subsref method for myclass.m, i.e. the default subsref method is used, then I have:


>> obj=myclass(1:5)
obj =
  myclass with properties:
    data: [1 2 3 4 5]
>> obj.methodA
Not enough input arguments.
Error in myclass/methodA (line 10)
      r = val;
>> obj.methodA(end)
Not enough input arguments.
Error in myclass/methodA (line 10)
      r = val;
+verbatim+

Fernando <tutissanalio>
Fri 11 Sep 2020 03:40:06 PM UTC, comment #27: 

Yes, that's it, we just call subsref for obj.X and END will apply to the result of that.

I changed the subsref method and executed what you asked, and yes, I see the error "methodA called without arguments!".

Fernando <tutissanalio>
Fri 11 Sep 2020 03:18:36 PM UTC, comment #26: 

OK, I think understand now.

So, when evaluating obj.X(end), we don't need to look up fields/properties of obj to check whether X is a property or method and then do different things depending on that result (because normally, END would not be valid in a call to a function).

Instead, we just call subsref for the obj.X part and END will apply to the result of that, whatever it is.  And that subsref evaluation may fail if, for example, it actually determines that X is a method that requires arguments.  If you change your subsref method to be


  function r = subsref(obj,S)
      fprintf('-----------------\n')
      S.type
      S.subs
      fprintf('-----------------\n')
if (numel (S) == 1 && strcmp (S(1).type, '.') && strcmp (S(1).subs, 'methodA'))
  error ('methodA called without arguments!');
end
      r = ones(1,3);


and then execute


obj=myclass(1:5)
obj.methodA (end)


do you see the error "methodA called without arguments!" or something else?

John W. Eaton <jwe>
Group administrator
Fri 11 Sep 2020 06:43:12 AM UTC, comment #25: 

In the second case, it's the same behaviour as for obj.data(end). In order to evaluate "end", we need to evaluate obj.data by calling subsref. The subsref method for this class always returns the array ones(3,1), so "end" evaluates to 3. After that, the complete subsref operation can be executed.

Fernando <tutissanalio>
Fri 11 Sep 2020 06:18:49 AM UTC, comment #24: 

Thanks.

The first one makes sense to me.  There is no "end" in the argument list, so subsref is called only once.

But for the second?  I see that subsref is called for obj.methodA, and it makes some sense to make that call to find out whether obj.methodA is a value or a function, but why does "end" get the value 3?  Where does that come from?

John W. Eaton <jwe>
Group administrator
Fri 11 Sep 2020 05:39:10 AM UTC, comment #23: 

Here is what Matlab R2020a gives:


>> obj=myclass(1:5)
obj =
  myclass with properties:
    data: [1 2 3 4 5]
>> obj.data(5)
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'data'
ans =
  1×1 cell array
    {[5]}
-----------------
ans =
     1     1     1
>> obj.methodA(end)
-----------------
ans =
    '.'
ans =
    'methodA'
-----------------
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'methodA'
ans =
  1×1 cell array
    {[3]}
-----------------
ans =
     1     1     1


Fernando <tutissanalio>
Fri 11 Sep 2020 01:38:09 AM UTC, comment #22: 

What happens for the following?  Is subsref called once or twice in this case?


obj = myclass (1:5);
obj.data (5)


Also, what happens for the following?  Is subsref called once or twice or is this an error before the subsref methdo is called?


obj = myclass (1:5);
obj.methodA (end)


With this info, I think I have a good chance of being able to make the behavior compatible, at least as far as I understand it.


John W. Eaton <jwe>
Group administrator
Thu 10 Sep 2020 11:41:34 PM UTC, comment #21: 

Ops, sorry, function "f" should be called "idem" in my previous comment (comment #20)

Fernando <tutissanalio>
Thu 10 Sep 2020 11:37:15 PM UTC, comment #20: 

I have done some tests in Matlab (2020a) to understand how expressions involving subsref and end are processed. Maybe it can help. However, it seems complex and I don't know if it is worth the effort to fix this issue.

So if you want to read on, I defined the following class


classdef myclass < handle
  properties
    data
  end
  methods
    function obj = myclass(data)
      obj.data = data;
    end
    function r = methodA (obj, val)
      r = val;
    end
    function r = subsref(obj,S)
      fprintf('-----------------\n')
      S.type
      S.subs
      fprintf('-----------------\n')
      r = ones(1,3);
    end
  end
end


and the following function


function r = f(x)
  r=x;
end


Then I executed:


>> obj=myclass(1:5)
obj =
  myclass with properties:

    data: [1 2 3 4 5]
>> obj.data(end)
-----------------
ans =
    '.'
ans =
    'data'
-----------------
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'data'
ans =
  1×1 cell array
    {[3]}
-----------------
ans =
     1     1     1


The subsref method is called twice. The first one evaluates obj.data, and its result is used to call the end method. The second one evaluates the complete expression: obj.data(3).

Another case:


>> x=1:5
x =
     1     2     3     4     5
>> obj.data(x(end))
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'data'
ans =
  1×1 cell array
    {[5]}
-----------------
ans =
     1     1     1


In this case only one invocation to subsref is necessary.

Another case:


>> obj.data(idem(end))
-----------------
ans =
    '.'
ans =
    'data'
-----------------
-----------------
ans =
    '.'
ans =
    '()'
ans =
    'data'
ans =
  1×1 cell array
    {[3]}
-----------------
ans =
     1     1     1


The expression looks very similar to the previous one, but now "end" refers to obj.data, so two subrefs are done.

It seems that Matlab tries to evaluate "idem(end)" before "obj.data", but finds an "end" that is refering to "obj.data" and evaluates "obj.data" on the fly.

Fernando <tutissanalio>
Wed 09 Sep 2020 09:40:30 PM UTC, comment #19: 

OK, thanks for the info.  So Octave gets that part right.

I'll see if I can arrange for the correct object to be set for END when executing an expression like "obj.data(minus(end,1))" and also find out why subsref is being called in some cases when performing indexed assignment (bug #55856).

John W. Eaton <jwe>
Group administrator
Wed 09 Sep 2020 08:29:52 PM UTC, comment #18: 

Matlab R2020a gives:

>> obj = myclass (13)

obj =

  myclass with properties:

    data: 13

>> obj.methodA (42)

ans =

    '.'


ans =

    '()'


ans =

    'methodA'


ans =

  1x1 cell array

    {[42]}

>> methodA (obj, 42)

ans =

    42


On a perhaps related matter, there is bug #55856

Guillaume <gyom>
Wed 09 Sep 2020 08:17:13 PM UTC, comment #17: 

BTW, if we are getting subsref fundamentally wrong for this case, then I would make this bug a blocker for the release.

John W. Eaton <jwe>
Group administrator
Wed 09 Sep 2020 08:15:32 PM UTC, comment #16: 

Oops, the class definition in comment #15 was supposed to be displayed inside a verbatim block:


classdef myclass
  properties
    data
  end
  methods
    function obj = myclass (data)
      obj.data = data;
    end
    function r = methodA (obj, val)
      r = val;
    end
    function r = subsref (obj, S)
      S.type
      S.subs
    end
  end
end


John W. Eaton <jwe>
Group administrator
Wed 09 Sep 2020 08:14:00 PM UTC, comment #15: 

OK, I see how this might be handled, but I'm now wondering whether we have a more basic error in the indexing logic for objects.

Given the following class:

+verbatim+
-verbatim-
classdef myclass
  properties
    data
  end
  methods
    function obj = myclass (data)
      obj.data = data;
    end
    function r = methodA (obj, val)
      r = val;
    end
    function r = subsref (obj, S)
      S.type
      S.subs
    end
  end
end
-verbatim-

What does Matlab display if you execute the following code?


obj = myclass (13)
obj.methodA (42)
methodA (obj, 42)


Is the user-defined subsref method involved in either of the calls to methodA?  Or does Matlab convert the obj.methodA(42) call to be methodA(obj,42) and skip calling subsref (because in that case, we are not indexing an object)?

In Octave, we are calling the user-defined subsref in the first case but not the second.  What does Matlab say is supposed to happen here?

John W. Eaton <jwe>
Group administrator
Wed 09 Sep 2020 03:15:06 PM UTC, comment #14: 

This solves the example in comment #0, but there is still an issue when evaluating an expression like:


object.data(func(expression_with_end))


For example, consider the following class (a modified version of the example class for bug #50716:


classdef myclass < handle

  properties
    data
  endproperties

  methods

    function obj = myclass(data)
      obj.data = data;
    endfunction

    function r = methodA (obj, val)
      r = val;
    endfunction

  endmethods

endclassdef


and execute the following sentences:


octave:1> obj=myclass(1:10)
obj =
  myclass object with properties:
      data: [1x10 double]
octave:2> obj.data(minus(end,1))
error: obj(0): subscripts must be either integers 1 to (2^63)-1 or logicals


The problem seems to be that "minus(end,1)" is evaluated before evaluating "obj.data", but should be evaluated after it in order to get the correct value for "end".

Fernando <tutissanalio>
Wed 09 Sep 2020 01:39:15 PM UTC, comment #13: 

The motivating example in comment #0 now works correctly.  Bug #50716 is already closed as fixed (and the examples in the comments seem to run for me).  Closing this report as fixed as well.

Rik <rik5>
Group administrator
Wed 09 Sep 2020 08:55:33 AM UTC, comment #12: 

A similar issue was discussed in a previous bug report:
https://savannah.gnu.org/bugs/?50716#comment9

Guillaume <gyom>
Wed 09 Sep 2020 03:58:36 AM UTC, comment #11: 

I pushed the following change on stable and merged with default:

http://hg.savannah.gnu.org/hgweb/octave/rev/2a47fa406ed8

John W. Eaton <jwe>
Group administrator
Tue 08 Sep 2020 08:40:06 PM UTC, comment #10: 

I'll take another look at it.  If it's not too difficult, I'd like to try to make it compatible with Matlab again.

John W. Eaton <jwe>
Group administrator
Fri 04 Sep 2020 04:25:15 AM UTC, comment #9: 

@jwe: What's your feeling on this bug?  Do we care to be Matlab-compatible here or just forget about it and close this report.

Rik <rik5>
Group administrator
Sat 15 Aug 2020 05:19:05 PM UTC, comment #8: 

There are other Octave repository scripts using that undocumented semantics of 'end':


./scripts/time/datevec.m:270:      msec = ds(nc:min (nc+2,end)); # pull 3-digit fractional seconds.
./scripts/io/importdata.m:302:    idx = (na_rows(1:min (header_rows, end)) + num_header_rows) <= header_rows;
f


There could be more.

Fernando <tutissanalio>
Sat 15 Aug 2020 05:08:59 PM UTC, comment #7: 

I found the stackoverflow thread very interesting. Thank you for the link. The funny part is that I thought that expressions like x(min(2,end)) were incorrect, until the other day I found they are used in one of the scripts in the Octave source code repository:


./scripts/miscellaneous/parseparams.m:111:  fname = stack(min (2, end)).name;


I was experimenting with one of the bugs and I made a modification that caused that end expression to fail. After the surprise, I thought it was a reasonable behaviour. As one of the comments of the stackexchange thread puts it: "when end occurs, I would assume it evaluates to an index (or dimension subscript) for the array with the most immediate scope, looking "up" through nested statements to make the determination."

In any case, since it is not a documented behaviour, I think it would be better to avoid that kind of 'end' expressions in the scripts of the Octave repository.

Fernando <tutissanalio>
Fri 14 Aug 2020 08:52:59 PM UTC, comment #6: 

I read the Stack Overflow article and that is just wacky.  I guess the question is whether we care to try for weird-for-weird compatibility with Matlab or not.  If not, then we can close this report.  Otherwise, we had the same behavior before so it might not be too hard to bring it back.

Rik <rik5>
Group administrator
Fri 14 Aug 2020 07:07:12 PM UTC, comment #5: 

While thinking about this "feature" I thought maybe I should see whether the behavior is documented.  I don't see that it is, and I found some interesting notes in the responses to this stack overflow question:  https://stackoverflow.com/questions/23016606/what-are-the-semantics-of-end-in-matlab/23017087

John W. Eaton <jwe>
Group administrator
Fri 14 Aug 2020 06:28:24 PM UTC, comment #4: 

Confirmed.  This is a regression from version 4.2.1 where it worked and 4.4.1 where it does not.  And for reference, Matlab produces the value 8 as well.

Rik <rik5>
Group administrator
Fri 14 Aug 2020 04:02:05 PM UTC, comment #3: 

"You are right", l meant.

Fernando <tutissanalio>
Fri 14 Aug 2020 03:52:23 PM UTC, comment #2: 

Young are right, this bug was already there. It was not caused by the changes you pushed yesterday.

Fernando <tutissanalio>
Fri 14 Aug 2020 03:27:32 PM UTC, comment #1: 

Thanks for the report.

This seems to also be a problem with Octave 5, so I don't think this is a new issue caused by the hanges I pushed yesterday, correct?

I thought we were doing this job correctly, but maybe we were only testing one level deep?  Hmm.  I'll try to take a look at this.

John W. Eaton <jwe>
Group administrator
Fri 14 Aug 2020 02:52:27 PM UTC, original submission:  

Octave stable (hg id: af302efce502) throws the following error


octave:1> x=1:10;
octave:2> x(minus(minus(end,1),1))
error: invalid use of 'end': may only be used to index existing value


The result should have been 8 ('end' should take the value 10).

In this other case, the result is correct.


octave:4> x(minus(end,1))
ans = 9


Fernando <tutissanalio>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #49901:  bug-58593-tests.patch added by tutissanalio (3KiB - text/x-patch)
file #49846:  refactor-end-diffs-3.txt added by jwe (39KiB - text/plain)
file #49828:  bug-58593.zip added by tutissanalio (2KiB - application/zip)
file #49823:  refactor-2-fix.txt added by tutissanalio (3KiB - text/plain)
file #49810:  myclass4.zip added by gyom (935B - application/zip)
file #49808:  refactor-end-diffs-2.txt added by jwe (35KiB - text/plain)
file #49806:  refactor-end-diffs.txt added by jwe (33KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by tutissanalio (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 24 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-05 jwe StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-10-01 tutissanalio Attached File- Added bug-58593-tests.patch, #49901
    2020-09-28 jwe StatusIn Progress Ready For Test
    2020-09-23 jwe Attached File- Added refactor-end-diffs-3.txt, #49846
    2020-09-21 jwe Attached File- Added end-refactor-with-tests-patch.txt, #49832
        Attached File- Added eliminate-has-magic-end-patch.txt, #49833
    2020-09-20 tutissanalio Attached File- Added bug-58593.zip, #49828
    2020-09-19 tutissanalio Attached File- Added refactor-2-fix.txt, #49823
    2020-09-18 gyom Attached File- Added myclass4.zip, #49810
    2020-09-18 jwe Attached File- Added refactor-end-diffs-2.txt, #49808
        Open/ClosedClosed Open
    2020-09-18 jwe Attached File- Added refactor-end-diffs.txt, #49806
        StatusFixed In Progress
    2020-09-09 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-09-09 jwe StatusNeed Info Ready For Test
    2020-08-14 rik5 Severity3 - Normal 2 - Minor
        Item GroupUnexpected Error or Warning Matlab Compatibility
        StatusNone Need Info
    2020-08-14 jwe Item GroupRegression Unexpected Error or Warning
        StatusConfirmed None
    2020-08-14 rik5 Item GroupUnexpected Error or Warning Regression
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code