bugGNU Octave - Bugs: bug #46536, Bad indexing could produce more...

 
 

bug #46536: Bad indexing could produce more informative error messages

Submitter:  Rik <rik5>
Submitted:  Wed 25 Nov 2015 11:57:13 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 17 Jan 2016 04:08:47 PM UTC, comment #17: 

@jwe: I think I need to leave this patch review to you.  It is too deep in the parser for me to follow well.

Rik <rik5>
Group administrator
Tue 05 Jan 2016 09:09:21 PM UTC, comment #16: 

@jwe: open question to you about splitting.  Beyond that I think we should commit something and move on.

Rik <rik5>
Group administrator
Tue 29 Dec 2015 05:33:13 AM UTC, comment #15: 

Yes, jwe.  I was assuming that the splitting was so that subsref would work, but I was assuming the reason for not splitting all cases was efficiency.  Is there another reason not to split after each () and perhaps after each {}?

Lachlan Andrew <lachlan>
Sun 20 Dec 2015 05:57:31 PM UTC, comment #14: 

I'm not sure whether the splitting was for efficiency or so that subsref for classes would work properly.

John W. Eaton <jwe>
Group administrator
Sat 19 Dec 2015 04:19:51 AM UTC, comment #13: 

Thanks for checking that, jwe.  I'll edit the comment.

The question now is whether more informative error messages are worth the extra cost of splitting after every  ().  I assume that the evaluation in the final  subsref()  gets cheaper by about the expense of the code executed if force_split is true. Is that the case?  I did some experiments with the following script, and the differences were lost in the noise


for i = 30:-1:1
  for j = 30:-1:1
    for k = 30:-1:1
      for l = 30:-1:1
        a.foo(i).bar(j).cat(k).dog(l) = 1;
      end
    end
  end
end


If so, it seems to me that splitting after every () would be worthwhile.  If the line is unchanged, it will just mean there are more cases that get  c...(-1).  What do you think?

Lachlan Andrew <lachlan>
Fri 18 Dec 2015 08:27:56 PM UTC, comment #12: 

The force_split variable was introduced with this changeset:

  http://hg.savannah.gnu.org/hgweb/octave/rev/126b49caba0d

Given the commit message, I think the code is not a mistake but the comment is not as clear as it could be.

John W. Eaton <jwe>
Group administrator
Thu 17 Dec 2015 02:43:10 AM UTC, comment #11: 

Thanks for your feedback Rik (and jwe) -- it is making the patch much better, and I'm learning lots.

Version 5 of the patch:
- indicates which position is in error in an r-value, provided the only thing following a "(" is a ".", as required by Matlab.
- indicates the structure components leading up to a single "(" in an assignment statement.
- writes c...(-1)  instead of c<...>(-1) in other cases, in an attempt to put less "visual distance" between the "c" and the "(-1)", so that  c(-1).foo(1)  seems a more plausible bug to look for.

There is more code in the error path, but no additional code on the fast path, except fixing what I think is a bug, as follows.

I think the definition of force_split was wrong.  It was


// In Matlab, () can only be followed by . In Octave, we do not
// enforce this for rvalue expressions, but we'll split the
// evaluation at this point. This will, hopefully, allow Octave's
// looser rules apply smoothly for Matlab overloaded subsref
// codes.
bool force_split = type[i-1] == '(' && type[i] != '.';


which forces a split whenever () is not followed by ., whereas the comment says that the split is forced whenever () is followed by '.'.  The splits are useful to determine which () has the error.  The patch currently replaces != by ==.  Is that what was actually intended?

(file #35777)

Lachlan Andrew <lachlan>
Tue 15 Dec 2015 05:59:37 PM UTC, comment #10: 

@Lachlan: jwe expressed an opinion, and it was to keep your patch as is.

I made a tiny tweak which is to rename the temporary variable tmp_XXX rather than my_XXX.  That's just a convention I like because it indicates the purpose of the variable in the name.  Having programmed in Perl quite a bit, I also recognize "my" as a way to get a locally instantiated copy of a variable.  But I don't think that convention is as widespread as the other.

Anyways, I'm uploading a version 4 of the cset back to this bug report for you to review.

Currently it fails to pass tests in test/index.tst.  For example:


abc = [1 2];
abc(-1)(1)(1)
error: abc<...>(-1): subscripts must be either integers 1 to (2^31)-1 or logicals

Expected: "abc(-1): subscripts"
Observed: "abc<...>(-1): subscripts"


It seems like for multiple levels of indexing where it is the first instance that is incorrect it would be nice to not use the '<...>' notation.  But if that is hard then we could certainly skip it.



(file #35753)

Rik <rik5>
Group administrator
Tue 15 Dec 2015 12:44:39 AM UTC, comment #9: 

I generally don't like the idea of returning const reference.  Currently that would work because the object being returned is a tree_index_expression data member, so it will be live for as long as expr is (in the context we are discussing).  But what if tree_index_expression::arg_names were changed so that instead of returning a data member, it created a temporary value to return?  Then I don't think returning a const reference to a temporary would help you.  In that case I don't think there would be any guarantee that the reference would be valid after the arg_names method returns.  So please leave it returning a std::list<> object and just make a temporary that has the lifetime you need.

John W. Eaton <jwe>
Group administrator
Mon 14 Dec 2015 07:57:42 PM UTC, comment #8: 

The third patch works fine.

Regarding the coding question, none of the code is performance sensivitive since it will only be hit if an error has occurred and we are returning to the interactive prompt.  This says that we should just do it the way we like.  Your current patch with a temporary variable has the advantage of already being coded.  On the other hand, if it is easy not to create a temporary variable, call a bunch of constructors, get memory from the stack for the new object, etc. then we should do that.

Is the only change really to add "const" to a function declaration?

Rik <rik5>
Group administrator
Mon 14 Dec 2015 06:30:41 AM UTC, comment #7: 

Rik, sorry for being unresponsive -- gmail was treating everything from this as junk mail, so I didn't notice your update.

The bug was that


 ...::const_iterator field = expr->arg_names ().begin ()


was referring to a transient value, since arg_names() returns by value instead of reference.  I'm sure I had tested this case (you can see it in the BIST test as c.fred.foo.bar(-1).), but somehow the transient value must have been hanging around long enough for the iteration.

I now assign the value to a temporary variable.  Would it be better to return by (const) reference instead?

Also, thanks for the instructions on writing commit messages.

(file #35738)

Lachlan Andrew <lachlan>
Fri 04 Dec 2015 06:05:43 PM UTC, comment #6: 

@Lachlan: I went ahead and made the changes that revert the bad indexing messages to use a colon rather than a semicolon.  See http://hg.savannah.gnu.org/hgweb/octave/rev/ba2367658dc8.

That solves the original issue of this bug report.  However I can't apply the other part of your patch because it leads to segfaults.  I am changing this issue report to a Feature Request and changing the title to "Bad indexing could produce more informative error messages".

Rik <rik5>
Group administrator
Tue 01 Dec 2015 04:17:28 PM UTC, comment #5: 

@Lachlan: Any idea why your patch produces a segfault?  I'd like to apply it and close this report, but we need it to be clean.

Rik <rik5>
Group administrator
Fri 27 Nov 2015 11:19:01 AM UTC, comment #4: 

I reviewed your patch and have attached an updated version to the report.  jwe has a specific style he likes in the commit messages so you can see the format I used there.  The only other change was the variable name 'ex' to 'expr' because that was the common name used elsewhere in the code.  It seemed worthwhile to go with that convention since 'ex' could potentially also mean exception and there was an exception input to the function.

One thing is not perfect, however, I now get a segfault for the following sample code.


octave:1> x.foo.bar.baz = 1;
octave:2> x.foo.bar.baz(2)
panic: Segmentation fault -- stopping myself...
attempting to save variables to 'octave-workspace'...
save to 'octave-workspace' complete
Segmentation fault




(file #35575)

Rik <rik5>
Group administrator
Fri 27 Nov 2015 03:17:26 AM UTC, comment #3: 

The attached patch considers all cases with a single numeric index (e.g., x.foo(-1), x.foo.bar(-1), x.foo(-1,1).bar), but gives something generic for anything with two numeric indices.

The code would be slightly simpler if we lump together anything with multiple levels, but a the risk of slight code bloat, I'd go for keeping it.  (I wrote it before I saw your suggestion.)  Let me know if you want it trimmed.

L

(file #35574)

Lachlan Andrew <lachlan>
Thu 26 Nov 2015 03:57:52 PM UTC, comment #2: 

@Lachlan: I like trying to give the programmer more help in finding their errors, particularly as lots of the people using Octave are not professional programmers but scientists, engineers, or other types.

I think your change helps quite a bit in that regard, but I did object to the ';'. ;)

I don't think we need to solve this perfectly.  Before they were getting no information, now they will be getting some, but trying to re-execute bits of the parser seems like overkill.

I would say that if you can easily distinguish between the cases of 0 sublevels of indexing and more than 0 then have just two error messages.


x(-1)
error: x(-1); subscripts must be either integers 1 to (2^31)-1 or logicals
x.foo(-1);
error: x.<...>(-1): subscripts must be either integers 1 to (2^31)-1 or logicals
x.foo(1).bar(-1);
error: x.<...>(-1): subscripts must be either integers 1 to (2^31)-1 or logicals



Rik <rik5>
Group administrator
Thu 26 Nov 2015 03:23:19 AM UTC, comment #1: 

Yes, the change was intentional.

I'll fix the ';' once I write a patch for a more serious bug.  In dev, it gives


octave:1> x.foo = 1:5
octave:2> x.foo(-1)
error: x(-1); subscripts must be either integers 1 to (2^31)-1 or logicals


which omits the ".foo".  That isn't too bad, but if we use "x.foo(n).bar(m)", it will just say "x(-1)" for either n or m being -1.  Do you have suggestions on how to handle that?

It seems overkill to re-execute the indexing (which may also give different results, if ++, or -= were used).  Would it be better to revert to a generic message in those cases?

For the x.foo(-1) case, would it be better to trace one level down in the execution, or perhaps report  x.<...>(-1)?

Lachlan Andrew <lachlan>
Wed 25 Nov 2015 11:57:13 PM UTC, original submission:  

With a recent development tip (20759:2892f62fb37c)


octave:4> x = 1:5
x =

   1   2   3   4   5

octave:5> x(-1)
error: x(-1); subscripts must be either integers 1 to (2^31)-1 or logicals
octave:5> x(10)
error: x(10); out of bound 5


The same code run on the default branch results in


octave:3> x = 1:5
x =

   1   2   3   4   5

octave:4> x(-1)
error: subscript indices must be either positive integers less than 2^31 or logicals
octave:4> x(10)
error: A(I): index out of bounds; value 10 out of bound 5
octave:4> diary off


Maybe the change is intended, but I prefer using ':' to break parts of the error message up rather than ';'.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #35777:  bug_46536.v5.cset added by lachlan (13KiB - application/octet-stream)
file #35753:  bug_46536.v4.cset added by rik5 (5KiB - application/octet-stream)
file #35738:  bug_46536.v3.cset added by lachlan (5KiB - application/octet-stream)
file #35575:  bug_46536.v2.cset added by rik5 (13KiB - application/octet-stream)
file #35574:  patch_46536.txt added by lachlan (13KiB - 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 jwe (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by rik5 (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-12-17 lachlan Attached File- Added bug_46536.v5.cset, #35777
    2015-12-15 rik5 Attached File- Added bug_46536.v4.cset, #35753
    2015-12-14 lachlan Attached File- Added bug_46536.v3.cset, #35738
    2015-12-04 rik5 Item GroupRegression Feature Request
        Summarybad indexing produces different error message in dev than default Bad indexing could produce more informative error messages
    2015-11-27 rik5 Attached File- Added bug_46536.v2.cset, #35575
    2015-11-27 lachlan Attached File- Added patch_46536.txt, #35574
    2015-11-26 rik5 StatusNone In Progress

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code