Skip to content
This repository was archived by the owner on Apr 1, 2025. It is now read-only.

Conversation

patrickt
Copy link
Contributor

We define the DiffTreeVertex protobuf message like so:

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:

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:

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.

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.
@@ -1,5 +1,5 @@
-- Code generated by protoc-gen-haskell 0.1.0, DO NOT EDIT.
{-# LANGUAGE DerivingVia, DeriveAnyClass, DuplicateRecordFields #-}
{-# LANGUAGE DerivingVia, DeriveAnyClass, DuplicateRecordFields, PatternSynonyms #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the above comment imply that our pattern synonyms will be overwritten?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, unfortunately (though we haven't regenerated this file in ages). I'm going to file a bug with the upstream proto3-suite to add generation of pattern synonyms for oneof types.

@robrix robrix merged commit e0ff53e into master Jun 25, 2019
@robrix robrix deleted the fix-bad-proto-type branch June 25, 2019 18:56
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.

API.DiffTreeVertexDiffTerm should not be a sum type.

2 participants