Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not use recursion #148

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Do not use recursion #148

wants to merge 4 commits into from

Conversation

oittaa
Copy link
Contributor

@oittaa oittaa commented Dec 7, 2021

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.

@hukkin
Copy link
Owner

hukkin commented Dec 8, 2021

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.

@CAM-Gerlach
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants