Great. Thank you, jwe.

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)

Is this ready to close now?

I think stable is fine as well.

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.

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.

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?

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);

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)

I attach some tests for this bug.

(file #49828)

Oops! I forgot the patch. Here it is.

(file #49823)

Found the answer I was looking for:
https://www.mathworks.com/matlabcentral/answers/309521-overloading-of-subsref-partially-failing#accepted_answer_241039

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

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)

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)

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.

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

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.

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?

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)))

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+

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?

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?

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.

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.

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

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

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".

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

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.

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.

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.

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.

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

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