Using index() on the BlockBase.content list does not work

[arporter]

I've fallen over this twice now. I think the problem occurs because Base (and therefore BlockBase) inherit from ComparableMixin which re-defines the comparison operators. As a result, using my_node.content.index(other_node) fails to find other_node, even if it exists in my_node.content.
A possible solution to this is to make content an instance of some custom list class that puts back the correct index functionality (e.g. using is to compare the search node with the members of content).

[arporter]

@rupertford, if you're happy with my suggested solution then I'm happy to take this one.

[rupertford]

I had a similar problem when integrating fparser2 into PSyclone for parsing the algorithm layer. Python 3 does not allow you to use an object as a dictionary key if it defines its own equals operator (but Python 2 does not care). My workaround was to avoid using an fparser2 instance as a dictionary key, but that obviously did not solve the underlying issue.

[rupertford]

I don't think I understand enough to know whether your suggested solution is good or not.

The mixin class means that all equivalence operations for fparser2 are based on the values in the objects rather than the objects themselves being the same. I'm not sure if this is the behaviour we (or others) want, or expect. Do you have any thoughts?

One option would be to remove the mixin class. I don't think it is required for fparser2 to function correctly. This would mean all equivalences would be based on whether the instances were the same or not. This is what I would expect to happen when traversing a tree of objects, so that would be my (perhaps naive) preference.

[arporter]

Ah! I was assuming the mixin class was required. If it isn't then we should get rid of it!

[arporter]

ComparableMixin compares two instances based on their _cmpkey properties. Base defines _cmpkey to return self.items.

[arporter]

If I replace ComparableMixin with an empty class then we get some test failures:

FAILED tests/test_fortran2003.py::test_proc_component_def_stmt - fparser.two.utils.NoMatchErro...
FAILED tests/test_utils.py::test_endstmtbase_match - AssertionError: assert ('SUBROUTINE', Nam...
FAILED tests/fortran2003/test_component_part_r438.py::test_proc_component_part[-pointer, nopass]
FAILED tests/fortran2003/test_component_part_r438.py::test_proc_component_part[real_func-pointer]
FAILED tests/fortran2003/test_component_part_r438.py::test_proc_component_part[real_func-pointer, pass]
FAILED tests/fortran2003/test_entity_decl_r504.py::test_entity_decl_name - AssertionError: ass...
FAILED tests/fortran2003/test_function_stmt_r1224.py::test_function_get_name - AssertionError:...
FAILED tests/fortran2003/test_module_r1104.py::test_module_get_name - AssertionError: assert N...
FAILED tests/fortran2003/test_program_stmt_r1102.py::test_get_name - AssertionError: assert Na...
FAILED tests/fortran2003/test_subroutine_stmt_r1232.py::test_subroutine_get_name - AssertionEr...

Alternatively, if I change the implementation of Base._cmpkey to include type(self) in the tuple that it returns then all tests pass and the unexpected equality in #400 is removed.

[arporter]

Many of the test failures are because comparing two different instances of Name() now returns False, even if the text is the same:

-> assert obj.get_name() == Name("foo")
(Pdb) obj.get_name()
Name('foo')

As in the PSyIR, testing two nodes for equality could present some subtleties.

[reuterbal]

I haven't looked into this in detail, so please disregard if this sounds stupid:
Fparser2 often "shortcuts" to "equivalent" nodes, Name and all the many variations that the standard defines being only one example. The question therefore is, if something like Subroutine_Name('func') == Name('func') should compare equal or not?
If they should be the same, wouldn't it suffice to keep Base._cmpkey as is and instead add a distinguishing value to the _cmpkey for nodes that don't have items (such as ELSE, RETURN) etc., such as the Fortran keyword itself?