[m-rev.] for review: fix a bug in duration parsing

[Julien Fischer]

For review by anyone.

Fix a bug in duration parsing.

duration_from_string/2 incorrectly accepts strings containing duration
component designators that are not preceded by digits (e.g. "PY", "PTH") for
all designators except for S (which is handled by a different code path).
Such components are currently treated as being zero, instead of being
rejected.

Fix this by requiring that if a given component designator is present in
a duration, then it must be preceded by at least one digit.

library/calendar.m:
    Make the predicates that parse duration components semidet and have them
    fail if we encounter a component designator with an associated digit.

    Move the predicate that responsible for parsing duration components
    to their own section. Document them.

    Delete some predicates that are no longer required.

tests/hard_coded/calendar_duration_conv.{m,exp}:
    Extend this test case to cover the above.

Julien.

diff --git a/library/calendar.m b/library/calendar.m
index fa70c13e0..a85ea4ded 100644
--- a/library/calendar.m
+++ b/library/calendar.m
@@ -1477,6 +1477,30 @@ foldl3_days(Pred, !.Curr, End, !Acc1, !Acc2, !Acc3) :-
 % Parsing predicates.
 %

+:- pred read_char(char::out, list(char)::in, list(char)::out) is semidet.
+
+read_char(Char, [Char | Rest], Rest).
+
+:- pred read_int_and_num_chars(int::out, int::out,
+    list(char)::in, list(char)::out) is det.
+
+read_int_and_num_chars(Val, N, !Chars) :-
+    read_int_and_num_chars_2(0, Val, 0, N, !Chars).
+
+:- pred read_int_and_num_chars_2(int::in, int::out, int::in, int::out,
+    list(char)::in, list(char)::out) is det.
+
+read_int_and_num_chars_2(!Val, !N, !Chars) :-
+    ( if
+        !.Chars = [Char | Rest],
+        decimal_digit_to_int(Char, Digit)
+    then
+        !:Val = !.Val * 10 + Digit,
+        read_int_and_num_chars_2(!Val, !.N + 1, !:N, Rest, !:Chars)
+    else
+        true
+    ).
+
 :- pred read_microseconds(microseconds::out, list(char)::in, list(char)::out)
     is det.

@@ -1492,26 +1516,14 @@ read_microseconds(MicroSeconds, !Chars) :-
         MicroSeconds = 0
     ).

-:- pred read_int_and_num_chars(int::out, int::out,
-    list(char)::in, list(char)::out) is det.
-
-read_int_and_num_chars(Val, N, !Chars) :-
-    read_int_and_num_chars_2(0, Val, 0, N, !Chars).
-
-:- pred read_int_and_num_chars_2(int::in, int::out, int::in, int::out,
-    list(char)::in, list(char)::out) is det.
-
-read_int_and_num_chars_2(!Val, !N, !Chars) :-
-    ( if
-        !.Chars = [Char | Rest],
-        decimal_digit_to_int(Char, Digit)
-    then
-        !:Val = !.Val * 10 + Digit,
-        read_int_and_num_chars_2(!Val, !.N + 1, !:N, Rest, !:Chars)
-    else
-        true
-    ).
-
+%---------------------------------------------------------------------------%
+%
+% Duration parsing predicates.
+%
+
+    % Read an optional leading minus sign. Sign is -1 if one is present (and is
+    % consumed), and 1 otherwise (consuming nothing).
+    %
 :- pred read_sign(int::out, list(char)::in, list(char)::out) is det.

 read_sign(Sign, !Chars) :-
@@ -1522,35 +1534,73 @@ read_sign(Sign, !Chars) :-
         Sign = 1
     ).

-:- pred read_char(char::out, list(char)::in, list(char)::out) is semidet.
-
-read_char(Char, [Char | Rest], Rest).
-
-:- pred read_years(int::out, list(char)::in, list(char)::out) is det.
+    % read_years, read_months, read_days, read_hours and read_minutes each read
+    % one optional duration component using read_int_and_char_or_zero, with the
+    % designator character 'Y', 'M', 'D', 'H' and 'M' respectively.
+    % Months and minutes share the designator 'M', and are distinguished only
+    % by position: months are read before the 'T' separator and minutes after
+    % it.
+    %
+:- pred read_years(int::out, list(char)::in, list(char)::out) is semidet.

 read_years(Years, !Chars) :-
     read_int_and_char_or_zero(Years, 'Y', !Chars).

-:- pred read_months(int::out, list(char)::in, list(char)::out) is det.
+:- pred read_months(int::out, list(char)::in, list(char)::out) is semidet.

 read_months(Months, !Chars) :-
     read_int_and_char_or_zero(Months, 'M', !Chars).

-:- pred read_days(int::out, list(char)::in, list(char)::out) is det.
+:- pred read_days(int::out, list(char)::in, list(char)::out) is semidet.

 read_days(Days, !Chars) :-
     read_int_and_char_or_zero(Days, 'D', !Chars).

-:- pred read_hours(int::out, list(char)::in, list(char)::out) is det.
+:- pred read_hours(int::out, list(char)::in, list(char)::out) is semidet.

 read_hours(Hours, !Chars) :-
     read_int_and_char_or_zero(Hours, 'H', !Chars).

