fix negative tests, allow devel fails, CI updates (#38)

.github/workflows/ci.yml

@@ -6,6 +6,10 @@ on:
   pull_request:
   workflow_dispatch:
 
+concurrency: # Cancel stale PR builds (but not push builds)
+  group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }}
+  cancel-in-progress: true
+
 jobs:
   build:
     strategy:
@@ -43,9 +47,10 @@ jobs:
 
     name: '${{ matrix.target.os }}-${{ matrix.target.cpu }} (Nim ${{ matrix.branch }})'
     runs-on: ${{ matrix.builder }}
+    continue-on-error: ${{ matrix.branch == 'devel' }}
     steps:
       - name: Checkout
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
 
       - name: Install build dependencies (Linux i386)
         if: runner.os == 'Linux' && matrix.target.cpu == 'i386'
@@ -93,23 +98,23 @@ jobs:
         id: windows-dlls-cache
         uses: actions/cache@v2
         with:
-          path: external/dlls
-          key: 'dlls'
+          path: external/dlls-${{ matrix.target.cpu }}
+          key: 'dlls-${{ matrix.target.cpu }}'
 
-      - name: Install DLL dependencies (Windows)
+      - name: Install DLLs dependencies (Windows)
         if: >
           steps.windows-dlls-cache.outputs.cache-hit != 'true' &&
           runner.os == 'Windows'
         run: |
-          mkdir external
+          mkdir -p external
           curl -L "https://nim-lang.org/download/windeps.zip" -o external/windeps.zip
-          7z x external/windeps.zip -oexternal/dlls
+          7z x -y external/windeps.zip -oexternal/dlls-${{ matrix.target.cpu }}
 
       - name: Path to cached dependencies (Windows)
         if: >
           runner.os == 'Windows'
         run: |
-          echo '${{ github.workspace }}'"/external/dlls" >> $GITHUB_PATH
+          echo "${{ github.workspace }}/external/dlls-${{ matrix.target.cpu }}" >> $GITHUB_PATH
 
       - name: Derive environment variables
         run: |
@@ -155,5 +160,5 @@ jobs:
           nim --version
           nimble --version
           nimble install -y --depsOnly
-          env TEST_LANG="c" nimble test
-          env TEST_LANG="cpp" nimble test
+          env NIMLANG="c" nimble test
+          env NIMLANG="cpp" nimble test

.gitignore

@@ -1,2 +1,4 @@
 nimcache/
 *.exe
+build/
+nimbus-build-system.paths

protobuf_serialization.nimble

@@ -2,6 +2,7 @@ import os, strutils
 
 mode = ScriptMode.Verbose
 
+packageName   = "protobuf_serialization"
 version       = "0.3.0"
 author        = "Status"
 description   = "Protobuf implementation compatible with the nim-serialization framework."
@@ -15,20 +16,26 @@ requires "nim >= 1.2.0",
          "combparser",
          "unittest2"
 
-const styleCheckStyle =
-  if (NimMajor, NimMinor) < (1, 6):
-    "hint"
-  else:
-    "error"
+let nimc = getEnv("NIMC", "nim") # Which nim compiler to use
+let lang = getEnv("NIMLANG", "c") # Which backend (c/cpp/js)
+let flags = getEnv("NIMFLAGS", "") # Extra flags for the compiler
+let verbose = getEnv("V", "") notin ["", "0"]
 
-proc test(args, path: string) =
-  exec "nim " & getEnv("TEST_LANG", "c") & " " & getEnv("NIMFLAGS") & " " & args &
-    " -r --hints:off --skipParentCfg --styleCheck:usages --styleCheck:" & styleCheckStyle & " " & path
+let styleCheckStyle = if (NimMajor, NimMinor) < (1, 6): "hint" else: "error"
+let cfg =
+  " --styleCheck:usages --styleCheck:" & styleCheckStyle &
+  (if verbose: "" else: " --verbosity:0 --hints:off") &
+  " --skipParentCfg --skipUserCfg --outdir:build --nimcache:build/nimcache -f"
+
+proc build(args, path: string) =
+  exec nimc & " " & lang & " " & cfg & " " & flags & " " & args & " " & path
+
+proc run(args, path: string) =
+  build args & " -r", path
 
 task test, "Run all tests":
-  #Explicitly specify the call depth limit in case the default changes in the future.
-  test "--threads:off", "tests/test_all"
-  test "--threads:on", "tests/test_all"
+  for threads in ["--threads:off", "--threads:on"]:
+    run threads, "tests/test_all"
 
   #Also iterate over every test in tests/fail, and verify they fail to compile.
   echo "\r\n\x1B[0;94m[Suite]\x1B[0;37m Test Fail to Compile"
