bugGNU Octave - Bugs: bug #67096, nargout incorrect when index...

 
 

bug #67096: nargout incorrect when index expression with multiple outputs begins with function call

Submitter:  Fernando <tutissanalio>
Submitted:  Wed 07 May 2025 11:12:02 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Ready For Test Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Release: 
Operating System:  * Any Fixed Release:  None
Planned Release:  10.2.0 (current stable)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 16 May 2025 02:51:23 PM UTC, comment #14: 

I realized I had completely messed up the commit message for the patch in comment #12. I resend it here fixing the commit message. Also, I set nargout to 1 instead of -1. It is not necessary to use -1 here, since all non-final subsref expressions (and function calls) should return exactly one result.

I also checked that this does not affect non-final subsref operations with . or {}, i.e. if they produce more than one output (a cs-list), an error is thrown, just like without the patch, e.g.

octave:> x=struct('a',{num2cell(1:5),num2cell(2:5),num2cell(3:5)});
octave:> r=x.a{1}
error: a cs-list cannot be further indexed
octave:> y={struct('a',10),struct('a',20),struct('a',30)};
octave:> r=y{:}.a
error: a cs-list cannot be further indexed


So this should only affect function calls.

(file #57228)

Fernando <tutissanalio>
Thu 15 May 2025 07:19:39 PM UTC, comment #13: 

Maybe we can accept the original patch and open a new report for the issue in comment #12. I have also seen anoother case that fails, with or without the second patch:


classdef SimpleClass
   methods
      function r = get(obj)
         r = struct('data',num2cell(nargout:nargout+4));
      end
   end
end

octave:> sc=SimpleClass;
octave:> sc.get.data
ans = 0
ans = 1
ans = 2
ans = 3
ans = 4
octave:> [a,b]=sc.get.data
error: get: function called with too many outputs
error: called from
    get


It works if you put the () in the method call:

octave:> [a,b]=sc.get().data
a = 1
b = 2


In Matlab (R2024b), it works with or without the ().

Fernando <tutissanalio>
Tue 13 May 2025 05:13:17 PM UTC, comment #12: 

I did not take into account that there can be also a function call at an intermediate position of the index expression (via function handle, anonymous function, something else?).

I rewrote the patch (with tests) to consider that case too. Maybe this report's summary could be updated.

(file #57221)

Fernando <tutissanalio>
Tue 13 May 2025 09:50:36 AM UTC, comment #11: 


> The code for the current bytecode interpreter is only present on a separate branch.  It is not part of the default development branch of Octave.  So it is definitely not enabled by default on the branch that is currently on track to become Octave 11.x

Yes I forgot, thanks for pointing that out :-)

> If you are using it then you must have explicitly switched to the bytecode-interpreter branch at some point in the past.

Yes.
I'm blindly assuming that I do get "default branch behavior" after entering the command '__vm_enable__ (0)' , but I don't know if that were always completely true. FWIW, up till now I haven't noted any differences.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 13 May 2025 03:48:29 AM UTC, comment #10: 

RE: Comment #8:  The code for the current bytecode interpreter is only present on a separate branch.  It is not part of the default development branch of Octave.  So it is definitely not enabled by default on the branch that is currently on track to become Octave 11.x.  If you are using it then you must have explicitly switched to the bytecode-interpreter branch at some point in the past.

John W. Eaton <jwe>
Group administrator
Mon 12 May 2025 08:07:47 PM UTC, comment #9: 

comentario nº8:

> OK, so what do we do about it?


Is it an option to just ignore these 2 BIST errors for now, until someone fixed the issue in the bytecode-interpreter? After all, the error has been there up to now, only we did not have BIST tests to detect it.

> AFAICS the problematic index expression isn't inside the command line function used for the BISTs. Yet somehow the bytecode-interpreter stumbles over it.


I think the test function get the BIST code and wraps it into a function before running it.

Fernando <tutissanalio>
Mon 12 May 2025 07:03:21 PM UTC, comment #8: 

OK, so what do we do about it?

Currently it's only a problem with dev Octave (11.0.0) as the bytecode-interpreter is enabled by default there.
On stable the bytecode-interpreter isn't implemented so this is no problem for soon-to-come 10.2.0.

AFAICS the problematic index expression isn't inside the command line function used for the BISTs. Yet somehow the bytecode-interpreter stumbles over it.
In the course of an earlier bug report I was wondering if there could be a config feature setting for the bytecode-interpreter so that BISTs can be skipped; but the issue with that is that the fault lies in the bytecode-interpreter rather than in the BIST. So that would be a wrong solution.

(FYI I mostly run bleeding edge Octave + pkgs for my projects so I'm often one of the first to hit my nose on s/th.)

Philip Nienhuis <philipnienhuis>
Group Member
Mon 12 May 2025 11:13:32 AM UTC, comment #7: 

It seems that the failure with the bytecode-interpreter happens when the problematic index expression is in a compiled function.


# Function defined in file or in command-line
function [a]=fcell()
  a=num2cell(1:2);
endfunction

# Function defined in file or in command-line
function [a,b]=myindex()
 [a, b] = fcell () {1:2};
end

octave:> [a, b] = fcell () {1:2}
a = 1
b = 2

octave:> myindex()
error: fcell: function called with too many outputs


Fernando <tutissanalio>
Sun 11 May 2025 05:49:32 PM UTC, comment #6: 

Seems this has more to do with the way the bytecode-interpreter handles functions declared in tests. If I declare fcell() before the test, it runs fine:

## just running test
>> test pt-idx.cc-tst
***** test
 function [a] = fcell ()
   a = num2cell (1:2);
 endfunction

 [a, b] = fcell () {1:2};
 assert ([a, b], [1, 2])
!!!!! test failed
fcell: function called with too many outputs

## declaring function on command line
>>  function [a] = fcell ()
   a = num2cell (1:2);
 endfunction

>> [a, b] = fcell ()
error: fcell: function called with too many outputs

## now test runs fine
>> [a, b] = fcell () {1:2}
a = 1
b = 2
>> fcell
ans =
{
  [1,1] = 1
  [1,2] = 2
}

>> __vm_enable__ ()
ans = 1


Philip Nienhuis <philipnienhuis>
Group Member
Sun 11 May 2025 12:32:16 PM UTC, comment #5: 

On dev Octave I get 2 FAILs for the new BISTs. Excerpt from fntests.log:

:
>>>>> processing /home/philip/devel/octdev/oct1100+/libinterp/parse-tree/pt-idx.cc-tst
***** test
 function [a] = fcell ()
   a = num2cell (1:2);
 endfunction

 [a, b] = fcell () {1:2};
 assert ([a, b], [1, 2])
!!!!! test failed
fcell: function called with too many outputs
***** test
 function [a] = fstruct ()
   a = struct ('a', {1, 2}, 'b', {3, 4});
 endfunction

 [a, b] = fstruct ().b;
 assert ([a, b], [3, 4])
!!!!! test failed
fstruct: function called with too many outputs
:

But that was with the bytecode-interpreter active. See:

## cd to libinterp/parse-tree/
>> test pt-idx.cc-tst
***** test
 function [a] = fcell ()
   a = num2cell (1:2);
 endfunction

 [a, b] = fcell () {1:2};
 assert ([a, b], [1, 2])
!!!!! test failed
fcell: function called with too many outputs
>>
>> __vm_enable__ ()
ans = 1
>>
>> __vm_enable__ (0)
>> clear -f
>>
>> test pt-idx.cc-tst
PASSES 8 out of 8 tests
>>


Philip Nienhuis <philipnienhuis>
Group Member
Thu 08 May 2025 03:36:50 PM UTC, comment #4: 

These changes look pretty safe to me. And they are fixing a bug. So, I went ahead and pushed the patch (with only minor changes) to the stable branch:
https://hg.savannah.gnu.org/hgweb/octave/rev/42c080f27d71

It should be part of Octave 10.2.0 when it is released.

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Thu 08 May 2025 06:59:35 AM UTC, comment #3: 

Yes, I figured that out after posting my comment, I am afraid.

Andreas Bertsatos <pr0m1th3as>
Thu 08 May 2025 06:19:46 AM UTC, comment #2: 

Thank you again for your contributions. Your patch looks good to me (untested so far).

Just some minor style issues (that I can also address locally when I come around to testing this):

  • Whitespace between function name and opening parenthesis in line 26 of your patch.
  • " (bug #67096)." at the end of the first line of the commit message.
  • Start sentences with capital letter in commit message.


(@Andreas: That's not the point of this report. It is about the indexing immediately after the function call...)

Markus Mützel <mmuetzel>
Group administrator
Thu 08 May 2025 05:27:16 AM UTC, comment #1: 

The way you define your function is with a single variable output. If you want multiple variable output arguments, then define

function varargout = fcell ()
  varargout = num2cell (1:2);
endfunction


Andreas Bertsatos <pr0m1th3as>
Wed 07 May 2025 11:12:02 PM UTC, original submission:  

If I define this function

function [a] = fcell ()
  a = num2cell (1:2);
endfunction

I get an error in the following sentence (which should result in a=1, b=2).

octave:> [a,b] = fcell () {1:2}
error: fcell: function called with too many outputs
error: called from
    fcell

The same with this:

function [a] = fstruct ()
  a = struct ('a', {1, 2}, 'b', {3, 4});
end
octave:> [a,b] = fstruct ().a
error: fstruct: function called with too many outputs
error: called from
    fstruc

The problem is that [a,b] is taken as the outpus of the function call, when they are the outputs of indexing with {} or . The solution seems easy. Here is a possible fix, with a couple of tests.

Fernando <tutissanalio>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #57228:  bug67096.patch added by tutissanalio (4KiB - text/x-patch)
file #57221:  bug67096.patch added by tutissanalio (4KiB - text/x-patch)
file #57200:  bug.patch added by tutissanalio (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2025-05-16 tutissanalio Attached File- Added bug67096.patch, #57228
    2025-05-13 tutissanalio Attached File- Added bug67096.patch, #57221
    2025-05-08 mmuetzel StatusPatch Reviewed Ready For Test
        Planned Release11.1.0 (current default) 10.2.0 (current stable)
    2025-05-08 mmuetzel StatusNone Patch Reviewed
        Planned ReleaseNone 11.1.0 (current default)
    2025-05-07 tutissanalio Attached File- Added bug.patch, #57200

    Back to the top

    Powered by Savane 3.15-f85b.
    Corresponding source code