-
Notifications
You must be signed in to change notification settings - Fork 428
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
Define record "inheritance" #6851
Comments
Add an error when a record tries to inherit [reviewed by @noakesmichael] Resolves #4954. We removed record inheritance from the language specification due to not having ironed out enough of the details. However, field inheritance for records was already implemented, and we did not remove it. This change adds an error message after parsing if we detect that a record has been defined which inherits. It also points users encountering this to an issue I created for discussion on record inheritance (#6851). It does not remove field inheritance for records. Testing: Adds a test to lock in the error message. Converts 7 tests into futures, and updates the expected output for 5 futures. Also, splits one test into a working portion and a now failing portion. Passed a full paratest with futures.
Record inheritance would be useful for error memory management if we don't get an Owned/Shared story first. I don't know if that should be a driving motivation though. |
@lydia-duncan @bradcray I suggest to close this issue. We did away with record inheritance officially in #5892 because we had - and have - no current plans to consider it. We can reopen this issue - or create a new one - when+if appropriate. |
Bonus: how about remove the tests that, since #6860, say "This test will probably be useful when we rethink the feature" and reference this issue:
|
I'm ambivalent to closing the issue but against removing the tests. |
I'm closing this issue to reflect that our current plan is not to support record inheritance. Of course, in the future, that can be revisited with a new issue proposing a particular meaning and showing a compelling use case. |
At one point, we discussed having a "mix-in" concept instead of supporting record inheritance. However, since we implemented
@lydia-duncan: Can you say more about this? I haven't reviewed all of them, but checking the first as an example, I'm not seeing that preserving it adds much value to our nightly testing... And I agree with Vass's sentiment that it doesn't represent a feature or syntax that we have any intention of supporting. |
That comment was from 2019. I don't feel strongly today |
We used to define record inheritance as "field only". We've since decided that we hadn't sufficiently defined what that would mean, what should happen with methods, etc, and so removed it from the language specification and will (shortly) make it an error for a record to inherit.
This is a placeholder issue for the discussion on what record inheritance should mean, so that the error message can point users to it and we can get feedback on what would be wanted. We don't anticipate working on this issue immediately, but would likely be disappointed if Chapel had not figured this out 5-10 years down the road
The text was updated successfully, but these errors were encountered: