Repository: stfc/fparser

Issue:
Expressions containing -ve exponents cannot be parsed
master is currently broken. e.g. the Fortran `zMv = zSST+1.33d-3*zSST` causes a syntax error. If I change the numerical constant to `1.33d3` instead then all is fine. This may be related to the recent bug fix for the slow parsing of expressions @rupertford?

Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index de0a4d9..58aa14c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -15,7 +15,11 @@ Modifications by (in alphabetical order):
 * P. Vitt, University of Siegen, Germany
 * A. Voysey, UK Met Office
 
-20/01/2021 PR #286 FOR #284. Adds checking for datatype of *_Expr classes.
+20/01/2021 PR #289 for #288. Bug fix for matching expressions containing
+           real literals with signed exponents. This bug was introduced
+           by #285.
+
+20/01/2021 PR #286 for #284. Adds checking for datatype of *_Expr classes.
 
 18/01/2021 PR #287 for #280. Fixes overly-deep recursion when reading
            multi-line comments.
diff --git a/doc/developers_guide.rst b/doc/developers_guide.rst
index 5120e64..b34125b 100644
--- a/doc/developers_guide.rst
+++ b/doc/developers_guide.rst
@@ -1,4 +1,4 @@
-..  Copyright (c) 2017-2020 Science and Technology Facilities Council.
+..  Copyright (c) 2017-2021 Science and Technology Facilities Council.
 
     All rights reserved.
 
@@ -818,6 +818,118 @@ a valid Fortran program.
    # +++++++++++++
    # TBD
 
+Expression matching
++++++++++++++++++++
+
+The Fortran2003 rules specify a hierarchy of expressions (specified in
+levels). In summary::
+
+    R722 expr is [ expr defined-binary-op ] level-5-expr
+    R717 level-5-expr is [ level-5-expr equiv-op ] equiv-operand
+    R716 equiv-operand is [ equiv-operand or-op ] or-operand
+    R715 or-operand is [ or-operand and-op ] and-operand
+    R714 and-operand is [ not-op ] level-4-expr
+    R712 level-4-expr is [ level-3-expr rel-op ] level-3-expr    
+    R710 level-3-expr is [ level-3-expr concat-op ] level-2-expr
+    R706 level-2-expr is [[level-2-expr] add_op ] add-operand
+    R705 add-operand is [ add-operand mult-op ] mult-operand
+    R704 mult-operand is level-1-expr [ power-op mult-operand ]
+    R702 level-1-expr is [ defined-unary-op ] primary
+
+As can hopefully be seen, the "top level" rule is `expr`, this depends
+on a `level-5_expr`, which depends on an `equiv-operand` and so on in
+a hierarchy in the order listed.
+
+Fparser2 naturally follows this hierarchy, attempting to match in the
+order specified. This works well apart from one case, which is the
+matching of a Level-2 expression::
+
+    R706 level-2-expr is [[level-2-expr] add_op ] add-operand
+
+The problem is to do with falsely matching an exponent in a
+literal. Take the following example::
+
+    a - 1.0e-1
+
+When searching for a match, the following pattern is a valid candidate
+and will be the candidate used in fparser2 as fparser2 matches from the
+right hand side of a string by default::
+
+    level-2-expr = "a - 1.0e"
+    add-op = "-"
+    add-operand = "1"
+
+As expected, this would fail to match, due to the level-2 expression
+("a - 1.0e") being invalid. However, once R706 failed to match it
+would not be called again as fparser2 follows the rule hierarchy
+mentioned earlier. Therefore fparser2 would fail to match this string.
+
+To solve the problem for this specific case fparser2 includes
+additional code when implementing R706. There is an optional `is_add`
+argument in `BinaryOpBase` which is set to `True` in the
+`Level_2_Expr` class. This argument causes the `rsplit` method in the
+`pattern` instance to try to match a real literal constant on the
+right hand side of the string. If a literal constant exists then the
+exponent is ignored and the correct `+` or `-` is matched
+successfully::
+
+    level-2-expr = "a"
+    add-op = "-"
+    add-operand = "1.0e-1"
+
+However, this aproach only works if the real literal constant is on
+the right hand side of the string. Consider::
+
+    a - 1.0e-1 * b
+
+In this case the attempted match would be::
+
+    level-2-expr = "a - 1.0e"
+    add-op = "-"
+    add-operand = "1 * b"
+
+This would fail as `level-2-expr` is not valid and fparser2 would
+proceed to try to match with::
+
+    R705 add-operand is [ add-operand mult-op ] mult-operand
+
+This would attempt to match the following::
+
+    add-operand = "a - 1.0e-1"
+    mult-op = "*"
+    mult-operand = "b"
+
+This looks good, but unfortunately the `add-operand` part of the
+string ("a - 1.0e-1") would fail to match as R705 is lower in the
+hierarchy than R706, so R706 will not be called again.
+
+To solve this problem, R705 has been modified in the fparser2 implementation to::
+
+    R705 add-operand is [ level-2-expr mult-op ] mult-operand
+
+By replacing `add-operand` with `level-2-expr` we allow the "a -
+1.0e-1" to be matched with R706, i.e. we allow fparser2 to jump back up
+the rule hierarchy to fix the fallout from missing the original match.
+
+We now end up with something that can be matched correctly due to the
+`is_add` fix in rule R706.
+
+In combination these two modifications can cope with the problem of
+falsely matching an exponent in a literal.
+
+There are at least two other potential solutions which are probably
+preferable to the current solution, with the 2nd likely to be the most
+robust.
+
+1: R706 could be implemented so that it never matched an exponent in a
+literal. This would require a robust way of determining whether the
+"-" or "+" was an exponent in a literal.
+
+2: R706 could be implemented so that it tried to match a second time
+if the first match failed, using the next "+" or "-" operator found in
+the string to the left of the first one.
+
+
 Continuous Integration
 ----------------------
 
diff --git a/src/fparser/two/Fortran2003.py b/src/fparser/two/Fortran2003.py
index 2f505d7..a0f0c58 100644
--- a/src/fparser/two/Fortran2003.py
+++ b/src/fparser/two/Fortran2003.py
@@ -4747,19 +4747,53 @@ class Mult_Operand(BinaryOpBase):  # R704
     match = staticmethod(match)
 
 
-class Add_Operand(BinaryOpBase):  # R705
-    """
-    <add-operand> = [ <add-operand> <mult-op> ] <mult-operand>
-    <mult-op>  = *
-                 | /
-    """
+class Add_Operand(BinaryOpBase):  # pylint: disable=invalid-name
+    '''Fortran 2003 rule R705
+
+    add-operand is [ add-operand mult-op ] mult-operand
+
+    Rule R705 is implemented in two parts, the first with the optional
+    part included (in the match method for this class) and the second
+    without the optional part (specified in subclass_names).
+
+    Note rule R708 (mult-op is * or /) is implemented directly here as
+    the mult_op pattern.
+
+    Rule R705 specifies matching using 'add-operand', however this
+    implementation uses Level_2_Expr instead. The reason for this is
+    due to the potential to accidentally match a negative exponent as
+    the minus sign in a level-2-expr. If this happens then it is
+    possible to end up matching a * or / (a level 1 expression) before
+    matching a valid + or - which would normally result in no match
+    overall as * or / are matched after + or -. By matching with
+    Level_2_Expr, this allows us to match with a * or / and then a +
+    or - afterwards. A particular example is "a + 1.0e-1 * c", where
+    (rightly) failing to match on the "-" leads us to try to match on
+    the "*" which then fails to match on the + (as + and - have
+    already been tested).
+
+    '''
     subclass_names = ['Mult_Operand']
-    use_names = ['Add_Operand', 'Mult_Operand']
+    use_names = ['Level_2_Expr', 'Mult_Operand']
 
+    @staticmethod
     def match(string):
-        return BinaryOpBase.match(
-            Add_Operand, pattern.mult_op.named(), Mult_Operand, string)
-    match = staticmethod(match)
+        '''Implement the matching for the add-operand rule. Makes use of the
+        pre-defined mult_op pattern and the BinaryOpBase baseclass.
+
+        :param str string: the string to match.
+
+        :returns: a tuple of size 3 containing an fparser2 class \
+            instance matching a level-2-expr expression, a string \
+            containing the matched operator and an fparser2 class \
+            instance matching a mult-operand if there is a match, or \
+            None if there is not.
+        :rtype: (subclass of :py:class:`fparser.two.utils.Base`, str, \
+            subclass of :py:class:`fparser.two.utils.Base`) or NoneType
+
+        '''
+        return  BinaryOpBase.match(
+            Level_2_Expr, pattern.mult_op.named(), Mult_Operand, string)
 
 
 class Level_2_Expr(BinaryOpBase):  # R706
@@ -4792,9 +4826,9 @@ class Level_2_Unary_Expr(UnaryOpBase):  # R706.c
             pattern.add_op.named(), Add_Operand, string)
     match = staticmethod(match)
 
-# R707: <power-op> = **
-# R708: <mult-op> = * | /
-# R709: <add-op> = + | -
+# R707: power-op is **
+# R708: mult-op is * or /
+# R709: add-op is + or -
 
 
 class Level_3_Expr(BinaryOpBase):  # R710