@@ -37,8 +44,9 @@ task test, "Run all tests":
     if path.split(".")[^1] != "nim":
       continue
 
-    if gorgeEx("nim c " & path).exitCode != 0:
+    if gorgeEx(nimc & " c " & path).exitCode != 0:
       echo "  \x1B[0;92m[OK]\x1B[0;37m ", path.split(DirSep)[^1]
     else:
       echo "  \x1B[0;31m[FAILED]\x1B[0;37m ", path.split(DirSep)[^1]
       exec "exit 1"
+

protobuf_serialization/internal.nim

@@ -28,6 +28,9 @@ macro unsupportedProtoType*(FieldType, RootType, fieldName: typed): untyped =
   # TODO fix RootType printing
   error "Serializing " & humaneTypeName(FieldType) & " as field type is not supported: " & humaneTypeName(RootType) & "." & repr(fieldName)
 
+template fieldError(T: type, name, msg: static string) =
+  {.fatal: $T & "." & name & ": " & msg.}
+
 proc isProto2*(T: type): bool {.compileTime.} = T.hasCustomPragma(proto2)
 proc isProto3*(T: type): bool {.compileTime.} = T.hasCustomPragma(proto3)
 
@@ -45,46 +48,67 @@ proc isRequired*(T: type, fieldName: static string): bool {.compileTime.} =
   T.hasCustomPragmaFixed(fieldName, required)
 
 proc fieldNumberOf*(T: type, fieldName: static string): int {.compileTime.} =
-  T.getCustomPragmaFixed(fieldName, fieldNumber)
+  const fieldNum = T.getCustomPragmaFixed(fieldName, fieldNumber)
+  when fieldNum is NimNode:
+    fieldError T, fieldName, "Missing {.fieldNumber: N.}"
+  else:
+    fieldNum
 
 template protoType*(InnerType, RootType, FieldType: untyped, fieldName: untyped) =
   mixin flatType
+
   when FieldType is seq and FieldType isnot seq[byte]:
     type FlatType = flatType(default(typeof(for a in default(FieldType): a)))
   else:
     type FlatType = flatType(default(FieldType))
+
+  const
+    isPint = RootType.hasCustomPragmaFixed(fieldName, pint)
+    isSint = RootType.hasCustomPragmaFixed(fieldName, sint)
+    isFixed = RootType.hasCustomPragmaFixed(fieldName, fixed)
+    isInteger =
+      (FlatType is int32) or (FlatType is int64) or
+      (FlatType is uint32) or (FlatType) is uint64
+
+  when ord(isPint) + ord(isSint) + ord(isFixed) != ord(isInteger):
+    when isInteger:
+      fieldError RootType, fieldName, "Must specify one of `pint`, `sint` and `fixed`"
+    else:
+      fieldError RootType, fieldName, "`pint`, `sint` and `fixed` should only be used with integers"
+
   when FlatType is float64:
     type InnerType = pdouble
   elif FlatType is float32:
     type InnerType = pfloat
   elif FlatType is int32:
-    when RootType.hasCustomPragmaFixed(fieldName, pint):
+    when isPint:
       type InnerType = pint32
-    elif RootType.hasCustomPragmaFixed(fieldName, sint):
+    elif isSint:
       type InnerType = sint32
-    elif RootType.hasCustomPragmaFixed(fieldName, fixed):
+    else:
       type InnerType = sfixed32
-    else:
-      {.fatal: "Must annotate `int32` fields with `pint`, `sint` or `fixed`".}
   elif FlatType is int64:
-    when RootType.hasCustomPragmaFixed(fieldName, pint):
+    when isPint:
       type InnerType = pint64
-    elif RootType.hasCustomPragmaFixed(fieldName, sint):
+    elif isSint:
       type InnerType = sint64
-    elif RootType.hasCustomPragmaFixed(fieldName, fixed):
+    else:
       type InnerType = sfixed64
-    else:
-      {.fatal: "Must annotate `int64` fields with `pint`, `sint` or `fixed`".}
   elif FlatType is uint32:
-    when RootType.hasCustomPragmaFixed(fieldName, fixed):
-      type InnerType = fixed32
-    else:
+    when isPint:
       type InnerType = puint32
-  elif FlatType is uint64:
-    when RootType.hasCustomPragmaFixed(fieldName, fixed):
-      type InnerType = fixed64
+    elif isSint:
+      fieldError RootType, fieldName, "Must not annotate `uint32` fields with `sint`"
     else:
+      type InnerType = fixed32
+  elif FlatType is uint64:
+    when isPint:
       type InnerType = puint64
+    elif isSint:
+      fieldError RootType, fieldName, "Must not annotate `uint64` fields with `sint`"
+    else:
+      type InnerType = fixed64
+
   elif FlatType is bool:
     type InnerType = pbool
   elif FlatType is string:
@@ -101,42 +125,40 @@ template elementType[T](_: type seq[T]): type = typeof(T)
 func verifySerializable*[T](ty: typedesc[T]) {.compileTime.} =
   type FlatType = flatType(default(T))
   when FlatType is int | uint:
-    {.fatal: "Serializing a number requires specifying the amount of bits via the type.".}
+    {.fatal: $T & ": Serializing a number requires specifying the amount of bits via the type.".}
   elif FlatType is seq:
-    verifySerializable(elementType(T))
-  elif FlatType is object:
+    when FlatType isnot seq[byte]:
+      verifySerializable(elementType(FlatType))
+  elif FlatType is object and T isnot PBOption:
     var
       inst: T
       fieldNumberSet = initHashSet[int]()
     discard fieldNumberSet
-
     const
       isProto2 = T.isProto2()
       isProto3 = T.isProto3()
     when isProto2 == isProto3:
-      {.fatal: "Serialized objects must have either the proto2 or proto3 pragma attached.".}
+      {.fatal: $T & ": missing {.proto2.} or {.proto3}".}
 
     enumInstanceSerializedFields(inst, fieldName, fieldVar):
       when isProto2 and not T.isRequired(fieldName):
         when fieldVar is not seq:
           when fieldVar is not PBOption:
-            {.fatal: "proto2 requires every field to either have the required pragma attached or be a repeated field/PBOption.".}
+            fieldError T, fieldName, "proto2 requires every field to either have the required pragma attached or be a repeated field/PBOption."
       when isProto3 and (
         T.hasCustomPragmaFixed(fieldName, required) or
         (fieldVar is PBOption)
       ):
-        {.fatal: "The required pragma/PBOption type can only be used with proto2.".}
+        fieldError T, fieldName, "The required pragma/PBOption type can only be used with proto2."
 
       protoType(ProtoType {.used.}, T, typeof(fieldVar), fieldName) # Ensure we can form a ProtoType
 
       const fieldNum = T.fieldNumberOf(fieldName)
-      when fieldNum is NimNode:
-        {.fatal: "No field number specified on serialized field.".}
-      else:
-        when not validFieldNumber(fieldNum, strict = true):
-          {.fatal: "Field numbers must be in the range [1..2^29-1]".}
+      when not validFieldNumber(fieldNum, strict = true):
+        fieldError T, fieldName, "Field numbers must be in the range [1..2^29-1]"
 
-        if fieldNumberSet.containsOrIncl(fieldNum):
-          raiseAssert "Field number was used twice on two different fields: " & $fieldNum
+      if fieldNumberSet.containsOrIncl(fieldNum):
+        raiseAssert $T & "." & fieldName & ": Field number was used twice on two different fields: " & $fieldNum
 
-      # verifySerializable(typeof(fieldVar))
+      type FieldType = typeof(fieldVar)
+      verifySerializable(FieldType)

tests/fail/test_invalid_byte_encoding.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type InvalidByteEncoding = object
+type InvalidByteEncoding {.proto3.} = object
   x {.sint, fieldNumber: 1.}: uint8
 
 discard Protobuf.encode(InvalidByteEncoding())

tests/fail/test_invalid_float_bits.nim

@@ -1,6 +0,0 @@
-import ../../protobuf_serialization
-
-type InvalidFloatBits = object
-  x {.pfloat32, fieldNumber: 1.}: float64
-
-discard Protobuf.encode(InvalidFloatBits())

tests/fail/test_invalid_float_encoding.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type InvalidFloatEncoding = object
-  x {.pint, pfloat32, fieldNumber: 1.}: float32
+type InvalidFloatEncoding {.proto3.} = object
+  x {.pint, fieldNumber: 1.}: float32
 
 discard Protobuf.encode(InvalidFloatEncoding())

tests/fail/test_invalid_nested_object.nim

@@ -1,10 +1,10 @@
 import ../../protobuf_serialization
 
 type
-  X = object
+  X {.proto3.} = object
     y {.pint, sint, fieldNumber: 1.}: int32
 
-  A = object
+  A {.proto3.} = object
     b {.fieldNumber: 1.}: X
 
 discard Protobuf.encode(A())