-:- pred read_minutes(int::out, list(char)::in, list(char)::out) is det.
+:- pred read_minutes(int::out, list(char)::in, list(char)::out) is semidet.

 read_minutes(Minutes, !Chars) :-
     read_int_and_char_or_zero(Minutes, 'M', !Chars).

+    % read_int_and_char_or_zero(Int, Char, !Chars):
+    %
+    % Read one optional "<integer><Char>" duration component from the front of
+    % !.Chars, where <integer> is a run of decimal digits.
+    % If !.Chars begins with Char preceded by at least one digit, then set Int
+    % to the value of those digits and !:Chars to the characters after Char.
+    % If !.Chars begins with Char with no digits before it, then fail.
+    % Otherwise, set Int to zero and leave !:Chars unchanged.
+    %
+:- pred read_int_and_char_or_zero(int::out, char::in,
+    list(char)::in, list(char)::out) is semidet.
+
+read_int_and_char_or_zero(Int, Char, !Chars) :-
+    ( if
+        read_int_and_num_chars(Int0, NumDigits, !.Chars, Chars1),
+        Chars1 = [Char | Rest]
+    then
+        NumDigits > 0,
+        !:Chars = Rest,
+        Int = Int0
+    else
+        Int = 0
+    ).
+
+    % read_seconds_and_microseconds(Seconds, MicroSeconds, !Chars):
+    %
+    % Read the optional seconds component from the front of !.Chars: a run of
+    % at least one decimal digit, an optional fractional part, and the
+    % designator 'S' (for example "9S" or "9.0003S").
+    % If !.Chars begins with such a component, then set Seconds to the whole
+    % part, MicroSeconds to the fractional part expressed in microseconds, and
+    % !:Chars to the characters after the 'S'.
+    % Otherwise, set both Seconds and MicroSeconds to zero and leave !:Chars
+    % unchanged.
+    %
 :- pred read_seconds_and_microseconds(seconds::out, microseconds::out,
     list(char)::in, list(char)::out) is det.

@@ -1569,38 +1619,6 @@ read_seconds_and_microseconds(Seconds,
MicroSeconds, !Chars) :-
         MicroSeconds = 0
     ).

-:- pred read_int_and_char_or_zero(int::out, char::in,
-    list(char)::in, list(char)::out) is det.
-
-read_int_and_char_or_zero(Int, Char, !Chars) :-
-    ( if
-        read_int(Int0, !.Chars, Chars1),
-        Chars1 = [Char | Rest]
-    then
-        !:Chars = Rest,
-        Int = Int0
-    else
-        Int = 0
-    ).
-
-:- pred read_int(int::out, list(char)::in, list(char)::out) is det.
-
-read_int(Val, !Chars) :-
-    read_int_2(0, Val, !Chars).
-
-:- pred read_int_2(int::in, int::out, list(char)::in, list(char)::out) is det.
-
-read_int_2(!Val, !Chars) :-
-    ( if
-        !.Chars = [Char | Rest],
-        decimal_digit_to_int(Char, Digit)
-    then
-        !:Val = !.Val * 10 + Digit,
-        read_int_2(!Val, Rest, !:Chars)
-    else
-        true
-    ).
-
 %---------------------------------------------------------------------------%
 :- end_module calendar.
 %---------------------------------------------------------------------------%
diff --git a/tests/hard_coded/calendar_duration_conv.exp
b/tests/hard_coded/calendar_duration_conv.exp
index a2f31333d..86ada02b6 100644
--- a/tests/hard_coded/calendar_duration_conv.exp
+++ b/tests/hard_coded/calendar_duration_conv.exp
@@ -270,3 +270,21 @@ PASS: ERROR DETECTED

 "P1DT":                             T with no time components
 PASS: ERROR DETECTED
+
+"PY":                               Y with no year digits
+PASS: ERROR DETECTED
+
+"P5YM":                             M with no month digits
+PASS: ERROR DETECTED
+
+"P5YD":                             D with no day digits
+PASS: ERROR DETECTED
+
+"PTH":                              H with no hour digits
+PASS: ERROR DETECTED
+
+"PT5HM":                            M with no minute digits
+PASS: ERROR DETECTED
+
+"PTS":                              S with no second digits
+PASS: ERROR DETECTED
diff --git a/tests/hard_coded/calendar_duration_conv.m
b/tests/hard_coded/calendar_duration_conv.m
index e79f9a35f..68ac43837 100644
--- a/tests/hard_coded/calendar_duration_conv.m
+++ b/tests/hard_coded/calendar_duration_conv.m
@@ -270,7 +270,15 @@ invalid_durations = [
     duration_test("letter in time component", "PT1HxM"),

     % T present but no time components follow.
-    duration_test("T with no time components", "P1DT")
+    duration_test("T with no time components", "P1DT"),
+
+    % Component designators with missing digits.
+    duration_test("Y with no year digits", "PY"),
+    duration_test("M with no month digits", "P5YM"),
+    duration_test("D with no day digits", "P5YD"),
+    duration_test("H with no hour digits", "PTH"),
+    duration_test("M with no minute digits", "PT5HM"),
+    duration_test("S with no second digits", "PTS")
 ].

 %---------------------------------------------------------------------------%