tomli
tomli copied to clipboard
Do not use recursion
I noticed that there are couple of TOML test files without JSON counterparts (https://github.com/hukkin/tomli/blob/master/tests/data/toml-lang-compliance/valid/qa/qa-array-inline-nested-1000.toml, https://github.com/hukkin/tomli/blob/master/tests/data/toml-lang-compliance/valid/qa/qa-table-inline-nested-1000.toml) that couldn't be loaded with the current version due to deep recursion. Maybe they're not very realistic use cases, but the fix isn't that complicated. On my computer there's a 1-2% performance hit, but you might want to test more. One another thing is that I don't have deep knowledge about NestedDict
, Flags
and other more complicated inner workings of the software. I just naively copied more or less everything to the stack even though they might be available in some smarter way.
Thanks! I'll try to find the time to review this in the future.
I will say already that I'm not 100% sure yet that I want this fixed, at least not until someone comes up with a real world issue regarding this limitation. Using recursion is a simpler approach (as can be seen e.g. from the number of lines added in this PR) so if there's even a small chance of introducing bugs, and a performance penalty, I'm not sure this is worth fixing.
There will always be limits to how deeply nested structures we can parse due to memory constraints if nothing else. I don't know if a crash at something like 100000 levels of nesting is that much better than 1000 levels of nesting.
NB, @hukkin if you consider merging this, before doing so I suggest getting explicit written consent from @oittaa to license this under both the MIT license and the PSF license. Otherwise, this contribution is substantial enough to block relicensing tomli as a whole under the PSF license.