Python: use the shared regex pack

[erik-krogh]

the commits should explain what's happening

Evaluation looks fine.

I'm pretty sure the QLDoc check failure is spurious, but I don't know that happens.
I've tried to add all the QLDoc I could in those files, and I've even tried to add extra modules into the file (just to have something to attach a QLDoc too), but it keeps failing.

[yoff]

Only a very minor typo.

You write that the dropped result is due to locations. I can see that a result in a nested location is retained. Is that what is going on, some stronger canonicalisation?

It looks like we are getting better alert messages? I remember we turned off prefix computation for Python for a period due to performance issues, perhaps it was never fully turned on again...in any case, the evaluation looks fine.

Unrelated to this PR: Is the construction in PolynimoalReDoS.ql, where we filter the sources to be PolynomialBackTrackingTerms after the dataflow computation a little silly? Should the constraint be part of the definition of Sink instead?

[erik-krogh]

You write that the dropped result is due to locations. I can see that a result in a nested location is retained. Is that what is going on, some stronger canonicalisation?

I would call it some slightly better canonicalisation.
I drilled into it, and it is this de-duplication code that produces different results.
The old behavior is actually a bug, the result was supposed to be removed (because it's effectively a duplicate), but with the better canonicalisation we now properly remove that result.

You can add more terms to the order by clause in the old implementation to replicate the results seen in the new implementation.

I'll make the canonicalisation even more robust.

[erik-krogh]

It looks like we are getting better alert messages? I remember we turned off prefix computation for Python for a period due to performance issues, perhaps it was never fully turned on again...in any case, the evaluation looks fine.

I seem to remember us fixing the root cause of that a while ago.
I'm quite sure this PR doesn't change the alert-messages? We had prefix as part of the alert-message before this PR.

Unrelated to this PR: Is the construction in PolynimoalReDoS.ql, where we filter the sources to be PolynomialBackTrackingTerms after the dataflow computation a little silly? Should the constraint be part of the definition of Sink instead?

Yes.
But could you wait with that until after this PR?

[yoff]

I would call it some slightly better canonicalisation. I drilled into it, and it is this de-duplication code that produces different results. The old behavior is actually a bug, the result was supposed to be removed (because it's effectively a duplicate), but with the better canonicalisation we now properly remove that result.

Thanks for finding the precise code :-)

I seem to remember us fixing the root cause of that a while ago.

That is how I remember it also.

I'm quite sure this PR doesn't change the alert-messages? We had prefix as part of the alert-message before this PR.

The diff (of the .expected-files) has a lot of added starting with '*' and and similar, though.

But could you wait with that until after this PR?

Absolutely, that belongs in a fix of its own.

[yoff]

I ran the tests in a Codespace to make sure everything could compile and find each other. I have no further issues. The QLdoc failure is annoying, is it because there is no longer a public import, so it gets confused about where the comment should be? In any case, the check looks to be in the wrong..

[erik-krogh]

The QLdoc failure is annoying, is it because there is no longer a public import, so it gets confused about where the comment should be?

I tried to add a public module in the files to see if that made the errors go away, but that didn't happen.
So your theory doesn't seem to be what's happening.