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 instance Data #614

Merged
merged 13 commits into from
Nov 7, 2023
Merged

Implement instance Data #614

merged 13 commits into from
Nov 7, 2023

Conversation

Bodigrim
Copy link
Contributor

Fixes #513.

This is #519 rebased + tweaked tests. Thanks to @cbclemmer for trailblazing.

@Bodigrim Bodigrim added this to the 0.12.1.0 milestone Sep 24, 2023
@clyring
Copy link
Member

clyring commented Sep 27, 2023

As ShortByteString is not an opaque type (even without internal modules), I would rather guess it should keep its existing derived Data instance. But I haven't thought too hard about this yet.

@Bodigrim
Copy link
Contributor Author

@clyring good catch. No particular reason, just a leftover from the previous PR. Reverted now.

Since there are no internal invariants to uphold, I also added instance Generic ShortByteString.

Data/ByteString/Internal/Type.hs Show resolved Hide resolved
Data/ByteString/Internal/Type.hs Outdated Show resolved Hide resolved
tests/Properties/ByteString.hs Outdated Show resolved Hide resolved
gunfold k z c = case constrIndex c of
1 -> k (z packBytes)
_ -> error "gunfold: unexpected constructor of strict ByteString"
dataTypeOf _ = byteStringDataType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dataTypeOf change affects non-throwing traces. Major version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PVP does not prescribe this: somewhat surprisingly, it does not say anything about term-level changes. I deem this as a bug fix, not a change of intended semantics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of the existing instance implementation is not so obvious. I do think this dataTypeOf change makes the instance more self-consistent and is an improvement.

But as a non-expert in the wider usage of Data I'm hesitant to put even small potentially-breaking changes into a minor release. And our users seem in no particular hurry to get working gread functionality.

The rest of the patch is fine for a minor release, of course.

PVP does not prescribe this: somewhat surprisingly, it does not say anything about term-level changes.

Right, the text of PVP mentions nothing on this subject. I'm reasonably sure that's an oversight. The graphical decision tree on the front page does have a node indicating that if "the behavior of any exported function change[d]," then a major version bump is necessary. The FAQ also provides an explicit example on this subject.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reasonably sure that's an oversight.

Neither FAQ nor decision tree (haskell/pvp#52) are normative. PVP is compile-time only. There was an attempt to change this (haskell/pvp#30), but it was withdrawn.

It's not a hill I want to die on, so if you feel strongly about it, I'll revert changes to dataTypeOf.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to revert! The change is fine; I just think this should be where bytestring-0.12 finally diverges from master.

@Bodigrim
Copy link
Contributor Author

@clyring any more suggestions?

@Bodigrim Bodigrim removed this from the 0.12.1.0 milestone Nov 7, 2023
@Bodigrim Bodigrim merged commit 1b9e6ec into haskell:master Nov 7, 2023
24 checks passed
@Bodigrim Bodigrim deleted the cc/#513 branch November 7, 2023 23:32
clyring pushed a commit to clyring/bytestring that referenced this pull request Feb 1, 2024
* Add functionality for toConstr

* Other instances fixed

* Move test

* test passes

* Add gshow tests

* Typo

* Add explicit string test

* instance Data: implement gunfold and dataTypeOf

* instance Data: fix tests

* Fix emulated builds

* Restore derived instance Data ShortByteString

* Add instance Generic ShortByteString

* Review suggestions

---------

Co-authored-by: Colton Clemmer <coltonclemmerdev@gmail.com>
(cherry picked from commit 1b9e6ec)
clyring pushed a commit to clyring/bytestring that referenced this pull request Feb 1, 2024
* Add functionality for toConstr

* Other instances fixed

* Move test

* test passes

* Add gshow tests

* Typo

* Add explicit string test

* instance Data: implement gunfold and dataTypeOf

* instance Data: fix tests

* Fix emulated builds

* Restore derived instance Data ShortByteString

* Add instance Generic ShortByteString

* Review suggestions

---------

Co-authored-by: Colton Clemmer <coltonclemmerdev@gmail.com>
(cherry picked from commit 1b9e6ec)
clyring pushed a commit that referenced this pull request Feb 1, 2024
* Add functionality for toConstr

* Other instances fixed

* Move test

* test passes

* Add gshow tests

* Typo

* Add explicit string test

* instance Data: implement gunfold and dataTypeOf

* instance Data: fix tests

* Fix emulated builds

* Restore derived instance Data ShortByteString

* Add instance Generic ShortByteString

* Review suggestions

---------

Co-authored-by: Colton Clemmer <coltonclemmerdev@gmail.com>
(cherry picked from commit 1b9e6ec)
@clyring clyring added this to the 0.13.0.0 milestone Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error calling 'toConstr' method for ByteString
3 participants