Check if new constraint is useless against all previous constraints #5565
+14
−6
Conversation
This file contains hidden or 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
Currently, 'addConstraint' will check if the added constraint is "useless" (i.e., adding an UPPER constraint when EQUAL exists). There are also special condition where a constraint is never considered useless, for example when it replaces a constraint from an upper bound. In the flow of 'addConstraint', if the constraint is not deemed useless, the next step is checking for whether the previous constraint can be simplified. If yes, the function immediately returns the simplified constraint. Normally, the method would iterate through the list of constraints and find one that makes the new constraint useless or can be simplified by it. But when the first constraint in the list happens to be from an upper bound, and it can be simplified, then the simplified constraint will be added to the END of the list. If the same constraint is added again, instead of skipping it becuase it would be useless, only the first constraint is checked, and the new constraint is added twice. This commit instead goes through all constraints first to check if the constraint is made useless by any of them, and only then tries to simplify.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
I randomly stumbled across this while playing around with the constraint systems.
Currently, the
addConstraintfunction has a small bug (I think) where it's possible to add lots of duplicate constraints:In this case
addConstraintis called with anUPPER(kotlin/Number)constraint. The flow is the following:getConstraintsWithSameTypeHashCode(constraint)and start iterating with the first constraintfrom position DeclaredUpperBound, and the new constraint is not, so thenewConstraintIsUselesscheck is skippedisMatchingForSimplificationistrue, because we have an existingUPPERand newLOWERconstraintEQUALITYand returned.The eight other
EQUALITYconstraints that make the new constraint fully obsolete are never checked. The outcome of this scenario is always to add a newEQUALITYconstraint, no matter how often it is repeated. Combined with propagation to other type variables, this can lead to an exponential number of useless constraints being added.To get this to actually blow up, I had to do a lot of contrived stuff with type parameters and the declared bounds, but I think this might be worth fixing. The simplification logic returns
actualConstraint to true, which causes it to always be propagated, so it's presumably a lot more expensive than quickly checking for useless types first. Since the declared upper bound constraints are added early, the chance that the first entry in a constraint is a declared upper bound is pretty high.The worst case scenario is this: https://gist.github.com/fxshlein/314b8e3fc79d038eefc90e842a810fd4
This manually builds a constraint system and then fixes a type to trigger this bug. But I wasn't able to fully reproduce the
list = List<Number>equality constraint through actual kotlin code, so I'm not sure if it is possible to actually crash the compiler using this. Either way its probably worth solving.I'm not fully sure if/how I should test this. I didn't find any unit tests, and any effect I could produce through code was just a bunch of useless constraints, but no crash. I'd be happy to write a test for this if someone points me to the right place (if there even is one) 😄