Python: Add support for __all__ assigned to tuple

[RasmusWL]

I ended up needing this for some other work I'm doing, but thought it would be easier to handle in its own PR.

Although this is an improvement, we're still not handling __all__ perfectly, for example we don't model bar being part of __all__ in all_dynamic.py, don't handle flow through intermediate variable temp in all_dynamic2.py, and we don't support any sequence (only tuple/list).

I did not add a change note, since I wasn't sure whether we would want that or not for a change of this size. Let's discuss 😊

[yoff]

I would rename all_dynamic2 to all_indirect as it does not feel dynamic in the same way as all_dynamic.

It might also be good to add a value starting with underscore to no_all if we at some point start testing what is actually in scope.

[yoff]

Not sure about the change note. It does change results, so should perhaps be there, but it is also a very small change..

[RasmusWL]

I would rename all_dynamic2 to all_indirect as it does not feel dynamic in the same way as all_dynamic.

It might also be good to add a value starting with underscore to no_all if we at some point start testing what is actually in scope.

Sure, both good ideas 👍

[tausbn]

Given how things went, performance-wise, the last time we messed with modules, I'd like to see a performance comparison even if this is a minor change.

Apart from that, this LGTM. 👍

[RasmusWL]

Given how things went, performance-wise, the last time we messed with modules, I'd like to see a performance comparison even if this is a minor change.

Sure, running performance test in https://github.com/dsp-testing/RasmusWL-dca/issues/15

[yoff]

I am still learning to read the performance test reports. It basically looks good timing-wise, but there are a lot of runs with error status?

[tausbn]

I am still learning to read the performance test reports. It basically looks good timing-wise, but there are a lot of runs with error status?

I believe this may be due to non-fatal extractor errors being classified somewhat more severely than necessary.

[RasmusWL]

Since I didn't really get clarity from asking internally whether this deserves a change-note or not, I'm going to suggest we don't add one for now (to not bloat the final change-notes for next release).

Performance test is also done, or rather it got stuck, but I think results looks very reasonable, so I don't think there a need to rerun the entire performance test just to get more runs against salt

[tausbn]

Since I didn't really get clarity from asking internally whether this deserves a change-note or not, I'm going to suggest we don't add one for now (to not bloat the final change-notes for next release).

Sounds reasonable. 👍

Performance test is also done, or rather it got stuck, but I think results looks very reasonable, so I don't think there a need to rerun the entire performance test just to get more runs against salt

I'm still puzzled why some projects appear to be significantly faster with this change (which should only result in more computation), but I guess the main idea was to show that it didn't blow up outright, so I think this is safe.

Incidentally, it seems the performance test only ran a single query (py/path-injection). For more confidence, we should probably run the entire Code Scanning suite going forward.

[RasmusWL]

Incidentally, it seems the performance test only ran a single query (py/path-injection). For more confidence, we should probably run the entire Code Scanning suite going forward.

wow, that was unexpected. good catch 👍 🕵️ I updated my local configuration to use the full code scanning suite by default now 👍