bugGNU roff - Bugs: bug #60802, [troff] make .substring handle an...

 
 

bug #60802: [troff] make .substring handle an empty result

Submitter:  None
Submitted:  Sun 20 Jun 2021 03:01:26 AM UTC
   
 
Category:  Core Severity:  3 - Normal
Item Group:  Feature change Status:  None
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 02 Mar 2022 07:49:49 PM UTC, comment #6: 

This is the most significant feature change pending.  I can't promise I'll get to it for 1.23, but I'm not giving up yet.

G. Branden Robinson <gbranden>
Group administrator
Mon 05 Jul 2021 06:58:27 PM UTC, comment #5: 

Hi Branden,

I agree that changing existing expected behavior should be avoided.  With 20-20 hindsight, it might have been better to make .substring take OFFSET LENGTH parameters instead of N1 N2 (similar to Perl's "substr").  But that water reached the ocean long ago, and .substring has been documented and presumably in use in the wild for a while.

Moving forward, the cases where I ran into trouble were trying to extract or remove characters which might not exist, e.g. chop off the last char, if there are any chars, with

  .substring STR 0 -2

or delete the first char, if they are any chars, with

  .substring STR 2

I think this could work compatibly with existing semantics by allowing N1 and N2 to "refer" to positions before or after the existing string without error; if the range is negative, i.e. N2 points before N1, then the substring is empty and STR is set to "".  If the range is positive, then the substring is whatever characters exist within the range.

I will attach a new regression script for this revised proposal, including use of negative indicies which count backwards from the end (like currently, except it is allowed to refer to before the start of the string without error).

Here is revised documentation:

-- Request: .substring str n1 [n2]
     Replace the string named STR with the substring defined by the
     indices N1 and N2.  The first character in the string has index 0.
     If N1 or N2 is negative, it is counted from the end of the string,
     going backwards: The last character has index -1, the character before
     the last character has index -2, etc.  If N2 is omitted then -1 is used
     implicitly, indicating the last character, if any.

     If N2 refers to a position before N1 then the substring is empty, and
     the string named STR is set to contain "".   It is allowed for N1 or N2
     to point to positions before the start or after the end of the existing
     string; the substring includes only characters at valid positions within
     the indicated range, if any.
    
          .ds xxx abcdefgh
          .substring xxx 1 -4
          \*[xxx]
              => bcde
          .substring xxx 2
          \*[xxx]
              => de
          .substring xxx -2
          \*[xxx]
              => de
          .substring xxx 1
          \*[xxx]
              => e
          .substring xxx 1
          \*[xxx]
              => (empty)
          .substring xxx 1
          \*[xxx]
              => (empty)


(file #51649)

Jim Avera <jimav>
Mon 05 Jul 2021 06:57:07 AM UTC, comment #4: 

Here some older threads discussing .substring semantics.

The behavior was changed back in 2002; these threads discuss an earlier implementation.

https://lists.gnu.org/archive/html/groff/2001-10/msg00165.html

This one features Werner seeking stable semantics for the request, which I think we should stick to except arguably for the corner case originally raised on the mailing list this year.

https://lists.gnu.org/archive/html/groff/2002-07/msg00035.html

Here's a discussion from 2014.

https://lists.gnu.org/archive/html/groff/2014-11/msg00224.html

G. Branden Robinson <gbranden>
Group administrator
Mon 05 Jul 2021 04:44:08 AM UTC, comment #3: 

Hi Jim,

Thank you!  This is really helpful.  It's already led me to a preliminary patch that resolves every test case you presented, with one exception which you seem to have anticipated.

The use of negative indices to count backward from the end of the string is a common semantic in programming languages that support array slicing (which is basically what .substring does).

I'm not sure the groff community will be willing to give up that feature, and in-tree groff code already uses it.

Anyway, with my very first stab at a patch to src/roff/troff/input.cpp, I've got the following output:


$ bash ./substring_test_revised.bash
Exit status was 1
--- Pick a single character from non-empty ---
.substring 'abc' 0 0 -> 'a' (OK)
.substring 'abc' 1 1 -> 'b' (OK)
.substring 'abc' 2 2 -> 'c' (OK)
--- Pick multiple characters from non-empty ---
.substring 'abcd' 0 1 -> 'ab' (OK)
.substring 'abcd' 1 1 -> 'b' (OK)
.substring 'abcd' 0 3 -> 'abcd' (OK)
.substring 'abcd' 1 3 -> 'bcd' (OK)
.substring 'abcd' 2 3 -> 'cd' (OK)
.substring 'abcd' 3 3 -> 'd' (OK)
--- Omit n2 with non-empty input and non-empty result ---
.substring 'abc' 0 -> 'abc' (OK)
.substring 'abc' 1 -> 'bc' (OK)
.substring 'abc' 2 -> 'c' (OK)
.substring 'a' 0 -> 'a' (OK)
--- Specify empty substring with n2==(n1-1) ---
.substring 'abcd' 3 2 -> '' (OK)
.substring 'abcd' 2 1 -> '' (OK)
.substring 'abcd' 1 0 -> '' (OK)
ERROR: .substring 'abcd' 0 -1 yielded 'abcd', EXPECTED ''
--- Specify empty substring with n1==length and n2 omitted ---
troff: ./substring_test_revised.bash:78: warning: start and end index of substring out of range
.substring 'abcd' 4 -> '' (OK)
troff: ./substring_test_revised.bash:79: warning: start and end index of substring out of range
.substring 'abc' 3 -> '' (OK)
troff: ./substring_test_revised.bash:80: warning: start and end index of substring out of range
.substring 'ab' 2 -> '' (OK)
troff: ./substring_test_revised.bash:81: warning: start and end index of substring out of range
.substring 'a' 1 -> '' (OK)
troff: ./substring_test_revised.bash:82: warning: start and end index of substring out of range
.substring '' 0 -> '' (OK)
Aborting, got 1 errors.


Also, I don't think we should talk about "undefined behavior" in our documentation.  We are implementors, not a standards body, so we should document how .substring works.  It's okay for us to say "it is an error if...", and our Texinfo manual already has language like this in many places.

What do you think?

G. Branden Robinson <gbranden>
Group administrator
Sun 04 Jul 2021 09:32:49 PM UTC, comment #2: 

In the email list where this was reported, Brandon Robinson asked for (1) a unit test for the boundary conditions of the .substring request and (2) a precise description of its slicing semantics in our documentation.

Here is a first crack at those.   Please see attached shell script which runs a regression test (which currently fails because .substring does not yet behave in the way expected).

Here is a revised description of desired behavior:

  .substring xx n1 n2
     Replace string xx with the substring defined by zero-based indicies
     n1 and n2. Or, put another way, delete any characters outside the
     range (n1..n2).  If n2 is omitted then all characters after index
     n1 are included in the substring.

     If n1 > n2 or if n1 equals the length of the string, then nothing
     is selected and the string is set to empty if it was not already empty.

     Behavior is undefined if n1 > (n2+1) or if either index is negative.


(file #51644)

Jim Avera <jimav>
Sun 20 Jun 2021 10:03:03 AM UTC, comment #1: 
Dave <barx>
Group Member
Sun 20 Jun 2021 03:01:26 AM UTC, original submission:  

I'm trying to use .substring to remove the first character of a string, leaving behind whatever follows, even if that is nothing (i.e. the string had only one character).

  .substring str 1

or

  .length n2 "\*[str]
  .nr n2 -1
  .substring str 1 \n[n2]

(if n2 is omitted, it appears to default to length-1 as you might expect).

The above works as long as the string originally had at least two characters.  If it only has one, so the desired result is the empty string, it does not remove anything, leaving behind the 0th character and emitting a warning (with -w all).

This seems like a corner-case bug.

At first glance, it seems like it would be safe to fix this because the result is currently always wrong in this case, i.e. n1 equals the length of the string (so the result should be empty).

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51650:  substring_test_rev3b.bash added by jimav (4KiB - application/octet-stream - Minor fix (make test-case comments appear); add copyright disclaimer)
file #51649:  substring_test_rev3.bash added by jimav (4KiB - application/octet-stream)
file #51645:  substring_test.bash added by jimav (2KiB - application/octet-stream - Revised regression test script)
file #51644:  substring_test.bash added by jimav (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by jimav (Updated the item)
  • -email is unavailable- added by barx (reported the bug)
  •  

    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 logged-in users can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-05-31 gbranden StatusIn Progress None
    2022-03-02 gbranden Item GroupIncorrect behaviour Feature change
        SummaryMake .substring handle an empty result [troff] make .substring handle an empty result
    2021-07-06 gbranden StatusNeed Info In Progress
    2021-07-05 jimav Attached File- Added substring_test_rev3b.bash, #51650
    2021-07-05 jimav Attached File- Added substring_test_rev3.bash, #51649
    2021-07-05 gbranden StatusNone Need Info
        Assigned toNone gbranden
    2021-07-04 jimav Attached File- Added substring_test.bash, #51645
    2021-07-04 jimav Attached File- Added substring_test.bash, #51644
    2021-06-20 barx Carbon-CopyRemoved 93119 -
    2021-06-20 barx Carbon-Copy- Added jimav

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code