-
Notifications
You must be signed in to change notification settings - Fork 22
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
Implement missing base types #627
Conversation
which typesystem check fails when the method is implemented for tagged types? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had a closer look to the problem. Chaning the behaviour impl. of TaggedType.baseType()
would change the semantic of what a base type of TaggedType should be. Before the change the baseType of a TaggedType was the TaggedType itself. After the change we delegate to its child TaggedType.baseType
. To be honest, I am not 100% sure what is the right impl. here. In case we change it, our internal API would calculate a different expected base type for TaggedType and might break on runtime, because it is expecting an other type.
This is excactly the problem for the breaking test, because the test is relying on the fact that:
ur.ancestor<concept = TaggedType>.baseType:TypedefType.baseType()is delgating to TypeDef.baseType() which again invokes the (now changed) baseType()
implementation on an instance of TaggedType, resulting in a different returned value.
I would argue, in case the want to override the default implemention of Type.baseType() of any subconcepts of Type
, we must ensure that the calculated runtime type is correct. This would mean, that code which expects the (old) default impl. of baseType()
would need to be updated to handle the (new) different return type.
I dont think we can merge this PR withouth the required changes sketched above.
Cherrypicked and squashed commits from Alexander Pann from PR #627 from branch bugfix/missingBaseTypes_20211
I am closing this PR in favor of #723 and I'll keep the |
I am not sure about the TaggedType. It also has a base type, but there is a type system check that fails when the method is implemented.