Pylint: enable modified-iterating-list and bugfix

[levbishop]

Turned on the modified-iterating-list pylint check and used it to find a bug.
The iteration would fail to remove matching list elements if they along came two-in-a-row.
Here's a simplified illustration of the problematic idiom:

In [38]: l=[1,12,13,14,5,6,7,8,9]
    ...: for i in l:
    ...:     if i >=10:
    ...:         l.remove(i)
    ...: l
Out[38]: [1, 13, 5, 6, 7, 8, 9]

Notice that 13 makes it through despite the filter.

For this PR I put a minimal change to fix the issue, but the rest of that routine does smell a bit funny to me and it would likely benefit from a rewrite. Eg, I'm not sure what was the intention with the superfluous conditional here:

        if len(pred) > 1:
            pred.sort()

Anyway, this all relates to #9614

[qiskit-bot]

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 5371321815

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 5 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.02%) to 85.949%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/optimization/template_matching/forward_match.py	1	84.62%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	90.39%
crates/qasm2/src/lex.rs	3	90.63%

Totals
Change from base Build 5356088179:	-0.02%
Covered Lines:	71425
Relevant Lines:	83102

---

💛 - Coveralls

[jlapeyre]

Great. Might prevent a bug in the future.

[jakelishman]

It's not clear to me that this is correct, because in the lines just out of sight of this diff, pred is modified such that it's no longer the same as matches. This change could cause an IndexError to be emitted if node_id_t ends up greater than maximal_index (though I don't know if that's possible).

[jlapeyre]

You are correct. I read the lines above, but somehow forgot that remove will throw an error if the value is not present. Even if this works because of the nature of the data, I'd say it's too opaque and fragile to write it this way.

[jlapeyre]

As long as this code is being touched, it might be a good idea to rewrite it a bit. Maybe this:

        matches = [_match[0] for _match in self.match]
        pred = matches.copy()
        pred.sort()

        if direct_successors := self.template_dag_dep.direct_successors(node_id_t):
            maximal_index = direct_successors[-1]
            pred = [_match for _match in matches if _match <= maximal_index]
        else:
            pred = matches.copy()
        if node_id_t in pred:
            pred.remove(node_id_t)

@jakelishman drew attention to a problem

[joesho112358]

i think this was resolved in #12294. sorry to step on toes, but i think this can be closed!

[jakelishman]

Joe: thanks for helping tidy the old PRs up! Lev won't mind that this minor PR staled and got obsoleted (but I think he's a bit busy at the moment) - thanks for fixing it elsewhere.