Sumtype enums #151

inanna-malick · 2017-10-26T23:13:55Z

This change is

googlebot · 2017-10-26T23:13:59Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

inanna-malick · 2017-10-30T17:34:42Z

I signed the CLA (by adding myself to the takt OSS google group), cc @judah

googlebot · 2017-10-30T17:34:47Z

CLAs look good, thanks!

judah · 2017-10-27T00:06:48Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

+    --         = Prelude.show k
+    --       showEnum k = Prelude.show k
+    --       readEnum "Enum2a" = Prelude.Just Enum2a -- alias
+    --       readEnum k = Text.Read.readMaybe k


I suspect this doesn't do what you think it does, for two reasons:

If the value is known, we should use the constructor instead of FooEnum'Unrecognized

If the value is not known, I think this reads a value of type FooEnum rather than of type int.

You probably want instead something like readMaybe k >>= maybeToEnum

Also please add tests for the above cases.

readMaybe k >>= maybeToEnum assumes that the textual representation of these enum's are Ints. I was trying to reproduce the pre-existing logic in which strings like "Enum2a", "Enum2", etc are converted into instances of the sum type. Should this function instead take strings holding ints mapping to different enum values, eg readEnum "1" -> Just Foo'Enum1?

Oh, I see, I think I misread your code earlier.

Taking another look, we actually can't use the Show and Read instances for FooEnum to implement showEnum/readEnum. Because for nested enums like

message Bar { enum Foo { A = 1; } }

we want showEnum Bar'A = "A" and readEnum "A" = Bar'A, not showEnum Bar'A = "Bar'A" and readEnum "Bar'A" = Bar'A. It could probably be described better in the docs, but readEnum/showEnum are specifically for the proto text format representation of the enum:
https://github.com/google/proto-lens/blob/master/proto-lens/src/Data/ProtoLens/Message.hs#L231

I think we should just take the string representations (not the ints like I suggested earlier), as in the code for proto2:
https://github.com/google/proto-lens/blob/master/proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs#L374

readEnum "Enum2" = Enum2 readEnum "Enum2a" = Enum2a etc.

That sounds good, but that still leaves us with no ability to serialize/deserialize unrecognized enum's. I'm assuming readEnum . showEnum == id is a property we want to maintain. If so what should showEnum Foo'FooEnum Foo'Unrecognized... result in?

Ah, good point, we do need something sensible for showEnum at least. How about this?

showEnum Enum2 = "Enum2" showEnum (Foo'Unrecognized (Foo'UnrecognizedValue n)) = show n readEnum "Enum2" = Enum2 readEnum "Enum2a" = Enum2 readEnum s = Foo'Unrecognized . UnrecognizedValue <$> readMaybe s

Since in proto text format, it's fine to refer to an enum by its numeric value rather than the case name.

judah · 2017-10-27T00:10:13Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

+    constructorNumbers = map (second (fromIntegral . (^. number))) constructors
+
+    succPairs = zip constructorNames $ tail constructorNames
+    succDecl funName boundName thePairs =


I think succ should be equivalent to toEnum . incr . fromEnum. The implications are kind of subtle though compared to the proto2 semantics listed here: https://github.com/google/proto-lens/blob/master/proto-lens/src/Data/ProtoLens/Message/Enum.hs#L2
If we do make that change, we should note in Data.ProtoLens.Message.Enum that it only applies to proto2.

Sounds good. Is not erroring on succ maxEnum and instead returning SomeEnum'UnrecognizedValue... (fromEnum maxEnum + 1) the desired behavior here?

Do you also want me to redefine pred as toEnum . (-1) . fromEnum? Or should pred minEnum still result in an error?

I think this would break things, eg for FooEnum as defined in proto3.proto:

enum FooEnum { option allow_alias = true; Enum1 = 0; Enum2 = 3; Enum2a = 3; }

it seems like we want succ Enum1 == Enum2, not succ Enum1 == Foo'FooEnum'Unrecognized (Foo'FooEnum'UnrecognizedValue 2)

judah · 2017-10-27T00:11:00Z

proto-lens-tests/tests/proto3.proto

@@ -16,17 +16,18 @@ message Foo {
  }
  Sub sub = 5;

-  enum FooEnum {


What was the reason for changing this test? My preference is to add a new test case if we want to support the different situation. (Or, at minimum, add another test that does check naming of nested enums.)

I think that was due to a transient issue, I've reverted this change.

judah · 2017-10-27T00:13:09Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

-                                          @@ ("Prelude.shows" @@ "k"))
-          ]
-        ]
+    -- data FooEnum = Enum1


Formatting nit in this comment:

-- data FooEnum -- = Enum1 -- | Enum2 -- | FooEnum'Unrecognized ...

judah · 2017-10-27T00:18:36Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

+
+    unrecognizedValueType = tyCon $ unQual unrecognizedValueName
+
+    constructors :: [(Name, EnumValueDescriptorProto)]


It's a shame this now duplicates logic from the proto2 code...not sure if there's a good way to re-combine the two. (They were split apart more in #137.)

judah · 2017-10-30T17:31:37Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Combinators.hs

@@ -359,3 +359,7 @@ modifyModuleName :: (String -> String) -> ModuleName -> ModuleName
 modifyModuleName f (Syntax.ModuleName _ unpacked) =
  Syntax.ModuleName () $ f unpacked

+
+modifyIdent :: (String -> String) -> Name -> Name


Code structure nit: we try to define names inside the Definitions module and use them in Generate. In this case, adding new fields to EnumInfo with the Names for the unrecognized types:
https://github.com/google/proto-lens/blob/master/proto-lens-protoc/src/Data/ProtoLens/Compiler/Definitions.hs#L157

judah

Overall looks good; just a few nits remaining.

judah · 2017-10-31T22:13:06Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

        , [ match "readEnum" [stringPat pn]
              $ "Prelude.Just" @@ con (unQual n)
          | v <- enumValues info
          , let n = enumValueName v
          , let pn = T.unpack $ enumValueDescriptor v ^. name
+          ] ++
+          [match "readEnum" [pVar "k"] $ ("Prelude.>>=" @@ paren ("Text.Read.readMaybe" @@ "k")) @@ ("Data.ProtoLens.maybeToEnum")]


Break this line which has gotten pretty long.

judah · 2017-10-31T22:13:30Z

proto-lens-tests/src/Data/ProtoLens/TestUtil.hs

@@ -68,6 +68,7 @@ import Text.PrettyPrint
 testMain :: [Test] -> IO ()
 testMain = defaultMain

+-- todo use this for enum msg on Foo


Remove comment

judah · 2017-10-31T22:16:12Z

proto-lens-tests/tests/proto3_test.hs

  , testGroup "enum"
      [ testCase "aliases are exported" $ Foo'Enum2 @=? Foo'Enum2a
+      , serializeTo "serializeTo enum"


Can you add a copy of this test which uses an unrecognized value?

Also: replace l.107 with more explicit generation:

tagged 6 $ VarInt 3

(where 6 is the proto tag of that field, and 3 is the value corresponding to Enum2)

Ping on the first line of the above comment (unless I'm misreading the diff).

More specifically, please add this test which checks that unrecognized values round-trip through serialization as expected.

serializeTo "serializeTo enum unknown" (def & enum .~ toEnum 5 :: Foo) "enum: 5" $ tagged 6 $ VarInt 5

whoops, sorry about that. Fixed.

judah · 2017-10-31T22:17:34Z

proto-lens-tests/tests/proto3_test.hs

              @=? map show [Foo'Enum1, Foo'Enum2, Foo'Enum2a, toEnum 5]
          ["Enum1", "Enum2", "Enum2", "6"]
              @=? map showEnum [Foo'Enum1, Foo'Enum2, Foo'Enum2a, toEnum 6]
-          [Just Foo'Enum1, Just Foo'Enum2, Just Foo'Enum2, Nothing, Nothing]
+          [Just Foo'Enum1, Just Foo'Enum2, Just Foo'Enum2, maybeToEnum 4, maybeToEnum 5]


Test nit (working around the fact that the constructor isn't provided): Add a new test that checks fromEnum <$> maybeToEnum 4 == Just 4

judah · 2017-10-31T22:20:15Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

+    --               (FooEnum'UnrecognizedValue (Prelude.fromIntegral k)))
+    --       showEnum (FooEnum'Unrecognized (FooEnum'UnrecognizedValue k))
+    --         = Prelude.show k
+    --       showEnum k = Prelude.show k


I think this comment needs to be updated since you're not calling Prelude.show anymore.

judah · 2017-10-31T22:40:18Z

proto-lens-protoc/src/Data/ProtoLens/Compiler/Generate.hs

+    --   deriving (Prelude.Eq, Prelude.Ord, Prelude.Show, Prelude.Read)
+    , newtypeDecl unrecognizedValueName
+       "Data.Int.Int32"
+        $ deriving' ["Prelude.Eq", "Prelude.Ord", "Prelude.Show, Prelude.Read"]


Can we remove the Read instance, to make sure it's not being used accidentally?

(Also because we'd like toEnum/maybeToEnum to be the only way to construct these values)

Up to you.

judah

Thanks again for taking on this change!

One small request remaining, otherwise LGTM.

judah · 2017-11-01T00:47:48Z

proto-lens-tests/tests/proto3_test.hs

  , testGroup "enum"
      [ testCase "aliases are exported" $ Foo'Enum2 @=? Foo'Enum2a
+      , serializeTo "serializeTo enum"


Ping on the first line of the above comment (unless I'm misreading the diff).

More specifically, please add this test which checks that unrecognized values round-trip through serialization as expected.

serializeTo "serializeTo enum unknown" (def & enum .~ toEnum 5 :: Foo) "enum: 5" $ tagged 6 $ VarInt 5

Fixes google#151.

Paul Kinsky added 3 commits October 26, 2017 10:59

working (or, technically, compiling)

c020f2d

working except for round trip to/from string logic

8205fb2

fix read/show issues, handle unrecognized value succ/pred

84a7a0d

Paul Kinsky added 2 commits October 26, 2017 16:47

formatting

62787e4

comments!

9454bdd

blackgnezdo requested a review from judah October 29, 2017 00:47

judah requested changes Oct 30, 2017

View reviewed changes

Paul Kinsky added 6 commits October 30, 2017 14:46

refactoring based on comments, test broken

339bec4

fix tests

bc7bdb4

fix tests

3dd6f6e

cleanup

009d8a2

cleanup

145a390

cleanup

1c1a18e

judah requested changes Oct 31, 2017

View reviewed changes

Paul Kinsky added 3 commits October 31, 2017 15:53

export unrecognized value type w/o constructors

1e0f059

nits

68d0c76

nit

251e93d

judah approved these changes Nov 1, 2017

View reviewed changes

test

f1fe93d

judah merged commit 7a6f900 into google:master Nov 1, 2017

inanna-malick deleted the sumtype-enums branch November 1, 2017 03:51

blackgnezdo pushed a commit that referenced this pull request Aug 17, 2018

Fix a small typo in the warning message of numOutputs (#151)

d79a919

avdv pushed a commit to avdv/proto-lens that referenced this pull request Aug 9, 2023

Sumtype proto3 enums

77736ac

Fixes google#151.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sumtype enums #151

Sumtype enums #151

inanna-malick commented Oct 26, 2017 •

edited by judah

googlebot commented Oct 26, 2017

inanna-malick commented Oct 30, 2017

googlebot commented Oct 30, 2017

judah Oct 27, 2017

inanna-malick Oct 30, 2017 •

edited

judah Oct 31, 2017

inanna-malick Oct 31, 2017

judah Oct 31, 2017

judah Oct 27, 2017

inanna-malick Oct 31, 2017

inanna-malick Oct 31, 2017

judah Oct 27, 2017

inanna-malick Oct 31, 2017

judah Oct 27, 2017

judah Oct 27, 2017

judah Oct 30, 2017

judah left a comment

judah Oct 31, 2017

judah Oct 31, 2017

judah Oct 31, 2017

judah Nov 1, 2017

inanna-malick Nov 1, 2017

judah Oct 31, 2017

judah Oct 31, 2017

judah Oct 31, 2017

judah left a comment

judah Nov 1, 2017


		unrecognizedValueType = tyCon $ unQual unrecognizedValueName

		constructors :: [(Name, EnumValueDescriptorProto)]

Sumtype enums #151

Sumtype enums #151

Conversation

inanna-malick commented Oct 26, 2017 • edited by judah

googlebot commented Oct 26, 2017

inanna-malick commented Oct 30, 2017

googlebot commented Oct 30, 2017

Choose a reason for hiding this comment

inanna-malick Oct 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inanna-malick commented Oct 26, 2017 •

edited by judah

inanna-malick Oct 30, 2017 •

edited