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.

Sign up for GitHub

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

Jump to bottom

API.DiffTreeVertexDiffTerm should not be a sum type. #168

Closed
patrickt opened this issue Jun 19, 2019 · 6 comments · Fixed by #180
Closed

API.DiffTreeVertexDiffTerm should not be a sum type. #168

patrickt opened this issue Jun 19, 2019 · 6 comments · Fixed by #180
Labels
backcompat Affects backwards-compatibility bug Something isn't working

Comments

@patrickt
Copy link
Contributor

The current definition is an uncomfortable hybrid of a message (a record type) and a field (a sum type):

data DiffTreeVertexDiffTerm
  = Deleted (Maybe DeletedTerm)
  | Inserted (Maybe InsertedTerm)
  | Replaced (Maybe ReplacedTerm)
  | Merged (Maybe MergedTerm)
  deriving stock (Eq, Ord, Show, Generic)
  deriving anyclass (Proto3.Message, Proto3.Named, NFData)

To abide by protobuf standards, this should be closer to

data DiffTreeVertexDiffTerm = DiffTreeVertexDiffTerm
  { deleted :: Maybe DeletedTerm
  , inserted :: Maybe InsertedTerm
  , replaced :: Maybe ReplacedTerm
  , merged :: Maybe MergedTerm
  }

which is much closer to the actual protobuf definition:

  oneof diff_term {
    DeletedTerm deleted = 2;
    InsertedTerm inserted = 3;
    ReplacedTerm replaced = 4;
    MergedTerm merged = 5;
  }
@patrickt patrickt added backcompat Affects backwards-compatibility bug Something isn't working labels Jun 19, 2019
@patrickt
Copy link
Contributor Author

This is blocking #111.

@robrix
Copy link
Contributor

robrix commented Jun 20, 2019

The product type has strictly less information than the sum type does. Is that gonna be ok?

@patrickt
Copy link
Contributor Author

Yes—indeed, we need it to hold less information for forward-compatibility. The case of DiffTreeVertexDiffTerm Nothing Nothing Nothing Nothing seems pathological but is entirely possible if we added a new alternative to the oneof within diff_term—consumers that hadn’t updated to see that new field should yield a term full of Nothings. We can yield a nice API, however, with -XPatternSynonyms:

pattern Deleted :: DeletedTerm -> DiffTreeVertexDiffTerm
pattern Deleted a = DiffTreeVertexDiffTerm (Just a) Nothing Nothing Nothing

@robrix
Copy link
Contributor

robrix commented Jun 20, 2019

Gotcha. Thanks v. much @patrickt!

patrickt added a commit that referenced this issue Jun 25, 2019
@patrickt
Fix DiffTreeVertexDiffTerm definition to be a valid protobuf object.
90fabfe 
We define the DiffTreeVertex protobuf message like so:

```protobuf
message DiffTreeVertex {
  int32 diff_vertex_id = 1;
  oneof diff_term {
    DeletedTerm deleted = 2;
    InsertedTerm inserted = 3;
    ReplacedTerm replaced = 4;
    MergedTerm merged = 5;
  }
}
```

This is turned into two Haskell types, a toplevel `DiffTreeVertex` type
and a `DiffTreeVertexDiffTerm` type that represents the anonymous
`oneof` type. Said types looked like so:

```haskell
data DiffTreeVertexDiffTerm
  = Deleted (Maybe DeletedTerm)
  | Inserted (Maybe InsertedTerm)
  | Replaced (Maybe ReplacedTerm)
  | Merged (Maybe MergedTerm)
  deriving stock (Eq, Ord, Show, Generic)
  deriving anyclass (Proto3.Message, Proto3.Named, NFData)
```

This is the wrong representation, as it neglects to account for the
fact that options could be added to the `diff_term` stanza. A sum type
does not provide enough constructors to handle the case of when none
of `deleted`, `inserted`, `replaced` etc. is `Just` anything. A more
correct definition follows:

```haskell
data DiffTreeVertexDiffTerm = DiffTreeVertexDiffTerm
  { deleted :: Maybe DeletedTerm
  , inserted :: Maybe InsertedTerm
  , replaced :: Maybe ReplacedTerm
  , merged :: Maybe MergedTerm
  }
```

This patch applies the above change, using `-XPatternSynonyms` to
provide backwards-compatible API shims. Though this changes JSON
output format (through the `ToJSON` instance), it should have no
bearing on backwards compatibility in protobuf objects, since there is
no way to consume diff trees as protobufs as of this writing.

Fixes #168.
@patrickt patrickt mentioned this issue Jun 25, 2019
Merged
@robrix
Copy link
Contributor

robrix commented Jul 26, 2019

Reopened by #205.

@robrix robrix reopened this Jul 26, 2019
@patrickt
Copy link
Contributor Author

patrickt commented Oct 4, 2019

Fixed by #296.

@patrickt patrickt closed this as completed Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
backcompat Affects backwards-compatibility bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix DiffTreeVertexDiffTerm definition to be a valid protobuf object. github/semantic
2 participants
@patrickt @robrix