Fixes related to HList type inference #2034
Closed
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
If an applied type has a refinement, it was printed before as one large refinement type including the type parameter bindings.
No reason why we should not - normalize handles implicit methods just fine. This fixes type errors in test HLists.scala.
We approximate dependencies to parameters by Wildcards. This was already done in one place, is now done in other places as well, instead of doing nothing for dependent methods.
We previously tried to force S1 and S2 be the same type when encountering a lub like `T1 { A = S1 } & T2 { A = S2 }`. The comments in this commit explain why this is unsound, so this rewrite is now made subject to a new config option, which is off by default. I verified that the new behavior does not affect the performance of the junit tests. These now compile with the changes to dependent methods, except for one which is invalid under dotty.
Variance changes quite a few things for type inference, so it's good to check a non-variant version as well.
senia-psm reviewed Mar 1, 2017
tests/run/HLists-nonvariant.scala Outdated
| } | ||
| } | ||
| | ||
| object TestNonVariant { |
Contributor
There was a problem hiding this comment.
These objects (Test and TestNonVariant) are identical. Is it intentional?
Need to use fresh PolyParams instead of WildcardTypes if constraint is committable.
Turned out hmaps.scala requires the arg alignment to compile. So we have our first counterexample that we cannot drop this hack. Now it is made safe in the sense that no constraints get lost anymore.
I believe this worked only accidentally because we matched more things with wildcards which turned out to be flawed. The test errors show some funky _#_ types, so not sure whether the tests are still valid or not. Moved back to pending awaiting further resolution.
Contributor Author
| Something seems to be wrong with the CI on this one.
@felixmulder: any ideas what this could be or how to fix it? |
Contributor Author
| Also, I noticed that commits before the last one all get a tick, even if they were not tested themselves. That risks being misleading. |
Type inference tends to take quite different paths for non-variant and variant data structures. Since, non-variant hmap has already exposed quite a few problems, it's good to test it also in the covariant case.
Contributor Author
| I pushed a new commit, but now CI won't even start on this one. It seems the previous zombie run prevents the CI from doing anything further here. Will try with a new PR. |
Contributor
| The bot currently sets the status of each commit that it has checked the CLA for. I could change it to set a status (failed) only if the user hasn't signed the CLA...But this won't let us know if the bot is really working... Yes, kill the PR, probably best...I'm currently in bed with a fever. Will check more on it tomorrow. On Thu, Mar 2, 2017, 18:12 odersky <notifications@github.com> wrote: Also, I noticed that commits before the last one all get a tick, even if they were not tested themselves. That risks being misleading. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2034 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdYwb0MNImHsyvYgUK6HqaXgyo2_wexks5rhogDgaJpZM4MM8E8> . |
Contributor Author
| Get better soon, Felix! |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
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.
Two main fixes and some cleanups: