Add configuration for maximum size of shaped array

[danog]

No description provided.

[orklah]

Sorry, but I don't like this

This kind of limits are frequently calibrated to prevent runaway processing that leads to memory issues, performance issues and prevent stability issues in general.

It adds complexity in Psalm and will probably be missused by most users and they'll end up ticketing here bugs that will be pretty hard to find.

What is your actual use case for this?

[danog]

My actual usecase involves completely disabling this check in my fork, I used this at work to automatically typecheck mongoDB queries (including up to 3-level nested $or, $and, $eq, $gt, $gte, $in, $lt, $lte, $ne, $nen syntax :D).

The typechecks worked perfectly, but caused 30-minute psalm scans with up to 20GB of RAM, and were promptly disabled :D

Still, I kept the checks disabled, as I imagine they can slightly improve overall type inferring, and that's also the reason why I submitted this MR.

[orklah]

Ok, I get it, I thought you wanted to go the other way (to have more literals). Well, I have mixed feelings on that.

On one hand, I know there are some issues not fixed yet where not having literals in an array created false positives, so this is not just a neutral feature we can just disable. On the other hand, if there are dramatic perf improvements, it would be a shame not to be able to have them when needed...

[danog]

I thought you wanted to go the other way (to have more literals)

Actually, that's exactly what I wanted to do, I wanted to set a huge limit like PHP_INT_MAX :D

[orklah]

You'll hit hard walls at some point. If you try to use a switch on that, I believe the wall will be very big and with spikes too.

The issue is that Psalm checks can be exponential. Luckily, Psalm is pretty quick, but no matter how quick, there is one point where the exponential process will takes years to complete. And it's waaay before PHP_INT_MAX 😄

[danog]

The issue is that Psalm checks can be exponential

Does this mean we can use a SAT solver to run psalm now? :D

[danog]

Btw, at work we run psalm with this config @ PHP_INT_MAX, and (single-threaded, uncached) scans complete in just 5 minutes with 420k lines of code, ~4.7 million lines considering dependencies :)

[AndrolGenhald]

@orklah How would you feel about this if documentation were added that warned about potential memory/runtime issues? I've sometimes wanted to increase other limits like this, and it would be nice to be able to do, as long as the documentation makes it very clear that you're playing with fire by increasing it.

[orklah]

Configs like that

• are easy to add
• may create weird bugs that will be hard to reproduce
• will be used only by a handful of users
• will add complexity
• are very hard to get rid off once we don't need them anymore

I guess I just don't see the point. Once this one is included, I can think of a handful more exactly like that at different places in Psalm, we won't add config for each and every one...
Just the last two weeks we added or were suggested to add at least 5 different new configs, this is not a healthy pace and even I can't say that I know what every config we have does.

Maybe we should have a much more powerful config tool if we want to consider going at this level of details. I'm thinking of something like about:config in firefox where a lot of configs are available but you have to be sure of what you do before changing one.

Maybe something that wouldn't depend on the xsd to add or remove configs...

That way we could keep important configs in a visible place and the rest would be available for power users but almost invisible for the rest?

You have thoughts on that @weirdan?

[danog]

Once this one is included, I can think of a handful more exactly like that

Actually, I added it exactly after another (undocumented) similar config, maxStringLength :D

I also didn't document it in a similar manner to maxStringLength: I assumed that the latter was a somewhat internal config, and followed the same approach with this one.

[AndrolGenhald]

I guess I just don't see the point. Once this one is included, I can think of a handful more exactly like that at different places in Psalm, we won't add config for each and every one... Just the last two weeks we added or were suggested to add at least 5 different new configs, this is not a healthy pace and even I can't say that I know what every config we have does.

True, there are a lot of config options available, and more being added.

That way we could keep important configs in a visible place and the rest would be available for power users but almost invisible for the rest?

What do you think about not using attributes on the root element, but on some child element? Something like:

<psalm
    [a bunch of config attributes]
>
    <RuntimeLimits
        maxStringLength="100"
        maxShapedArraySize="100"
    />
</psalm>

That way they'd be grouped separately from the rest of the options.

[orklah]

Grouping them is a good idea!

I wonder if getting them out of the xml could be interesting. I find the XSD validation to be perfect for issuesHandler because we can autocomplete and validate the user input. However, for simple key/values for configs, I find it pretty heavy. Also the current handling in Config that force us to have a new property for each config is really heavy and create unnecessary BC breaks when we want to remove some.

For complex config, maybe a simple json would be enough and stored as an array in some kind of ConfigValue object?

Just throwing ideas here :p

[weirdan]

You have thoughts on that @weirdan?

I kinda like what they did in this regard in roave/backward-compatibility-check: Roave/BackwardCompatibilityCheck#377.

With that said, there's nothing wrong with using your own fork of Psalm either.

[danog]

Can I haz merge||definitive close :3
This is really just a counterpart for maxStringLength: if that config key already exists, I don't see why shouldn't this one exist, either...

[orklah]

Can you add some documentation on this config? Please add a note about how changing this to low or high values could have unintended effects and that those side effects won't be considered as bugs.

[danog]

Done!

[orklah]

Thanks!