Skip to content

make traverse safe for delete operations #640

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

Merged
merged 1 commit into from
Oct 30, 2017

Conversation

shlomiassaf
Copy link
Contributor

Currently, traverse does not play nice when mutating the children, signature, etc, of a reflection while traversing.

A current issue happens silently here

Every child is removed, but the iteration is done on the children collection:

    traverse(callback: TraverseCallback) {
        if (this.children) {
            this.children.forEach((child: DeclarationReflection) => {
                callback(child, TraverseProperty.Children);
            });
        }
    }

So the first child with index 0 is removed, then the index increments to 1 but because we altered the collection index 1 points to what used to be in index 2, i.e. we skipped an item.

The fix is simple, when working with collection work on a copy.

    traverse(callback: TraverseCallback) {
        if (this.children) {
            this.children.slice().forEach((child: DeclarationReflection) => {
                callback(child, TraverseProperty.Children);
            });
        }
    }

@shlomiassaf
Copy link
Contributor Author

@aciccarello @blakeembrey can we kindly :) speed up PR merges?
I'm starting to pile up PR's and it creates a backlog 👍
thanks!

@aciccarello aciccarello merged commit a24e61e into TypeStrong:master Oct 30, 2017
@aciccarello
Copy link
Collaborator

@shlomiassaf Thanks for the PRs. I'm really glad someone is making improvements. Sorry things are a bit behind. I don't have the time or knowledge of the codebase to process things very quickly. I'll see if I can get through the other 3 PRs sometime soon. Let me know if there is a priority. I'd guess #597 is the main blocker.

I'm also usually available on gitter if you need to discuss something.

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.

2 participants