tomli icon indicating copy to clipboard operation
tomli copied to clipboard

Do not use recursion

Open oittaa opened this issue 2 years ago • 2 comments

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.

oittaa avatar Dec 07 '21 23:12 oittaa

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.

hukkin avatar Dec 08 '21 20:12 hukkin

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.

CAM-Gerlach avatar Feb 15 '22 03:02 CAM-Gerlach