A slightly more complicated version of the situation in
https://github.com/github/codeql/pull/2507 could cause the `toString`
calculation to diverge. Although the previous PR took tuples nested
inside tuples into account (and subscripted types cannot be nested
inside each other in our modelling), it did not account for having
this nesting be interleaved, and this is what caused the divergence.
I have not done the usual "test case first to show the problem
exists", since this would also diverge and take forever to fail. The
instance observed in `scipy` was likely caused by something akin to
```python
x = ()
while True:
x = x[(x,)]
```
Finally, to prevent this from happening with other types, I went
through and checked each instance where the string representation of
an `ObjectInternal` might potentially contain a reference to
itself (and thus explode). I encapsulated this in a
`bounded_toString` helper predicate, and used this in all the cases
where I was able to determine that the above _could_ happen.
This required a minor change in the type tracker implementation, but
apart from that no other changes appear to be needed. Seems to clean
up the test output quite a bit.
Since predicate name `import` is not allowed, I adopted `importNode` as it sort
of matches what `exprNode` does.
---
Due to only using `importMember` in `os_attr` we previously didn't handle
`import os.path as alias` :|
I did creat a hotfix for this (https://github.com/github/codeql/pull/4446), but
in doing so I realized the core of the problem: We're exposing ourselves to
making these kinds of mistakes by having BOTH importModule and importMember, and
we don't really gain anything from doing this!
We do loose the ability to easily only modeling `from mod import val` and not
`import mod.val`, but I don't think that will ever be relevant.
This change will also make us to recognize some invalid code, for example in
import os.system as runtime_error
we would now model that `runtime_error` is a reference to the `os.system`
function (although the actual import would result in a runtime error).
Overall these are tradeoffs I'm willing to make, as it does makes things simpler
from a QL modeling point of view, and THAT sounds nice 👍
Required a small change in `DataFlow::importModule` to get the desired
behaviour (cf. the type trackers defined in `moduleattr.ql`, but this
should be harmless. The node that is added doesn't have any flow
anywhere.
