Skip to content

Add configurable warnings for ignored protobuf keywords #9045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dineshkumar227 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add configurable warnings for ignored protobuf keywords #9045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions include/flatbuffers/idl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,9 @@ struct IDLOptions {

// field case style options for C++
enum CaseStyle { CaseStyle_Unchanged = 0, CaseStyle_Upper, CaseStyle_Lower };
enum class ProtoIdGapAction { NO_OP, WARNING, ERROR };
enum class ProtoAction { NO_OP, WARNING, ERROR };
static bool ParseProtoAction(const char* str, ProtoAction& action);

bool gen_jvmstatic;
// Use flexbuffers instead for binary and text generation
bool use_flexbuffers;
Expand Down Expand Up @@ -744,7 +746,10 @@ struct IDLOptions {

bool ts_omit_entrypoint;
bool ts_undefined_for_optionals;
ProtoIdGapAction proto_id_gap_action;
ProtoAction proto_id_gap_action;
ProtoAction proto_option_action;
ProtoAction proto_service_action;
ProtoAction proto_extensions_action;

// Possible options for the more general generator below.
enum Language {
Expand Down Expand Up @@ -871,7 +876,10 @@ struct IDLOptions {
python_gen_numpy(true),
ts_omit_entrypoint(false),
ts_undefined_for_optionals(false),
proto_id_gap_action(ProtoIdGapAction::WARNING),
proto_id_gap_action(ProtoAction::WARNING),
proto_option_action(ProtoAction::NO_OP),
proto_service_action(ProtoAction::NO_OP),
proto_extensions_action(ProtoAction::NO_OP),
mini_reflect(IDLOptions::kNone),
require_explicit_ids(false),
rust_serialize(false),
Expand Down Expand Up @@ -1180,6 +1188,27 @@ class Parser : public ParserState {
EnumDef** dest);
FLATBUFFERS_CHECKED_ERROR ParseDecl(const char* filename);
FLATBUFFERS_CHECKED_ERROR ParseService(const char* filename);
struct ProtoIgnoredInfo {
enum class Keyword : uint8_t {
kOption,
kService,
kExtensions,
};

enum class Scope : uint8_t {
kTopLevel,
kMessage,
};

static const char* KeywordName(Keyword keyword);
static const char* ScopeName(Scope scope);
};

FLATBUFFERS_CHECKED_ERROR HandleIgnoredProtoKeyword(
ProtoIgnoredInfo::Keyword keyword, ProtoIgnoredInfo::Scope scope);
IDLOptions::ProtoAction GetProtoIgnoredAction(
ProtoIgnoredInfo::Keyword keyword) const;

FLATBUFFERS_CHECKED_ERROR ParseProtoFields(StructDef* struct_def,
bool isextend, bool inside_oneof);
FLATBUFFERS_CHECKED_ERROR ParseProtoMapField(StructDef* struct_def);
Expand Down
51 changes: 42 additions & 9 deletions src/flatc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,22 @@

namespace flatbuffers {

bool IDLOptions::ParseProtoAction(const char* str, ProtoAction& action) {
if (!strcmp(str, "nop")) {
action = ProtoAction::NO_OP;
return true;
}
if (!strcmp(str, "warn")) {
action = ProtoAction::WARNING;
return true;
}
if (!strcmp(str, "error")) {
action = ProtoAction::ERROR;
return true;
}
return false;
}

static const char* FLATC_VERSION() { return FLATBUFFERS_VERSION(); }

void FlatCompiler::ParseFile(
Expand Down Expand Up @@ -186,6 +202,15 @@ const static FlatCOption flatc_options[] = {
"* 'error' - An error message will be shown and the fbs generation will "
"be "
"interrupted."},
{"", "proto-option", "",
"Action when a protobuf `option` keyword is ignored during conversion. "
"Supported values: 'nop' (default), 'warn', 'error'."},
{"", "proto-service", "",
"Action when a protobuf `service` keyword is ignored during conversion. "
"Supported values: 'nop' (default), 'warn', 'error'."},
{"", "proto-extensions", "",
"Action when a protobuf `extensions` keyword is ignored during conversion. "
"Supported values: 'nop' (default), 'warn', 'error'."},
{"", "grpc", "", "Generate GRPC interfaces for the specified languages."},
{"", "schema", "", "Serialize schemas instead of JSON (use with -b)."},
{"", "bfbs-filenames", "PATH",
Expand Down Expand Up @@ -602,15 +627,23 @@ FlatCOptions FlatCompiler::ParseFromCommandLineArguments(int argc,
} else if (arg == "--keep-proto-id") {
opts.keep_proto_id = true;
} else if (arg == "--proto-id-gap") {
if (++argi >= argc) Error("missing case style following: " + arg, true);
if (!strcmp(argv[argi], "nop"))
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
else if (!strcmp(argv[argi], "warn"))
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::WARNING;
else if (!strcmp(argv[argi], "error"))
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::ERROR;
else
Error("unknown case style: " + std::string(argv[argi]), true);
if (++argi >= argc) Error("missing action following: " + arg, true);
if (!IDLOptions::ParseProtoAction(argv[argi], opts.proto_id_gap_action))
Error("unknown action: " + std::string(argv[argi]), true);
} else if (arg == "--proto-option") {
if (++argi >= argc) Error("missing action following: " + arg, true);
if (!IDLOptions::ParseProtoAction(argv[argi], opts.proto_option_action))
Error("unknown action: " + std::string(argv[argi]), true);
} else if (arg == "--proto-service") {
if (++argi >= argc) Error("missing action following: " + arg, true);
if (!IDLOptions::ParseProtoAction(argv[argi],
opts.proto_service_action))
Error("unknown action: " + std::string(argv[argi]), true);
} else if (arg == "--proto-extensions") {
if (++argi >= argc) Error("missing action following: " + arg, true);
if (!IDLOptions::ParseProtoAction(argv[argi],
opts.proto_extensions_action))
Error("unknown action: " + std::string(argv[argi]), true);
} else if (arg == "--schema") {
options.schema_binary = true;
} else if (arg == "-M") {
Expand Down
8 changes: 4 additions & 4 deletions src/idl_gen_fbs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ static bool HasGapInProtoId(const std::vector<FieldDef*>& fields) {
}

static bool ProtobufIdSanityCheck(const StructDef& struct_def,
IDLOptions::ProtoIdGapAction gap_action,
IDLOptions::ProtoAction gap_action,
bool no_log = false) {
const auto& fields = struct_def.fields.vec;
if (HasNonPositiveFieldId(fields)) {
Expand Down Expand Up @@ -173,14 +173,14 @@ static bool ProtobufIdSanityCheck(const StructDef& struct_def,
return false;
}

if (gap_action != IDLOptions::ProtoIdGapAction::NO_OP) {
if (gap_action != IDLOptions::ProtoAction::NO_OP) {
if (HasGapInProtoId(fields)) {
// TODO: Use LogCompilerWarn
if (!no_log) {
fprintf(stderr, "Fields in struct %s have gap between ids\n",
struct_def.name.c_str());
}
if (gap_action == IDLOptions::ProtoIdGapAction::ERROR) {
if (gap_action == IDLOptions::ProtoAction::ERROR) {
return false;
}
}
Expand All @@ -199,7 +199,7 @@ struct ProtobufToFbsIdMap {
};

static ProtobufToFbsIdMap MapProtoIdsToFieldsId(
const StructDef& struct_def, IDLOptions::ProtoIdGapAction gap_action,
const StructDef& struct_def, IDLOptions::ProtoAction gap_action,
bool no_log) {
const auto& fields = struct_def.fields.vec;

Expand Down
77 changes: 73 additions & 4 deletions src/idl_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3049,6 +3049,67 @@ CheckedError Parser::ParseNamespace() {
return NoError();
}

const char* Parser::ProtoIgnoredInfo::KeywordName(Keyword keyword) {
switch (keyword) {
case Keyword::kOption:
return "option";
case Keyword::kService:
return "service";
case Keyword::kExtensions:
return "extensions";
}
FLATBUFFERS_ASSERT(false);
return "unknown";
}

const char* Parser::ProtoIgnoredInfo::ScopeName(Scope scope) {
switch (scope) {
case Scope::kTopLevel:
return "top-level scope";
case Scope::kMessage:
return "message scope";
}
FLATBUFFERS_ASSERT(false);
return "unknown";
}

IDLOptions::ProtoAction Parser::GetProtoIgnoredAction(
ProtoIgnoredInfo::Keyword keyword) const {
switch (keyword) {
case ProtoIgnoredInfo::Keyword::kOption:
return opts.proto_option_action;
case ProtoIgnoredInfo::Keyword::kService:
return opts.proto_service_action;
case ProtoIgnoredInfo::Keyword::kExtensions:
return opts.proto_extensions_action;
}
FLATBUFFERS_ASSERT(false);
return IDLOptions::ProtoAction::NO_OP;
}

CheckedError Parser::HandleIgnoredProtoKeyword(
ProtoIgnoredInfo::Keyword keyword, ProtoIgnoredInfo::Scope scope) {
const auto action = GetProtoIgnoredAction(keyword);

const std::string message =
"ignoring unsupported protobuf keyword `" +
std::string(ProtoIgnoredInfo::KeywordName(keyword)) + "` in " +
ProtoIgnoredInfo::ScopeName(scope);

switch (action) {
case IDLOptions::ProtoAction::NO_OP:
return NoError();
case IDLOptions::ProtoAction::WARNING:
Warning(message);
return NoError();
case IDLOptions::ProtoAction::ERROR:
return Error(message);
}

FLATBUFFERS_ASSERT(false);
return Error("internal error: unknown proto ignored action");
}

// Best effort parsing of .proto declarations, with the aim to turn them
// in the closest corresponding FlatBuffer equivalent.
// We parse everything as identifiers instead of keywords, since we don't
Expand Down Expand Up @@ -3103,10 +3164,14 @@ CheckedError Parser::ParseProtoDecl() {
EXPECT('=');
EXPECT(kTokenStringConstant);
EXPECT(';');
} else if (IsIdent("option")) { // Skip these.
} else if (IsIdent("option")) {
ECHECK(HandleIgnoredProtoKeyword(ProtoIgnoredInfo::Keyword::kOption,
ProtoIgnoredInfo::Scope::kTopLevel));
ECHECK(ParseProtoOption());
EXPECT(';');
} else if (IsIdent("service")) { // Skip these.
} else if (IsIdent("service")) {
ECHECK(HandleIgnoredProtoKeyword(ProtoIgnoredInfo::Keyword::kService,
ProtoIgnoredInfo::Scope::kTopLevel));
NEXT();
EXPECT(kTokenIdentifier);
ECHECK(ParseProtoCurliesOrIdent());
Expand Down Expand Up @@ -3142,15 +3207,19 @@ CheckedError Parser::ParseProtoFields(StructDef* struct_def, bool isextend,
if (IsIdent("message") || IsIdent("extend") || IsIdent("enum")) {
// Nested declarations.
ECHECK(ParseProtoDecl());
} else if (IsIdent("extensions")) { // Skip these.
} else if (IsIdent("extensions")) {
ECHECK(HandleIgnoredProtoKeyword(ProtoIgnoredInfo::Keyword::kExtensions,
ProtoIgnoredInfo::Scope::kMessage));
NEXT();
EXPECT(kTokenIntegerConstant);
if (Is(kTokenIdentifier)) {
NEXT(); // to
NEXT(); // num
}
EXPECT(';');
} else if (IsIdent("option")) { // Skip these.
} else if (IsIdent("option")) {
ECHECK(HandleIgnoredProtoKeyword(ProtoIgnoredInfo::Keyword::kOption,
ProtoIgnoredInfo::Scope::kMessage));
ECHECK(ParseProtoOption());
EXPECT(';');
} else if (IsIdent("reserved")) { // Skip these.
Expand Down
22 changes: 11 additions & 11 deletions tests/proto_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ void proto_test(const std::string& proto_path, const std::string& proto_file) {
flatbuffers::IDLOptions opts;
opts.include_dependence_headers = false;
opts.proto_mode = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

// load the .proto and the golden file from disk
std::string golden_file;
Expand All @@ -69,7 +69,7 @@ void proto_test_id(const std::string& proto_path,
opts.include_dependence_headers = false;
opts.proto_mode = true;
opts.keep_proto_id = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

// load the .proto and the golden file from disk
std::string golden_file;
Expand All @@ -87,7 +87,7 @@ void proto_test_union(const std::string& proto_path,
opts.include_dependence_headers = false;
opts.proto_mode = true;
opts.proto_oneof_union = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(flatbuffers::LoadFile((proto_path + "test_union.golden.fbs").c_str(),
Expand All @@ -104,7 +104,7 @@ void proto_test_union_id(const std::string& proto_path,
opts.proto_mode = true;
opts.proto_oneof_union = true;
opts.keep_proto_id = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(
Expand All @@ -121,7 +121,7 @@ void proto_test_union_suffix(const std::string& proto_path,
opts.proto_mode = true;
opts.proto_namespace_suffix = "test_namespace_suffix";
opts.proto_oneof_union = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(flatbuffers::LoadFile(
Expand All @@ -139,7 +139,7 @@ void proto_test_union_suffix_id(const std::string& proto_path,
opts.proto_namespace_suffix = "test_namespace_suffix";
opts.proto_oneof_union = true;
opts.keep_proto_id = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(flatbuffers::LoadFile(
Expand All @@ -155,7 +155,7 @@ void proto_test_include(const std::string& proto_path,
flatbuffers::IDLOptions opts;
opts.include_dependence_headers = true;
opts.proto_mode = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(
Expand All @@ -173,7 +173,7 @@ void proto_test_include_id(const std::string& proto_path,
opts.include_dependence_headers = true;
opts.proto_mode = true;
opts.keep_proto_id = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(
Expand All @@ -191,7 +191,7 @@ void proto_test_include_union(const std::string& proto_path,
opts.include_dependence_headers = true;
opts.proto_mode = true;
opts.proto_oneof_union = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(flatbuffers::LoadFile(
Expand All @@ -210,7 +210,7 @@ void proto_test_include_union_id(const std::string& proto_path,
opts.proto_mode = true;
opts.proto_oneof_union = true;
opts.keep_proto_id = true;
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::NO_OP;
opts.proto_id_gap_action = IDLOptions::ProtoAction::NO_OP;

std::string golden_file;
TEST_EQ(flatbuffers::LoadFile(
Expand Down Expand Up @@ -269,7 +269,7 @@ void ParseCorruptedProto(const std::string& proto_path) {

// Parse proto with error on gap.
{
opts.proto_id_gap_action = IDLOptions::ProtoIdGapAction::ERROR;
opts.proto_id_gap_action = IDLOptions::ProtoAction::ERROR;
flatbuffers::Parser parser(opts);
TEST_EQ(flatbuffers::LoadFile((proto_path + "test.proto").c_str(), false,
&proto_file),
Expand Down