refactoring decompose in typed pattern

[tias]

a better-typed decompose (inspired by the new pattern in negate).

It does show we need to continue typing our expressions #913 :
[ ] Element decompose can return int (even in our test suite there is a case for element), to be changed (see inline comment)
[ ] global constraint decompose can return bool, should only return Expressions

and this one should probably go after the CSEMAP #917

This typed version should be a good basis for the positive-decompose, e.g. we only want to support that in the for loop in decompose_in_tree (which can also check if expr.name == "->" and expr.args[1] isinstance GlobalConstraint... for the halfreified positives)

[IgnaceBleukx]

Great, had some trouble merging master into this one, but should be done now.

A couple of open questions still

[tias]

I've changed, it but

• the code was consistent: in both paths, it was first doing decompose and only then recurse over the args (so it was consistently different from what it used to be)
• the reason was indeed that after decompose, you need to decompose what you get again (for nested decompositions)
• it also made the csemap easier: you could check it, decompose the global, decompose it args, then insert it; now, you have to check the map, recurse over the args, decompose, decompose the args, then only insert it

I think the current code is harder to read, it also requires an additional 'cast', and with the csemap, the cost of 're-decomposing' globals in the args was actually non-existing... the whole reason of the map was to avoid that.

So, I would be in favor of going back...

[tias]

actually two casts...

now that it is still fresh in my mind, I am going to revert it and document why we do it the way we do it (and there are some minor cleanups possible I now realize)

[tias]

hmm, it was a rather extensive refactor for the args part; that looks quite a bit like what I reverted; its still a bit cleaner.

Also compatible with the other PR about 'decompose_custom_positive', I would expect, as it only touches decompose_in_tree().

This is compatible with the 'supported_implication' for choco, needs a name == '->' check and a few lines in decompose_in_tree() (non-urgent?)

[IgnaceBleukx]

Ok so if I get it, the pattern now is:

if unsupported global cons/func: decompose.
then treat the decomposition as any expression: either recursively decompose, or check the args of the decomp.

I think that's clean, easy to follow and better than what we had before : )
but some comments on the code itself, where cleanup is possible.

[IgnaceBleukx]

Great, I think we're there, and with a nice runtime improvement too!
dev/time_transformations has a decompose time doing down from 43s -> 34s on my machine.
Also memory usage is decreased by ±10%

I've added a type alias for the custom decomposition, its somewhat cryptic so fine to remove again if we don't like it.

Good to merge for me.