Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Literals not being rebuilt causing shallow copy issues #379

Open
Andrew-Beggs-ECMWF opened this issue Sep 23, 2024 · 1 comment
Open

Literals not being rebuilt causing shallow copy issues #379

Andrew-Beggs-ECMWF opened this issue Sep 23, 2024 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@Andrew-Beggs-ECMWF
Copy link
Contributor

Loki seems to be shallow copying literals by not rebuilding the leaf nodes due to the assumption they are constant. @reuterbal identified the issue being due to the code at https://github.com/ecmwf-ifs/loki/blob/main/loki/expression/mappers.py#L552-L563

The reproducer for this issue is as follows:

from loki import Sourcefile, FindNodes, Loop, IntLiteral, SubstituteExpressions

source = Sourcefile.from_file('bar.F90', preprocess=True)
for routine in source.routines:
  o = FindNodes(Loop).visit(routine.body)[0]

  body_a = SubstituteExpressions().visit(o.body)
  body_b = SubstituteExpressions().visit(o.body)
  assert body_a[0].rhs.children[0] == body_b[0].rhs.children[0]     # Compare equal...
  assert body_a[0].rhs.children[0] is not body_b[0].rhs.children[0] # ...but not the same object
subroutine bar()
  integer :: c

  do i = 1, 5
    c = 1 + 3
  end do

end subroutine bar
@reuterbal reuterbal added the bug Something isn't working label Oct 17, 2024
@reuterbal
Copy link
Collaborator

As a follow-up, here's a bunch of tests that expose the shallow copy problems on a pure expression tree level:

from loki.expression import (
    symbols as sym, ExpressionRetriever, parse_expr, SubstituteExpressionsMapper
)


def test_substitute_expression_mapper():
    expr_str = '5 * a + 4 * b(c) + a'
    expr = parse_expr(expr_str)
    assert expr == expr_str

    # Create a clone of the expr by replacing 'a' with 'x'
    retriever = ExpressionRetriever(lambda e: isinstance(e, sym.TypedSymbol))
    symbols = retriever.retrieve(expr)
    assert symbols == ['a', 'b', 'c', 'a']
    expr2 = SubstituteExpressionsMapper({symbols[0]: symbols[0].clone(name='x')})(expr)
    expr2_str = expr_str.replace('a', 'x')
    assert expr == expr_str
    assert expr2 == expr2_str

    # As a first test, let's rename 'c' to 'y' in the original expression by modifying
    # the object in symbols: That should be the identical object, therefore it should
    # also affect expr but it should not change expr2
    symbols[2].name = 'y'
    assert symbols == ['a', 'b', 'y', 'a']
    expr_str = expr_str.replace('c', 'y')
    assert expr == expr_str
    assert expr2 == expr2_str  # Fails: str(expr2) is now '5*x + 4*b(y) + x'

    # Now rename the last 'a' to 'q' in the original expression in-place: That should
    # affect only the last instance of a to q but keep the first instance of a the same,
    # and should not change expr2
    assert expr.children[1] == 'a'
    expr.children[1].name = 'q'
    expr_str = expr_str[:-1] + 'q'
    assert expr == expr_str
    assert expr2 == expr2_str

    # Now do the same in expr2. If SubstituteExpressionsMapper did create unique nodes
    # again, then we should observe the same behaviour
    assert expr2.children[1] == 'x'
    expr2.children[1].name = 'q'
    expr2_str == expr2_str[:-1] + 'q'
    assert expr == expr_str
    assert expr2 == expr2_str  # Fails: str(expr2) is now '5*q + 4*b(c) + q'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants