Fix issue with unknown message sizes being referred inside oneof enco…

…ded size calculation (#569)

Nanopb generator attempts to calculate the maximum encoded size of messages
when possible. If the message size is unbounded due to e.g. callbacks, the
define for it will be skipped. However, for oneofs we need to calculate the
maximum of all fields inside it, which is not easily done in C in compile time.

The current method uses sizeof(union { .. }) to calculate the largest value.
A recent change in 3ff6743 (#415, #494) moved the union to a separate line.

This in turn caused compilation errors when the union was defined but one of
the symbols it depends on (from another .proto file) was not defined.

This commit fixes it by adding #if defined() guard around the definition.

generator/nanopb_generator.py

@@ -211,33 +211,38 @@ def varint_max_size(max_value):
 class EncodedSize:
     '''Class used to represent the encoded size of a field or a message.
     Consists of a combination of symbolic sizes and integer sizes.'''
-    def __init__(self, value = 0, symbols = [], declarations = []):
+    def __init__(self, value = 0, symbols = [], declarations = [], required_defines = []):
         if isinstance(value, EncodedSize):
             self.value = value.value
             self.symbols = value.symbols
             self.declarations = value.declarations
+            self.required_defines = value.required_defines
         elif isinstance(value, strtypes + (Names,)):
             self.symbols = [str(value)]
             self.value = 0
             self.declarations = []
+            self.required_defines = [str(value)]
         else:
             self.value = value
             self.symbols = symbols
             self.declarations = declarations
+            self.required_defines = required_defines
 
     def __add__(self, other):
         if isinstance(other, int):
-            return EncodedSize(self.value + other, self.symbols, self.declarations)
+            return EncodedSize(self.value + other, self.symbols, self.declarations, self.required_defines)
         elif isinstance(other, strtypes + (Names,)):
-            return EncodedSize(self.value, self.symbols + [str(other)], self.declarations)
+            return EncodedSize(self.value, self.symbols + [str(other)], self.declarations, self.required_defines + [str(other)])
         elif isinstance(other, EncodedSize):
-            return EncodedSize(self.value + other.value, self.symbols + other.symbols, self.declarations + other.declarations)
+            return EncodedSize(self.value + other.value, self.symbols + other.symbols,
+                               self.declarations + other.declarations, self.required_defines + other.required_defines)
         else:
             raise ValueError("Cannot add size: " + repr(other))
 
     def __mul__(self, other):
         if isinstance(other, int):
-            return EncodedSize(self.value * other, [str(other) + '*' + s for s in self.symbols])
+            return EncodedSize(self.value * other, [str(other) + '*' + s for s in self.symbols],
+                               self.declarations, self.required_defines)
         else:
             raise ValueError("Cannot multiply size: " + repr(other))
 
@@ -248,8 +253,18 @@ def __str__(self):
             return '(' + str(self.value) + ' + ' + ' + '.join(self.symbols) + ')'
 
     def get_declarations(self):
+        '''Get any declarations that must appear alongside this encoded size definition,
+        such as helper union {} types.'''
         return '\n'.join(self.declarations)
 
+    def get_cpp_guard(self, local_defines):
+        '''Get an #if preprocessor statement listing all defines that are required for this definition.'''
+        needed = [x for x in self.required_defines if x not in local_defines]
+        if needed:
+            return '#if ' + ' && '.join(['defined(%s)' % x for x in needed]) + "\n"
+        else:
+            return ''
+
     def upperlimit(self):
         if not self.symbols:
             return self.value
@@ -977,7 +992,8 @@ def encoded_size(self, dependencies):
             # submessages.
             union_name = "%s_%s_size_union" % (self.struct_name, self.name)
             union_def = 'union %s {%s};\n' % (union_name, ' '.join('char f%d[%s];' % (k, s) for k,s in dynamic_sizes.items()))
-            return EncodedSize(0, ['sizeof(%s)' % union_name], [union_def])
+            required_defs = sum([s.required_defines for k,s in dynamic_sizes.items()], start = [])
+            return EncodedSize(0, ['sizeof(%s)' % union_name], [union_def], required_defs)
 
     def has_callbacks(self):
         return bool([f for f in self.fields if f.has_callbacks()])
@@ -1622,12 +1638,21 @@ def generate_header(self, includes, headername, options):
             yield '\n'
 
             yield '/* Maximum encoded size of messages (where known) */\n'
+            messagesizes = []
             for msg in self.messages:
-                msize = msg.encoded_size(self.dependencies)
                 identifier = '%s_size' % msg.name
+                messagesizes.append((identifier, msg.encoded_size(self.dependencies)))
+
+            # If we require a symbol from another file, put a preprocessor if statement
+            # around it to prevent compilation errors if the symbol is not actually available.
+            local_defines = [identifier for identifier, msize in messagesizes if msize is not None]
+            for identifier, msize in messagesizes:
                 if msize is not None:
+                    cpp_guard = msize.get_cpp_guard(local_defines)
+                    yield cpp_guard
                     yield msize.get_declarations()
                     yield '#define %-40s %s\n' % (identifier, msize)
+                    if cpp_guard: yield "#endif\n"
                 else:
                     yield '/* %s depends on runtime parameters */\n' % identifier
             yield '\n'

tests/regression/issue_569/SConscript

@@ -0,0 +1,16 @@
+# Regression test for #569:
+# Compilation error when dependent message size is not defined.
+
+Import("env")
+
+# For C compiler
+env.NanopbProto("a")
+env.NanopbProto("b")
+env.Object("a.pb.c")
+env.Object("b.pb.c")
+
+# For C++ compiler
+env.NanopbProtoCpp("a")
+env.NanopbProtoCpp("b")
+env.Object("a_cpp.o", "a.pb.cpp")
+env.Object("b_cpp.o", "b.pb.cpp")

tests/regression/issue_569/a.proto

@@ -0,0 +1,14 @@
+syntax = "proto3";
+
+package a;
+
+message AUnknown {
+  string data = 1;
+}
+
+message A {
+  oneof data {
+     AUnknown unknown = 1;
+     int32 x = 2;
+  }
+}

tests/regression/issue_569/b.proto

@@ -0,0 +1,19 @@
+syntax = "proto3";
+
+import "a.proto";
+
+package b;
+
+message B {
+  oneof data {
+     a.AUnknown unknown = 1;
+     int32 x = 2;
+  }
+}
+
+message B2 {
+  oneof data {
+     a.AUnknown unknown = 1;
+     a.A unknown2 = 2;
+  }
+}