Test patch:
diff --git a/src/fparser/two/tests/fortran2003/test_add_operand_r705.py b/src/fparser/two/tests/fortran2003/test_add_operand_r705.py
new file mode 100644
index 0000000..1775885
--- /dev/null
+++ b/src/fparser/two/tests/fortran2003/test_add_operand_r705.py
@@ -0,0 +1,107 @@
+# Copyright (c) 2021 Science and Technology Facilities Council
+
+# All rights reserved.
+
+# Modifications made as part of the fparser project are distributed
+# under the following license:
+
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+
+# 1. Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+
+# 2. Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+
+# 3. Neither the name of the copyright holder nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+'''Test Fortran 2003 rule R705 : This file tests support for the
+Add_Operand class.
+
+'''
+
+import pytest
+from fparser.two.utils import NoMatchError
+from fparser.two.Fortran2003 import Add_Operand
+from fparser.two.parser import ParserFactory
+# This is required to setup the fortran2003 classes (when matching
+# with Add_Operand)
+_ = ParserFactory().create(std="f2003")
+
+
+@pytest.mark.parametrize("string,str_repr", [
+    ("a ** b", "Mult_Operand(Name('a'), '**', Name('b'))"),
+    (".DEFINED. a", "Level_1_Expr('.DEFINED.', Name('a'))"),
+    ("1.0", "Real_Literal_Constant('1.0', None)"),
+    ("(a + b)", "Parenthesis('(', Level_2_Expr(Name('a'), '+', "
+     "Name('b')), ')')")])
+def test_mult(string, str_repr):
+    '''Test for a successful match with a valid mult-operand string'''
+    result = Add_Operand(string)
+    assert str(result) == string
+    assert repr(result) == str_repr
+
+
+def test_mult_fail():
+    '''Test for a failure to match an invalid multi-operand string'''
+    with pytest.raises(NoMatchError) as info:
+        Add_Operand("a .and. b")
+    assert "Add_Operand: 'a .and. b'" in str(info.value)
+
+
+@pytest.mark.parametrize("string,str_repr", [
+    ("a * b", "Add_Operand(Name('a'), '*', Name('b'))"),
+    ("a / b", "Add_Operand(Name('a'), '/', Name('b'))"),
+    ("a * b * c", "Add_Operand(Add_Operand(Name('a'), '*', Name('b')), "
+     "'*', Name('c'))"),
+    ("a * b / c", "Add_Operand(Add_Operand(Name('a'), '*', Name('b')), "
+     "'/', Name('c'))")])
+def test_add_operand(string, str_repr):
+    '''Test for a successful match with a valid add_operand'''
+    result = Add_Operand(string)
+    assert str(result) == string
+    assert repr(result) == str_repr
+
+
+@pytest.mark.parametrize("string,str_repr", [
+    ("a + b * c", "Add_Operand(Level_2_Expr(Name('a'), '+', Name('b')), "
+     "'*', Name('c'))"),
+    ("a + 1.0E-1 * c", "Add_Operand(Level_2_Expr(Name('a'), '+', "
+     "Real_Literal_Constant('1.0E-1', None)), '*', Name('c'))")])
+def test_level_2_match(string, str_repr):
+    '''Test that the level_2_expr class match allows a "+" or "-" operator
+    to be matched after a "*" or "/" operator if the former is to the
+    left of the latter. This would not be the case according to the
+    Fortran2003 rules, however it is needed to recover from a false
+    match on the '-' of an exponent.
+
+    '''
+    result = Add_Operand(string)
+    assert str(result) == string
+    assert repr(result) == str_repr
+
+
+@pytest.mark.parametrize("string", ["a + b", "_ * b", "a * _",
+                                    "a * b + c"])
+def test_add_operand_fail(string):
+    '''Test for a failure to match an invalid add-operand string'''
+    with pytest.raises(NoMatchError) as info:
+        Add_Operand(string)
+    assert "Add_Operand: '{0}'".format(string) in str(info.value)
diff --git a/src/fparser/two/tests/test_fortran2003.py b/src/fparser/two/tests/test_fortran2003.py
index 92fb75d..de88187 100644
--- a/src/fparser/two/tests/test_fortran2003.py
+++ b/src/fparser/two/tests/test_fortran2003.py
@@ -1797,27 +1797,6 @@ def test_mult_operand():  # R704
     assert str(obj) == '0.0E-1'
 
 
-def test_add_operand():  # R705
-
-    tcls = Add_Operand
-    obj = tcls('a*b')
-    assert isinstance(obj, tcls), repr(obj)
-    assert str(obj) == 'a * b'
-    assert repr(obj) == "Add_Operand(Name('a'), '*', Name('b'))"
-
-    obj = tcls('a/b')
-    assert isinstance(obj, tcls), repr(obj)
-    assert str(obj) == 'a / b'
-
-    obj = tcls('a**b')
-    assert isinstance(obj, Mult_Operand), repr(obj)
-    assert str(obj) == 'a ** b'
-
-    obj = tcls('0.0E-1')
-    assert isinstance(obj, Real_Literal_Constant), repr(obj)
-    assert str(obj) == '0.0E-1'
-
-
 def test_level_2_expr():  # R706
 
     tcls = Level_2_Expr