Fix infinite loop bug(s) in recursion checking #234
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow future optimizations to take advantage of the call tree built during validate_functions() to improve performance
If for some reason, remove() doesn't remove something, then we'll keep trying to remove it infinitely. remove() should never fail in this contexts in the absence of Cascade bugs. Transform the infinite loop bugs in this case into InternalErrors for easiest discoverability and debugging in the event that something has gone wrong.
The actual validation and error reporting part here is straightforward. Most of the complexity of this commit comes from tracking the file the alias is declared in through to the error site. That is both complex, and will likely hurt performance due to file clone()s in a non-error path. The good news is that we'll likely get all the performance back, and reduce code complexity once we implement #219. When we do that, it's probably worth revisiting this commit to make sure everything is cleaned up appropriately and we're not tracking files around for no reason.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first commit is just a minor reordering to 1. Prepare to totally rewrite search_for_recursion() on top of the call tree added recently and 2. Delay the performance hit associated with it until later, in case there are errors earlier in compilation.
The second commit turns the infinite loop we were seeing on alias conflicts into an internal error. This should help detect any future bugs in this area more easily.
The final commit is the real fix, which checks for alias conflicts and reports an error. The error handling is an extremely awkward dance with a performance hit, as described in the commit message. We should get everything back in terms of both code complexity and performance once we implement #219.