Skip to content
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

Closed

Conversation

alexanderpann
Copy link
Member

@alexanderpann alexanderpann commented Dec 5, 2022

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.

@alexanderpann alexanderpann added the PRIO Use for issues/PRs with customer project background label Dec 5, 2022
@lhartl
Copy link
Contributor

lhartl commented Dec 21, 2022

which typesystem check fails when the method is implemented for tagged types?

@arimer arimer self-requested a review March 15, 2023 09:45
Copy link
Member

@arimer arimer left a 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.

AlexeiQ pushed a commit that referenced this pull request Oct 23, 2023
Cherrypicked and squashed commits from Alexander Pann
from PR #627
from branch bugfix/missingBaseTypes_20211
@alexanderpann alexanderpann marked this pull request as draft November 17, 2023 07:25
@alexanderpann alexanderpann changed the base branch from maintenance/mps20211 to maintenance/mps20213 November 17, 2023 10:18
@alexanderpann
Copy link
Member Author

I am closing this PR in favor of #723 and I'll keep the TaggedType as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PRIO Use for issues/PRs with customer project background
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants