bugGNU Octave - Bugs: bug #46852, smarter diagnostic message when a...

 
 

bug #46852: smarter diagnostic message when a user variable shadows a function definition

Submitted by:  Dan Sebald <sebald>
Submitted on:  Fri 08 Jan 2016 08:56:07 PM UTC  
 
Category:  Interpreter Severity:  1 - Wish
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Wed 15 Aug 2018 07:25:45 PM UTC, comment #14: 

This has been Ready for Test for over a year.  I checked and using the 'ls' example it does produce a warning about shadowing.  Closing report as fixed.

Rik <rik5>
Project Administrator
Thu 19 Jan 2017 02:47:03 AM UTC, comment #13: 

I think we can attach the extra shadowed function info at the point where the variable name is available.  It doesn't need to be passed into the function that creates the message, or generated by a callback function.  It can just be added later.  I checked in this simpler change instead:

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

John W. Eaton <jwe>
Project Administrator
Sat 18 Jun 2016 10:53:55 AM UTC, comment #12: 

Here is a patch that allows users of libinterp to get the warnings, but users of liboctave not to need to link to libinterp.

I don't think the shadowing can occur without the symbol table, so I don't think there is any need to have any additional warning for those not using libinterp.  Is that correct?

(file #37521)

Lachlan Andrew <lachlan>
Sat 18 Jun 2016 05:39:31 AM UTC, comment #11: 

That is a good point, Carnë.

One workaround would be to use a callback.  Libinterp could initialise the callback to null (which wouldn't be called), and liboctave could set it to Dan's code.  Would that be OK?  If so, I'm happy to code it.

I do something similar in patch #9024, so if you have a better solution, I'll update that patch too.

Lachlan Andrew <lachlan>
Fri 17 Jun 2016 02:51:03 PM UTC, comment #10: 

This patch makes liboctave dependent on libinterp. I don't think that should happen (is it already the case?). Note that it should be possible to use liboctave (and therefore trigger index_exception), on another program without starting the interpreter.

Carnë Draug <carandraug>
Project Member
Tue 08 Mar 2016 07:40:55 AM UTC, comment #9: 

It crossed my mind, but then I thought it would be a lot of work.  Maybe if one used some kind of search tool for the documentation in HelpBook-like format that would simplify things.  Then there are functions in packages as well.

Dan Sebald <sebald>
Tue 08 Mar 2016 01:30:34 AM UTC, comment #8: 

The use of DIR as an example variable is an interesting point.  Do you think we should go through all functions an ensure that they don't encourage shadowing built-in functions?

Lachlan Andrew <lachlan>
Mon 07 Mar 2016 07:00:01 PM UTC, comment #7: 

I like this too.  I still have that patch in my working copy of Octave and last night caught something immediately, e.g.:

octave:1> txtfiles = dir('*.txt');
octave:2> [dir, name, ext] = fileparts (txtfiles(1).name);
octave:3> csvfiles = dir('*.csv');
error: dir(118): out of bound 0
hint: function dir() may have been inadvertently redefined

Of course, what I was coding was more convoluted within a bigger file of code.  The reason I had used "dir" as a variable was because "help fileparts" uses DIR as an example variable.

Dan Sebald <sebald>
Sat 27 Feb 2016 02:31:57 AM UTC, comment #6: 

Dan, I really like the idea of this patch.

I've freshened it (lo-array-gripes is deprecated) and removed the inclusion of error.h in symtab.h, which seems unnecessary.

(file #36484)

Lachlan Andrew <lachlan>
Sun 10 Jan 2016 12:11:14 AM UTC, comment #5: 

How about something like the attached patch?  The change is basically a warning about using a variable that matches something in the symbol table but not until it causes a conflict.
  Here is the behavior:

octave:1> ggg = 34
ggg =  34
octave:2> ggg('u')
error: ggg(117): out of bound 1
octave:2> ls = 1
ls =  1
octave:3> ls('u')
error: ls(117): out of bound 1
hint: function ls() may have been inadvertently redefined

(file #35957)

Dan Sebald <sebald>
Sat 09 Jan 2016 02:16:10 AM UTC, comment #4: 

I found the problem: programmer error.  "length" was assigned a value within the function so had become a variable rather than a function call.  So, "not a bug", and I should have caught that earlier.  However, I would say the error message could be enhanced pretty easily by checking after this type of indexing error whether the variable in question also appears in the function table.  If so, print out "The function XYZ may have been redefined."

Note the slightly unusual behavior of the check in the sense that something like

octave:102> ls = 1;
octave:103> ls ('testingwithu')
error: ls(119): out of bound 1
octave:103> char(119)
ans = w

is flagging an error on a character in the middle of the character array when one would think it should be the 't' already that causes a problem.

Dan Sebald <sebald>
Sat 09 Jan 2016 01:14:51 AM UTC, comment #3: 

It does seem related to some character in the string.  That is, the error is:

error: length(65): out of bound 1

where 65 (the value) is the last character in the string if the string is less than seven characters.  When the string is greater than seven characters, then the value that appears in the error message is the seventh character, ascii value--which happened to be a 'u'.  If I set the structure element to [], there's no error.  If the particular structure field is assigned first in the list rather than last, there's no error.

I'll keep looking...

Dan Sebald <sebald>
Sat 09 Jan 2016 12:47:09 AM UTC, comment #2: 

OK, the reason you can't find that error message is that I wasn't in fact running the current code, but a slightly older version.  I've uninstalled and rebuilt again to verify that the day-old repository is what is running.

The error still exists but the message now reflects the current format.  This suggests this may not be related to the gripe-code changes, but some obscure structure bug that hasn't arisen before.  The message is now:

error: length(117): out of bound 1

The example you gave produces:

octave:3> ls = 1;
octave:4> ls ('u')
error: ls(117): out of bound 1

So, the above two bugs are not of the same nature.  Where the second error message makes sense, the first doesn't.  length(x) is the syntax causing the error.  length(117) is valid syntax if that is arising somehow.

I think that something (probably in the structure code) is causing an invalid error to be thrown and then the gripe code prints out a message accordingly, but it is a nonsensical error message.

I've tried to replicate this in a more direct way, but can't.  I've extracted the structure and worked on it directly and no error occurs.  Only in the circumstance of a bigger program.  I'll keep trying things and see if I can find a pattern.

Dan Sebald <sebald>
Fri 08 Jan 2016 09:14:03 PM UTC, comment #1: 

117 is the ASCII code for 'u'.  Does that give any clues?  Usually when I see out of bounds errors like this it is because a character string is being interpreted as an index when it is not expected.  For example:

ls = 1;
ls ('u')

Exactly what hg id are you using?  I don't see where the "index out of bounds" message is coming from.  However, the example code above does generate this message when I run it with Octave 4.0.0.

Also, where is "writechars" defined?  I don't see it in the Octave or Octave Forge sources.

Regardless of whether it is possible to create a simple test script, is is possible to send something that can be used to duplicate the problem?

John W. Eaton <jwe>
Project Administrator
Fri 08 Jan 2016 08:56:07 PM UTC, original submission:  

This isn't going to be a very good report because all I can do is describe what I'm seeing.  It is difficult to set up the conditions and data in a simple test script, and the behavior is rather odd and can be easily avoided.  I see this is related to the gripe code and that has been worked on recently, so I can only hope someone who's been working on that might have a guess.

I have this recursive function that steps through a structure doing an assortment of things based upon class of the structure element.  If the class of the element/field is a string, part of the computation is to use the length of that string.  The syntax

x = s.(name);

to get that string.  In one circumstance where the structure has several fields which are strings, a length(x) produces

error: writechars: A(I): index out of bounds; value 117 out of bound 1

It makes little sense that this should happen, and the string causing the error after several have succeeded is only about 30 characters.  I don't understand where 117 is coming from, or bound 1 for that matter.  If I shuffle the manner in which the fields are constructed, e.g., move the one that is failing to the first one added to the structure, the error goes away.

I see that the "index out of bounds" error message originates in

liboctave/util/lo-array-gripes.cc

from function gripe_index_out_of_range(), but the particular message only appears to originate from two locations having to do with delete.  (Does length() use delete somehow?)  I say this from looking at the latest code.  Code from months ago has the above message within gripe_index_out_of_range().  But the recent code has a more indirect way of generating the error message using "throw".

If you want me to look into this more, I can, but some hints of where to look would help.  The Array.cc file is where much of this array length checking is done, but why there should even be an error, I'm not sure.  Fixing by rearranging the structure field assignment complicates things as well.

Dan Sebald <sebald>

 

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

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by lachlan (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by sebald (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-03 mtmiller Carbon-CopyRemoved 80942 => -
    2018-08-15 rik5 StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2017-01-19 jwe StatusPatch Submitted => Ready For Test
    2016-06-18 lachlan Attached File- => Added bug_46852_shadowed_functions.v2.cset, #37521
    2016-02-27 lachlan Attached File- => Added bug_46852_shadowed_functions.cset, #36484
    2016-02-22 mtmiller Severity3 - Normal => 1 - Wish
        Item GroupIncorrect Result => Feature Request
        StatusNone => Patch Submitted
        Summaryerror gripe appearing when it seemingly shouldn\'t during length() => smarter diagnostic message when a user variable shadows a function definition
    2016-01-10 sebald Attached File- => Added octave-index_error_hint-djs_2016jan09.patch, #35957

    Back to the top


    Powered by Savane 3.5