search

protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.39k stars 15.46k forks source link

"assert" as variable name in proto file causes syntax errors in generated c++ code #12010

Closed Drevanoorschot closed 1 year ago

Drevanoorschot commented 1 year ago

What version of protobuf and what language are you using? Version: main/v22.0 Language: C++

What operating system (Linux, Windows, ...) and version? Debian GNU/Linux 11 (bullseye) x86_64

What runtime / compiler are you using (e.g., python version or gcc version) gcc (Debian 10.2.1-6) 10.2.1 20210110 cmake version 3.23.2 ninja version 1.10.2 clion 2022.2.4

What did you do? Steps to reproduce the behavior:

set(CMAKE_CXX_STANDARD 20)

find_package(Protobuf REQUIRED PATHS /usr/local/lib/cmake/protobuf)

add_library(proto-objects OBJECT "proto/COL/col.proto")

target_link_libraries(proto-objects PUBLIC protobuf::libprotobuf)

set(PROTO_BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/generated")

target_include_directories(proto-objects PUBLIC "$<BUILD_INTERFACE:${PROTO_BINARY_DIR}>")

protobuf_generate( TARGET proto-objects IMPORT_DIRS "proto" PROTOC_OUT_DIR "${PROTO_BINARY_DIR}")

set(SOURCE_FILES tools/VcLLVM/VcLLVM.cpp) add_executable(VcLLVM ${SOURCE_FILES}) target_link_libraries(VcLLVM proto-objects)

```protobuf
// proto/COL/col.proto

syntax = "proto2";

message Statement {
  oneof v {
    Assert assert = 1;
  }
}

message Assert {
  required string x = 1;
}
// tools/VcLLVM/VcLLVM.cpp
#include <iostream>

int main(int argc, char **argv) {
    std::cout << "whatever\n";
    return 0;
}

What did you expect to see The following output:

[3/3] Linking CXX executable VcLLVM

Build finished

What did you see instead? The following output:

FAILED: CMakeFiles/proto-objects.dir/generated/COL/col.pb.cc.o 
/usr/bin/c++  -I/home/dre/Projects/afstuderen/VcLLVM/_build/generated -g -std=gnu++2a -MD -MT CMakeFiles/proto-objects.dir/generated/COL/col.pb.cc.o -MF CMakeFiles/proto-objects.dir/generated/COL/col.pb.cc.o.d -o CMakeFiles/proto-objects.dir/generated/COL/col.pb.cc.o -c /home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc
In file included from /usr/include/c++/10/cassert:44,
                 from /usr/local/include/google/protobuf/extension_set.h:42,
                 from /home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.h:32,
                 from /home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc:4:
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.h:213:19: error: expected unqualified-id before ‘static_cast’
  213 |   const ::Assert& assert() const;
      |                   ^~~~~~
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.h:213:19: error: expected ‘)’ before ‘static_cast’
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.h:470:35: error: expected unqualified-id before ‘(’ token
  470 | inline const ::Assert& Statement::assert() const {
      |                                   ^~~~~~
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc:140:26: error: expected unqualified-id before ‘static_cast’
  140 |   static const ::Assert& assert(const Statement* msg);
      |                          ^~~~~~
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc:140:26: error: expected ‘)’ before ‘static_cast’
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc:144:23: error: expected unqualified-id before ‘(’ token
  144 | Statement::_Internal::assert(const Statement* msg) {
      |                       ^~~~~~
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc: In member function ‘virtual uint8_t* Statement::_InternalSerialize(uint8_t*, google::protobuf::io::EpsCopyOutputStream*) const’:
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc:294:42: error: expected unqualified-id before ‘(’ token
  294 |       InternalWriteMessage(1, _Internal::assert(this),
      |                                          ^~~~~~
/home/dre/Projects/afstuderen/VcLLVM/_build/generated/COL/col.pb.cc:295:20: error: expected unqualified-id before ‘(’ token
  295 |         _Internal::assert(this).GetCachedSize(), target, stream);
      |                    ^~~~~~
ninja: build stopped: subcommand failed.

Anything else we should know about your project / environment It is worth noting that when i change the proto variable assert to _assert the project compiles just fine. I'm assuming the assert keyword is not escaped properly during code generation and is therefore causing problems. However, I'm very new to both C++ programming and protobuf so i might be complety off.

fowles commented 1 year ago

Technically assert is a macro and not a keyword. http://go/cppref/cpp/error/assert

This means we have two approaches we could use to solve this.

1) treat it like other macros that have frequent collisions (https://cs.github.com/protocolbuffers/protobuf/blob/cffde99fc6195ca108f320c16a6ded41c42a3317/src/google/protobuf/port_def.inc?q=path%3Aport_def.inc#L950-L953) 2) treat it as a keyword (https://cs.github.com/protocolbuffers/protobuf/blob/cffde99fc6195ca108f320c16a6ded41c42a3317/src/google/protobuf/compiler/cpp/helpers.cc?q=path%3Acpp%2Fhelpers.cc#L89)

I don't particularly love either option. (1) feels more principled, but users will have a hard time accessing such fields.

Drevanoorschot commented 1 year ago

Just out of curiosity, what makes the assert macro different from the other macros that are being undefined? (https://cs.github.com/protocolbuffers/protobuf/blob/cffde99fc6195ca108f320c16a6ded41c42a3317/src/google/protobuf/port_def.inc?q=path%3Aport_def.inc#L950-L953)

As in, what makes it more troublesome to undefine assert as opposed to the min and max example?

fowles commented 1 year ago

We actually talked about whether to handle it like a macro or a keyword in the team chat. We ended up deciding on keyword because the assert macro is pulled into a great many headers transitively. min and max on the other hand are only defined as macros on windows, so the magnitude of their scope is significantly smaller.