Skip to content
This repository has been archived by the owner on Sep 3, 2024. It is now read-only.

Remove dynamic type check in Static Binary instance. #16

Merged
merged 1 commit into from
Sep 29, 2016

Conversation

facundominguez
Copy link
Contributor

Authored by @mboes

Currently, a dynamic type check occurs when a static value is deserialized. The reason for this check is to preclude writing pathological functions such as decode . encode that make it possible to modify the phantom type parameter of the static value.

However, decode . encode is arguably the same pattern as read . show, which in general is unsafe (e.g. whenever phantom type parameters are involved) but not in any manner specific to static values. It is debatable whether we should defensively protect against the user writing such code through the use of extra Typeable constraints. For what is good for the goose is good for the gander, i.e. not just static values, yet we cannot hope to guarantee type safety whenever the representation of an abstract type is exported in any way.

Moreover, this dynamic type check is redundant with a second type check that occurs when resolving static values, in 'unstatic'. One can construct all manner of static values and compose them, but they can only be used by resolving them. Hence a dynamic type check will occur at least once, i.e. at resolution time. There is, therefore, little value in doing a dynamic type check earlier, while each such type check has a non negligible performance cost.

Currently, a dynamic type check occurs when a static value is
deserialized. The reason for this check is to preclude writing
pathological functions such as `decode . encode` that make it possible
to modify the phantom type parameter of the static value.

However, `decode . encode` is arguably the same pattern as `read .
show`, which in general is unsafe (e.g. whenever phantom type parameters
are involved) but not in any manner specific to static values. It is
debatable whether we should defensively protect against the user writing
such code through the use of extra Typeable constraints. For what is
good for the goose is good for the gander, i.e. not just static values,
yet we cannot hope to guarantee type safety whenever the representation
of an abstract type is exported in any way.

Moreover, this dynamic type check is redundant with a second type check
that occurs when resolving static values, in 'unstatic'. One can
construct all manner of static values and compose them, but they can
only be used by resolving them. Hence a dynamic type check will occur at
least once, i.e. at resolution time. There is, therefore, little value
in doing a dynamic type check earlier, while each such type check has
a non negligible performance cost.
@mboes
Copy link
Contributor

mboes commented Sep 23, 2016

I vaguely remember that this change has been discussed before, back when it was first proposed over 2 years ago. FTR one of the points in the discussion is that read . show and encode . decode are not entirely the same situation: encode . decode for static pointers at the wrong type can cause the program to segfault. Whereas read . show and encode . decode will at worse result in a runtime exception.

@mboes
Copy link
Contributor

mboes commented Sep 23, 2016

Correction: in the current implementation decode . encode won't segfault, it' just that Static Int might be a lie (You can't get an Int out of it). The major issue I have with the current Binary instance is that unlike most (if not all) other Binary instances out there, it makes the encoding dependent on the compiler. Change the compiler, can't decode anymore. Worse, change the package version, can't decode anymore. Worse still, move some datatype to a different package during a refactoring, can't decode anymore.

@facundominguez facundominguez merged commit 957c257 into master Sep 29, 2016
@facundominguez facundominguez deleted the fd/no-rtt-check branch September 29, 2016 17:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants