bugGNU Octave - Bugs: bug #63841, nargout is always 0 or 1 for...

 
 

bug #63841: nargout is always 0 or 1 for subsref() in classdef methods

Submitter:  A.R. Burgers <arb>
Submitted:  Sun 26 Feb 2023 10:23:21 PM UTC
   
 
Category:  Octave Function Severity:  5 - Blocker
Priority:  7 - High Item Group:  Regression
Status:  Ready For Test Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  9.1.0 (current stable)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 14 Oct 2023 12:54:42 PM UTC, comment #37: 

Just came across this bug linked from bug #48693. Hadn't realized there was a high-priority blocker for Octave 9. I've pushed this here:



Without the VM, there's no failure in `make check`:

  PASS                            18726
  FAIL                                0
  XFAIL (reported bug)               39
  SKIP (missing feature)             57
  SKIP (run-time condition)          27


With the VM, with `make check-vm`, there are 15 failures, one with bug #48693 and 14 errors-with-errors with bug #63841 (this bug). I will open a separate bug report for that.


  PASS                            18711
  FAIL                               15
  XFAIL (reported bug)               39
  SKIP (missing feature)             57
  SKIP (run-time condition)          27


Marking as ready for test.

Arun Giridhar <arungiridhar>
Group Member
Tue 29 Aug 2023 05:39:27 PM UTC, comment #36: 

Is it possible to commit this fix?
This is a pretty significant one.

A.R. Burgers <arb>
Sat 15 Apr 2023 08:59:18 AM UTC, comment #35: 

As for the tests, I think we should also consider struct-based classes for the #!error checks. I post here an updated test patch. I use the names of A.R. Burgers and myself as authors of the patch.

(file #54633)

Fernando <tutissanalio>
Sat 15 Apr 2023 08:50:59 AM UTC, comment #34: 

Not sure if I was expected to post a proper commit patch for the fix with a commit message. Just in case, I post it here.

As the fist line for the message, I just wrote:

Fix nargout when calling class subsref method (bug #63841).

I could be more precise but the message then would be probably too long, e.g:

Fix nargout when calling class subsref method and nargout independent of indexing expression (bug #63841).

(file #54632)

Fernando <tutissanalio>
Thu 13 Apr 2023 07:30:04 PM UTC, comment #33: 

In response to comment #27 a update of the change set of the tests with copyright restricted to 2023

(file #54618)

A.R. Burgers <arb>
Thu 13 Apr 2023 05:06:49 PM UTC, comment #32: 

@jwe: with respect to your question "Is it correct that this bug only affects the default branch, not stable?"

I thought that was the case, but I found it also affects the stable branch in some cases:

- For struct-based classes, most of the tests (of the last patch provided by @arb) fail.

- For classdef classes, the tests corresponding to error tests fail to produce an error.

However, these cases are not directly reported in this bug report. I don't know if they are a regression, because I don't know if they worked previously or when. I don't think these cases were caused by the fix for bug #48693.

Fernando <tutissanalio>
Thu 13 Apr 2023 04:22:06 PM UTC, comment #31: 

@jwe, you are completely right.

Fernando <tutissanalio>
Thu 13 Apr 2023 04:20:01 PM UTC, comment #30: 

@mmuetzel:  If I understand correctly, our setting of nargout to -1 is just an internal thing used to indicate whether the interpreter should analyze the expression to set nargout when performing indexing operations.  It shouldn't escape to function calls.

This internal "nargout" value is also separate from using the nargout function in the scripting language to ask "how many values is this function defined to return?".

@arb and @tutissanalio, Please correct me if I'm wrong.

John W. Eaton <jwe>
Group administrator
Thu 13 Apr 2023 04:19:45 PM UTC, comment #29: 

Markus, I don't think there is any conflict. In this patch, negative values of nargout are used internally by the interpreter and are never exposed to the called function. More precisely, a value of nargout<0 when calling a function is transformed to nargout=1.

Fernando <tutissanalio>
Thu 13 Apr 2023 03:58:17 PM UTC, comment #28: 

I believe negative nargout are used in Matlab to indicate that a function uses `varargout`:
https://www.mathworks.com/help/matlab/ref/nargout.html

> nargout(fun) returns the number of outputs that appear in the fun function definition. If the function includes varargout in its definition, then nargout returns the negative of the number of outputs. For example, if function myFun declares outputs y, z, and varargout, then nargout('myFun') returns -3.


Does that conflict with that usage?

Markus M├╝tzel <mmuetzel>
Group administrator
Thu 13 Apr 2023 02:32:06 PM UTC, comment #27: 

I looked over the latest patch (bug-63481-fix5.diff) and it seems reasonable to me to use nargout = -1 to indicate that nargout is unknown.

Is it correct that this bug only affects the default branch, not stable?  If so, then I think we can go ahead and push the change once it has a commit message.

Small nit: the test file should probably not claim copyright for prior years if it is a completely new file.

Thanks for all the work on this issue!

John W. Eaton <jwe>
Group administrator
Thu 13 Apr 2023 10:59:22 AM UTC, comment #26: 

I combined the two changesets for the tests into 1 (and fixed a bug in the integration into the build system as well)

(file #54611)

A.R. Burgers <arb>
Mon 10 Apr 2023 04:48:01 PM UTC, comment #25: 

Thanks for your efforts Fernando, that took you well beyond just the subsref methods, and it looks pretty good. Hope to do some testing with your most recent patches soon.

A.R. Burgers <arb>
Fri 07 Apr 2023 05:02:47 PM UTC, comment #24: 

Still another update, with more expressions that should produce errors: unary operations (prefix and postfix) and compound binary operations.

Again, I provide update for the code and for the tests (the patch for the tests to be applied on top of @arb's patch).

(file #54580, file #54581)

Fernando <tutissanalio>
Fri 07 Apr 2023 01:22:58 PM UTC, comment #23: 

Yet another update for the patches, to consider some corner cases which errored before the patch and I think they should continue to error. In particular:


octave:1> A=tst_nargout;
octave:2> 1+A(1:3).prop
nargout=3, s(1).type = ()
error: binary operator '+' not implemented for 'scalar' by 'cs-list' operations
octave:3> A(1:3).prop+1
nargout=3, s(1).type = ()
error: binary operator '+' not implemented for 'cs-list' by 'scalar' operations
octave:4> A(1:3).prop+A(1:3).prop
nargout=3, s(1).type = ()
nargout=3, s(1).type = ()
error: binary operator '+' not implemented for 'cs-list' by 'cs-list' operations


I have updated the patch for the tests also, to be applied on top of @arb's patch (file #54523).

Both patches have been generated with `hg diff`. Let me know if you want me to generate any of them with `hg export`.

(file #54577, file #54578)

Fernando <tutissanalio>
Thu 06 Apr 2023 09:20:58 PM UTC, comment #22: 

I have updated the patch to also take into account old-style (struct-based) classes. For that, the changes made to octave_classdef::subsref had to be copied also to octave_class::subsref (the code is practically duplicated in both places, as pointed out in a FIXME comment in octave_class::subsref).

I also include a patch for the tests, to be applied on top of @arb's patch (file #54523). This patch extends the tests to consider old-style classes, and also adds some more cases for both classdef and old-style classes, including the cases mentioned in comment #21.

(file #54574, file #54575)

Fernando <tutissanalio>
Thu 06 Apr 2023 09:33:01 AM UTC, comment #21: 

Here is a patch that fixes both the problem from the original post and the problem from comment #11. It passes all the tests (at least, those not skipped), including those provided by @arb.

The idea of this patch is to differentiate the case when the number of output arguments resulting from evaluating an expression is unkown a priori (and the expression is not a complete statement). In this case, we call the corresponding evaluate method with the argument nargout=-1.

I think it would be good to also add a test for these cases


>> A=tst_nargout;
>> if A(1:5).a_property,true,end
nargout=1, s(1).type = ()
ans =
  logical
   1
>> for x=A(1:5).a_property,true,end
nargout=1, s(1).type = ()
ans =
  logical
   1


It is different form the case


if cm('first').a_property, true, end


because A(1:5).a_property actually returns several outputs, of which only the first one should be taken, while cm('first').a_property returns only one output.

(file #54566)

Fernando <tutissanalio>
Tue 04 Apr 2023 02:53:17 PM UTC, comment #20: 

It looks like you are right: when the indexing expression is in the context of an if/while condition, nargout should always be set to 1. This is what I get in Matlab:


>> A=tst_nargout;
>> if A(1:5).a_property,true,end
nargout=1, s(1).type = ()
ans =
  logical
   1
>> while A(1:5).a_property,true,break,end
nargout=1, s(1).type = ()
ans =
  logical
   1
>> for x=A(1:5).a_property,true,end
nargout=1, s(1).type = ()
ans =
  logical
   1


I will review the code and think how this can be done. Thank you!

Fernando <tutissanalio>
Tue 04 Apr 2023 08:57:15 AM UTC, comment #19: 

So far I fail to see why there should be a difference between these two contexts


a = cm('first').a_property
if cm('first').a_property, true, end


I would expect the if condition context to request 1 output from the a_property method, and the tst_nargout subsref should be called with nargout=1.

In both cases I expect the cm('first') to be called in the same "." dot indexing context that should result in cm('first') to be returned.

A.R. Burgers <arb>
Mon 03 Apr 2023 08:40:59 AM UTC, comment #18: 

By the way, the title of the bug report could be changed, because nargout is not always 0 or 1, as can be verified executing:


> A=tst_nargout;
> A{1:5}     % nargout=5
> A(1:5).a_property  % nargout=5


The title could say something like "nargout incorrect in assignments involving subsref() for classdef methods".


Fernando <tutissanalio>
Mon 03 Apr 2023 08:11:54 AM UTC, comment #17: 

Yes, the issue from comment #11 broke also with the fix for bug #48693, but the issue is different from the one described in the original post, and it seems to be much more complex. Let me explain.

The fix for bug #48693 was introduced to cope with statements like

x{1:10}

where x is a classdef class with custom subsref method. In that case, the subsref method should be called with nargout set to the number of values that the indexing argument addresses (10 in this case).

However, the fix did not take into account that in expressions like

a=x{1:10}

or

[a,b]=x{1:10}

nargout should be taken from the number of variables being assigned to (1 or 2 in those cases). That's the cause of the problem described in the original submission of this bug, and it can be solved with the patch proposed (file #54549).

A second problem is the one identified in comment #11, and I think now that it should be treated in a separate bug report. It affects sentences like

cm('first').a_property

where cm is a containers.Map object. In this case, cm's subsref method should be called with nargout=1, but it is called with nargout=5 because 'first' is an array of 5 elements, so we are in the same case as in

x(1:5).a_property

Contrary to what I said in comment #14, now I think octave is not doing anything wrong here, because it is valid to assume that nargout will be 5 in both cases, unless we tell octave otherwise, which we are not telling. In fact, if you introduce in Matlab:


A=tst_nargout;
A('first').a_property


you will see that nargout=5. So in this case we need a way to tell octave that cm('first').a_property yields only one result. I have seen that in Matlab that can be done in two different ways:


The second is the recommended way to do it from version R2021b onwards. That version also introduces a different way to implement customized indexing, which is to be preferred over the subsref/subsasgn functions. I don't know what are the plans for octave in that respect.

So, I propose file #54549 for solving this issue. You can adapt some of the tests proposed by A.R. Burgers, taking into account that a sentence like

a = cm('first').a_property

should work, but a sentence like

if cm('first').a_property, true, end

does not work for the moment.

Fernando <tutissanalio>
Sun 02 Apr 2023 08:17:24 PM UTC, comment #16: 

The issue from comment #11 has been broken somewhere between 8.1.1 and 9.0.0. With this script:


version
hg_id = __octave_config_info__.hg_id
cm = containers.Map;
cm('first') = tst_nargout;
ti = tst_nargout;
a_ti = ti.a_property

try
  ti = cm('first');
catch me
  fprintf('ti = cm(''first'') failed: %s\n', me.message);
end

try
  a = cm('first').a_property
catch me
  fprintf('a = cm(''first'').a_property failed: %s\n', me.message);
end

try
  if cm('first').a_property
    true
  end
catch me
  fprintf('if cm(''first'').a_property failed: %s\n', me.message);
end


works with 8.1.1


ans = 8.1.1
hg_id = 622985ce895b
nargout=1, s(1).type = .
a_ti = 1
nargout=1, s(1).type = .
a = 1
nargout=1, s(1).type = .
ans = 1


doesn't work with 9.0.0


ans = 9.0.0
hg_id = 630187b00ca2
nargout=1, s(1).type = .
a_ti = 1
a = cm('first').a_property failed: subsref: function called with too many outputs
if cm('first').a_property failed: subsref: function called with too many outputs


A.R. Burgers <arb>
Sun 02 Apr 2023 11:39:16 AM UTC, comment #15: 

Here is a partial fix, solving the problem described in the original submission, but not the error in comment #11. I.e. it solves the first problem I pointed out in my previous comment.

The patch simply takes into account that, if the indexing operation is in the context of an assignment (or multiassignment), nargout should take its value from the number of variables being assigned to, and not from the analysis of the indexing expression.

(file #54549)

Fernando <tutissanalio>
Sat 01 Apr 2023 05:10:25 PM UTC, comment #14: 

I'm afraid I am partly to blame for this issue, because I proposed the fix for bug #48693.

A.R. Burgers, I could reproduce the error from your comment #11 even without applying the patch from comment #10. There seems to be two problems here:

1. In a (multi-)assigment expression, nargout should take its value from the number of variables being assigned. E.g. in a sentence like

[a,b]=tst_nargout.call_a_method

the value of nargout that subsref receives should be 2, no matter what the indexing expression is. That's the problem causing this issue.

2. Octave analyzes the object indexing expression to try to set up a proper value for nargout (this is necessary if the indexing result is not being assigned to variables). However, this analysis is sometimes incorrect, as in the expression

cm('first').a_property

from comment #11. In this case, the subsref method of the containers.Map object cm is receiving nargout=5, instead of 1, because 'first' is an array of length 5, so octave treats this like the expression x(1:5).a_property, which should provide 5 results.

Fernando <tutissanalio>
Fri 24 Mar 2023 01:11:41 PM UTC, comment #13: 

I propose a few tests for this issue. See the attached diff

(file #54523)

A.R. Burgers <arb>
Thu 23 Mar 2023 03:24:31 PM UTC, comment #12: 

Thanks for testing.  There was a small chance this was going to be easy to fix, but I guess it won't be.  It will need someone more expert in the classdef code to propose a solution.

Rik <rik5>
Group administrator
Thu 23 Mar 2023 03:16:06 PM UTC, comment #11: 

Unfortunately this approach seems to break something else.
I applied your change.

I changed the class to have a property:


classdef tst_nargout
  properties
    a_property = 1;
  end
  methods
    function varargout = subsref(obj, s)
      fprintf('nargout=%d, s(1).type = %s\n', nargout, s(1).type);
      varargout = {nargout, s(1).type, 'arg3'};
    end
  end
end


and call that class like this from a containers.Map:


m = containers.Map;
cm('first') = tst_nargout;

if cm('first').a_property
  true
end


I get this error.


error: subsref: function called with too many outputs
error: called from
    subsref
    u2 at line 4 column 4


A.R. Burgers <arb>
Mon 27 Feb 2023 04:58:00 PM UTC, comment #10: 

I don't pretend this is brilliant, but this patch seems to fix the case for the two reported bugs.


diff --git a/libinterp/octave-value/ov-classdef.cc b/libinterp/octave-value/ov-classdef.cc
--- a/libinterp/octave-value/ov-classdef.cc
+++ b/libinterp/octave-value/ov-classdef.cc
@@ -102,8 +102,9 @@ octave_classdef::subsref (const std::str
             {
               // Set up a proper nargout for the subsref call by calling numel.
               octave_value_list tmp;
-              if (type[0] != '.') tmp = idx.front ();
-              nargout = xnumel (tmp);
+              if (type[0] != '.')
+                tmp = idx.front ();
+              nargout = std::max (static_cast<int> (xnumel (tmp)), nargout);
             }

           retval = meth.execute (args, nargout, true, "subsref");


All it does is use the maximum value for nargout which is either the number of output variables asked for by the interpeter (nargout input parameter to function) or the maximum number of outputs for a varargout expression taken from the index operation.

(file #54400)

Rik <rik5>
Group administrator
Mon 27 Feb 2023 04:28:03 PM UTC, comment #9: 

Comment #8 (subsref works for (), not for {} or .) seems consistent with the changeset that jwe identified by bisection.  That cset begins with


+          // Attempt to set up a proper value for nargout at least in the
+          // simple case where the cs-list-type expression - i.e., {} or
+          // ().x, is the leading one.
+          bool maybe_cs_list_query = (type[0] == '.' || type[0] == '{'
+                                      || (type.length () > 1 && type[0] == '('
+                                          && type[1] == '.'));
+
+          if (maybe_cs_list_query)
+            {
+              // Set up a proper nargout for the subsref call by calling numel.
+              octave_value_list tmp;
+              if (type[0] != '.') tmp = idx.front ();
+              nargout = xnumel (tmp);
+            }


The comment in the code about the variable "maybe_cs_list_query" implies that this is about calculating the specific cases of "{}" and ".".

Rik <rik5>
Group administrator
Mon 27 Feb 2023 02:32:26 PM UTC, comment #8: 

for () indexing also octave-9 gets it right, but not for {} and .

class tst_nargout:


classdef tst_nargout
  methods
    function varargout = subsref(obj, s)
      fprintf('nargout=%d, s(1).type = %s\n', nargout, s(1).type);
    end
  end
end


exercising tst_nargout:


ti = tst_nargout;

fprintf('2 outputs requested\n');
try; [a, b] = ti.call_a_method; catch; end
try; [a, b] = ti{1};            catch; end
try; [a, b] = ti(1);            catch; end

fprintf('3 outputs requested\n');
try; [a, b, c] = ti.call_a_method; catch; end
try; [a, b, c] = ti{1};            catch; end
try; [a, b, c] = ti(1);            catch; end


octave-8 output


2 outputs requested
nargout=2, s(1).type = .
nargout=2, s(1).type = {}
nargout=2, s(1).type = ()
3 outputs requested
nargout=3, s(1).type = .
nargout=3, s(1).type = {}
nargout=3, s(1).type = ()


octave-9 output


2 outputs requested
nargout=1, s(1).type = .
nargout=1, s(1).type = {}
nargout=2, s(1).type = ()
3 outputs requested
nargout=1, s(1).type = .
nargout=1, s(1).type = {}
nargout=3, s(1).type = ()


A.R. Burgers <arb>
Mon 27 Feb 2023 05:40:47 AM UTC, comment #7: 

Bisecting points to the following change that was made in an attempt to fix bug #48693:

http://hg.savannah.gnu.org/hgweb/octave/rev/a18897e4c7b5

John W. Eaton <jwe>
Group administrator
  Spam posted by anonymous
Mon 27 Feb 2023 02:28:12 AM UTC, comment #5: 

I take back my remarks in comment #1: this seems to be specific to subsref().

Rik <rik5>
Group administrator
Mon 27 Feb 2023 02:14:41 AM UTC, comment #4: 

It is probably tangentially related to bug #48693, as in the fix probably is in the same section of code, but this is slightly different.  nargout is simply the wrong value.

Rik <rik5>
Group administrator
Mon 27 Feb 2023 02:06:14 AM UTC, comment #3: 
Nicholas Jankowski <nrjank>
Group Member
Mon 27 Feb 2023 01:50:14 AM UTC, comment #2: 

Clearly Octave does not have a sufficient test suite for classdef code because this should have been found much sooner.

Rik <rik5>
Group administrator
Mon 27 Feb 2023 01:48:22 AM UTC, comment #1: 

Confirmed.  And the problem seems to be general to any method of a classdef object.  I used the following code:


classdef tst_nargout
  methods
    function varargout = subsref (obj, s)
      nout = nargout
    end

    function varargout = myfunc (obj, s)
      nout = nargout
    end

  end
end


Using nargout in an ordinary method then produces the wrong answer.


o = tst_nargout
o =

  tst_nargout object with properties:


octave:2> [a,b,c] = o.myfunc ()
error: element number 1 undefined in return list
nout = 1




(file #54398)

Rik <rik5>
Group administrator
Sun 26 Feb 2023 10:23:21 PM UTC, original submission:  

consider this minimal class with a subsref method


classdef foo
  methods
    function varargout = subsref(obj, s)
      no = nargout
    end
  end
end


and this use of the foo class, which requests subsref to return 2 outputs:


fi = foo;
[a, b] = fi.call_a_method;


octave-8.0.91 - correctly - returns 2 for no, octave-9.0.0 returns 1

octave-9.0.0 hg-id=c80cf1588ed0

A.R. Burgers <arb>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54632:  bug-63841-fix6.patch added by tutissanalio (11KiB - text/x-patch)
file #54618:  bug-63841-tests6.txt added by arb (6KiB - text/plain)
file #54611:  bug-63841-tests5.txt added by arb (6KiB - text/plain)
file #54580:  bug-63481-fix5.diff added by tutissanalio (10KiB - text/x-patch)
file #54581:  bug-63481-tests4.diff added by tutissanalio (4KiB - text/x-patch)
file #54577:  bug-63481-fix4.diff added by tutissanalio (9KiB - text/x-patch)
file #54578:  bug-63481-tests3.diff added by tutissanalio (3KiB - text/x-patch)
file #54574:  bug-63481-fix3.diff added by tutissanalio (8KiB - text/x-patch)
file #54566:  bug-63481-fix2.diff added by tutissanalio (5KiB - text/x-patch)
file #54549:  bug-63481-fix1.diff added by tutissanalio (2KiB - text/x-patch)
file #54523:  bug-63481.diff added by arb (4KiB - application/octet-stream)
file #54400:  patch.63841 added by rik5 (702B - application/octet-stream)
file #54398:  tst_nargout.m added by rik5 (184B - text/x-matlab)

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by tutissanalio (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by arb (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 project members can vote.

     

    Follow 23 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-10-14 arungiridhar StatusConfirmed Ready For Test
    2023-04-15 tutissanalio Attached File- Added bug-63841-tests7.txt, #54633
    2023-04-15 tutissanalio Attached File- Added bug-63841-fix6.patch, #54632
    2023-04-13 arb Attached File- Added bug-63841-tests6.txt, #54618
    2023-04-13 arb Attached File- Added bug-63841-tests5.txt, #54611
    2023-04-07 tutissanalio Attached File- Added bug-63481-fix5.diff, #54580
        Attached File- Added bug-63481-tests4.diff, #54581
    2023-04-07 tutissanalio Attached File- Added bug-63481-fix4.diff, #54577
        Attached File- Added bug-63481-tests3.diff, #54578
    2023-04-06 tutissanalio Attached File- Added bug-63481-fix3.diff, #54574
        Attached File- Added bug-63481-tests-2.diff, #54575
    2023-04-06 tutissanalio Attached File- Added bug-63481-fix2.diff, #54566
    2023-04-02 tutissanalio Attached File- Added bug-63481-fix1.diff, #54549
    2023-03-24 arb Attached File- Added bug-63481.diff, #54523
    2023-02-27 rik5 Dependencies- Depends on bugs #48693
    2023-02-27 rik5 Attached File- Added patch.63841, #54400
    2023-02-27 rik5 Summarynargout is always 1 for classdef methods nargout is always 0 or 1 for subsref() in classdef methods
    2023-02-27 rik5 Planned ReleaseNone 9.1.0 (current stable)
    2023-02-27 rik5 Attached File- Added tst_nargout.m, #54398
        Severity3 - Normal 5 - Blocker
        Priority5 - Normal 7 - High
        StatusNone Confirmed
        Summarysubsref gets wrong number of varargout elements nargout is always 1 for classdef methods

    Back to the top

    Powered by Savane 3.12