tests/fail/test_invalid_uint_encoding.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type InvalidUIntEncoding = object
+type InvalidUIntEncoding {.proto3.} = object
   x {.sint, fieldNumber: 1.}: uint32
 
 discard Protobuf.encode(InvalidUIntEncoding())

tests/fail/test_multiple_int_encodings.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type MultipleIntEncodings = object
+type MultipleIntEncodings {.proto3.} = object
   x {.pint, sint, fieldNumber: 1.}: int32
 
 discard Protobuf.encode(MultipleIntEncodings())

tests/fail/test_multiple_uint_encodings.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type MultipleUIntEncodings = object
+type MultipleUIntEncodings {.proto3.} = object
   x {.pint, fixed, fieldNumber: 1.}: uint32
 
 discard Protobuf.encode(MultipleUIntEncodings())

tests/fail/test_negative_field_number.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type NegativeFieldNumber = object
+type NegativeFieldNumber {.proto3.} = object
   x {.fieldNumber: -1.}: bool
 
 discard Protobuf.encode(NegativeFieldNumber())

tests/fail/test_no_field_number.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type NoFieldNumber = object
+type NoFieldNumber {.proto3.} = object
   x: bool
 
 discard Protobuf.encode(NoFieldNumber())

tests/fail/test_no_int_encoding.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type NoIntEncoding = object
+type NoIntEncoding {.proto3.} = object
   x {.fieldNumber: 1.}: int32
 
 discard Protobuf.encode(NoIntEncoding())

tests/fail/test_no_uint_encoding.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type NoUIntEncoding = object
+type NoUIntEncoding {.proto3.} = object
   x {.fieldNumber: 1.}: uint32
 
 discard Protobuf.encode(NoUIntEncoding())

tests/fail/test_reused_field_number.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type ReusedFieldNumber = object
+type ReusedFieldNumber {.proto3.} = object
   x {.fieldNumber: 1.}: bool
   y {.fieldNumber: 1.}: bool
 

tests/fail/test_specified_byte_encoding.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type SpecifiedByteEncoding = object
+type SpecifiedByteEncoding {.proto3.} = object
   x {.pint, fieldNumber: 1.}: uint8
 
 discard Protobuf.encode(SpecifiedByteEncoding())

tests/fail/test_specified_float_encoding.nim

@@ -1,6 +0,0 @@
-import ../../protobuf_serialization
-
-type SpecifiedFloatEncoding = object
-  x {.fixed, fieldNumber: 1.}: float32
-
-discard Protobuf.encode(SpecifiedFloatEncoding())

tests/fail/test_table.nim

@@ -1,5 +0,0 @@
-import tables
-
-import ../../protobuf_serialization
-
-discard Protobuf.encode([(5, 5)].toTable())

tests/fail/test_too_high_field_number.nim

@@ -1,10 +1,7 @@
 import ../../protobuf_serialization
 
-#The field number must fix into 2^28.
-#A signed int has the first bit unavailable, lowering the available bits to 2^31.
-#Then encoding the wire type requires shifting left 3 bits.
-#That leaves you with 2^27 for consistency on all platforms without slow extensions
-type TooHighFieldNumber = object
-  x {.fieldNumber: 268435457.}: bool
+# https://developers.google.com/protocol-buffers/docs/proto3#assigning_field_numbers
+type TooHighFieldNumber {.proto3.} = object
+  x {.fieldNumber: 536870912.}: bool
 
 discard Protobuf.encode(TooHighFieldNumber())

tests/fail/test_unspecified_float_bits.nim

@@ -1,6 +0,0 @@
-import ../../protobuf_serialization
-
-type UnspecifiedFloatBits = object
-  x {.fieldNumber: 1.}: float64
-
-discard Protobuf.encode(UnspecifiedFloatBits())

tests/fail/test_unspecified_int_bits.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type UnspecifiedIntBits = object
+type UnspecifiedIntBits {.proto3.} = object
   x {.sint, fieldNumber: 1.}: int
 
 discard Protobuf.encode(UnspecifiedIntBits())

tests/fail/test_unspecified_uint_bits.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type UnspecifiedUIntBits = object
+type UnspecifiedUIntBits {.proto3.} = object
   x {.pint, fieldNumber: 1.}: uint
 
 discard Protobuf.encode(UnspecifiedUIntBits())

tests/fail/test_zero_field_number.nim

@@ -1,6 +1,6 @@
 import ../../protobuf_serialization
 
-type ZeroFieldNumber = object
+type ZeroFieldNumber  {.proto3.} = object
   x {.fieldNumber: 0.}: bool
 
 discard Protobuf.encode(ZeroFieldNumber())