diff --git a/src/google/protobuf/io/printer.cc b/src/google/protobuf/io/printer.cc index 580290837c..d46c009c7e 100644 --- a/src/google/protobuf/io/printer.cc +++ b/src/google/protobuf/io/printer.cc @@ -261,9 +261,10 @@ Printer::Printer(ZeroCopyOutputStream* output, Options options) } absl::string_view Printer::LookupVar(absl::string_view var) { - LookupResult result = LookupInFrameStack(var, absl::MakeSpan(var_lookups_)); + auto result = LookupInFrameStack(var, absl::MakeSpan(var_lookups_)); GOOGLE_ABSL_CHECK(result.has_value()) << "could not find " << var; - auto* view = absl::get_if(&*result); + + auto* view = result->AsString(); GOOGLE_ABSL_CHECK(view != nullptr) << "could not find " << var << "; found callback instead"; @@ -299,16 +300,13 @@ void Printer::Outdent() { indent_ -= options_.spaces_per_indent; } -void Printer::Emit( - std::initializer_list< - VarDefinition> - vars, - absl::string_view format, SourceLocation loc) { +void Printer::Emit(std::initializer_list vars, absl::string_view format, + SourceLocation loc) { PrintOptions opts; opts.strip_raw_string_indentation = true; opts.loc = loc; - auto defs = WithDefs(vars); + auto defs = WithDefs(vars, /*allow_callbacks=*/true); PrintImpl(format, {}, opts); } @@ -323,9 +321,10 @@ absl::optional> Printer::GetSubstitutionRange( } std::pair range = it->second; - if (!Validate(range.first <= range.second, opts, [varname] { - return absl::StrCat( - "variable used for annotation used multiple times: ", varname); + if (!Validate(range.first <= range.second, opts, [range, varname] { + return absl::StrFormat( + "variable used for annotation used multiple times: %s (%d..%d)", + varname, range.first, range.second); })) { return absl::nullopt; } @@ -614,7 +613,7 @@ void Printer::PrintImpl(absl::string_view format, continue; } - LookupResult sub; + absl::optional sub; absl::optional same_name_record; if (opts.allow_digit_substitutions && absl::ascii_isdigit(var[0])) { if (!Validate(var.size() == 1u, opts, @@ -652,7 +651,7 @@ void Printer::PrintImpl(absl::string_view format, size_t range_start = sink_.bytes_written(); size_t range_end = sink_.bytes_written(); - if (auto* str = absl::get_if(&*sub)) { + if (const absl::string_view* str = sub->AsString()) { if (at_start_of_line_ && str->empty()) { line_start_variables_.emplace_back(var); } @@ -666,7 +665,7 @@ void Printer::PrintImpl(absl::string_view format, PrintRaw(suffix); } } else { - auto* fnc = absl::get_if>(&*sub); + const ValueView::Callback* fnc = sub->AsCallback(); GOOGLE_ABSL_CHECK(fnc != nullptr); Validate( @@ -676,34 +675,42 @@ void Printer::PrintImpl(absl::string_view format, range_start = sink_.bytes_written(); (*fnc)(); range_end = sink_.bytes_written(); + } - // If we just evaluated a closure, and we are at the start of a line, - // that means it finished with a newline. If a newline follows - // immediately after, we drop it. This helps callback formatting "work - // as expected" with respect to forms like - // - // class Foo { - // $methods$; - // }; - // - // Without this line, this would turn into something like - // - // class Foo { - // void Bar() {} - // - // }; - // - // in many cases. We *also* do this if a ; or , follows the - // substitution, because this helps clang-format keep its head on in - // many cases. Users that need to keep the semi can write $foo$/**/; - ++chunk_idx; - if (chunk_idx < line.chunks.size()) { - absl::string_view text = line.chunks[chunk_idx].text; - if (!absl::ConsumePrefix(&text, ";")) { - absl::ConsumePrefix(&text, ","); + // If we just evaluated a value which specifies end-of-line consume-after + // characters, and we're at the start of a line, that means we finished + // with a newline. + // + // We trim a single end-of-line `consume_after` character in this case. + // + // This helps callback formatting "work as expected" with respect to forms + // like + // + // class Foo { + // $methods$; + // }; + // + // Without this post-processing, it would turn into + // + // class Foo { + // void Bar() {}; + // }; + // + // in many cases. Without the `;`, clang-format may format the template + // incorrectly. + auto next_idx = chunk_idx + 1; + if (!sub->consume_after.empty() && next_idx < line.chunks.size() && + !line.chunks[next_idx].is_var) { + chunk_idx = next_idx; + + absl::string_view text = line.chunks[chunk_idx].text; + for (char c : sub->consume_after) { + if (absl::ConsumePrefix(&text, absl::string_view(&c, 1))) { + break; } - PrintRaw(text); } + + PrintRaw(text); } if (same_name_record.has_value() && diff --git a/src/google/protobuf/io/printer.h b/src/google/protobuf/io/printer.h index 4896154362..45698c27da 100644 --- a/src/google/protobuf/io/printer.h +++ b/src/google/protobuf/io/printer.h @@ -172,6 +172,26 @@ class AnnotationProtoCollector : public AnnotationCollector { // will crash. Callers must statically know that every variable reference is // valid, and MUST NOT pass user-provided strings directly into Emit(). // +// Substitutions can be configured to "chomp" a single character after them, to +// help make indentation work out. This can be configured by passing a +// two-argument io::Printer::Value into Emit's substitution map: +// +// p.Emit({{"var", io::Printer::Value{var_decl, ";"}}}, R"cc( +// class $class$ { +// public: +// $var$; +// }; +// )cc"); +// +// This will delete the ; after $var$, regardless of whether it was an empty +// declaration or not. It will also intelligently attempt to clean up +// empty lines that follow, if it was on an empty line; this promotes cleaner +// formatting of the output. +// +// Any number of different characters can be potentially skipped, but only one +// will actually be skipped. For example, callback substitutions (see below) use +// ";," by default as their "chomping set". +// // # Callback Substitution // // Instead of passing a string into Emit(), it is possible to pass in a callback @@ -436,31 +456,71 @@ class PROTOBUF_EXPORT Printer { } }; - // Sink type for constructing values to pass to WithVars() and Emit(). - template - struct VarDefinition { - using StringOrCallback = absl::variant>; + // Helper type for wrapping a variable substitution expansion result. + template + struct ValueImpl { + private: + template + struct IsSubImpl : std::false_type {}; + template + struct IsSubImpl> : std::true_type {}; + + public: + using StringType = + std::conditional_t; + // These callbacks return false if this is a recursive call. + using Callback = std::function; + using StringOrCallback = absl::variant; + + ValueImpl() = default; + + // This is a template to avoid colliding with the copy constructor below. + template >::value, + int> = 0> + ValueImpl(Value&& value) // NOLINT + : value(ToStringOrCallback(std::forward(value), Rank2{})) { + if (absl::holds_alternative(this->value)) { + consume_after = ";,"; + } + } - template - VarDefinition(Key&& key, Value&& value) - : key(std::forward(key)), - value(ToStringOrCallback(std::forward(value), Rank2{})), - annotation(absl::nullopt) {} + // Copy ctor/assign allow interconversion of the two template parameters. + template + ValueImpl(const ValueImpl& that) { // NOLINT + *this = that; + } - // NOTE: This is an overload rather than taking optional - // with a default argument of nullopt, because we want to pick up - // AnnotationRecord's user-defined conversions. Because going from - // e.g. Descriptor* -> optional requires two user-defined - // conversions, this does not work. - template - VarDefinition(Key&& key, Value&& value, AnnotationRecord annotation) - : key(std::forward(key)), - value(ToStringOrCallback(std::forward(value), Rank2{})), - annotation(std::move(annotation)) {} + template + ValueImpl& operator=(const ValueImpl& that) { + // Cast to void* is required, since this and that may potentially be of + // different types (due to the `that_owned` parameter). + if (static_cast(this) == static_cast(&that)) { + return *this; + } + + using ThatStringType = typename ValueImpl::StringType; + + if (auto* str = absl::get_if(&that.value)) { + value = StringType(*str); + } else { + value = absl::get(that.value); + } + + consume_after = that.consume_after; + return *this; + } + + const StringType* AsString() const { + return absl::get_if(&value); + } + + const Callback* AsCallback() const { + return absl::get_if(&value); + } - K key; StringOrCallback value; - absl::optional annotation; + std::string consume_after; private: // go/ranked-overloads @@ -474,22 +534,33 @@ class PROTOBUF_EXPORT Printer { // // This is done to produce a better error message than the "candidate does // not match" SFINAE errors. - template - StringOrCallback ToStringOrCallback(std::function cb, Rank2) { - static_assert( - allowed, "callback-typed variables are not allowed in this location"); - return cb; + template ()())> + StringOrCallback ToStringOrCallback(Cb&& cb, Rank2) { + return Callback( + [cb = std::forward(cb), is_called = false]() mutable -> bool { + if (is_called) { + // Catch whether or not this function is being called recursively. + return false; + } + is_called = true; + cb(); + is_called = false; + return true; + }); } // Separate from the AlphaNum overload to avoid copies when taking strings - // by value. - StringOrCallback ToStringOrCallback(std::string s, Rank1) { return s; } + // by value when in `owned` mode. + StringOrCallback ToStringOrCallback(StringType s, Rank1) { return s; } StringOrCallback ToStringOrCallback(const absl::AlphaNum& s, Rank0) { - return std::string(s.Piece()); + return StringType(s.Piece()); } }; + using ValueView = ValueImpl; + using Value = ValueImpl; + // Provide a helper to use heterogeneous lookup when it's available. template using void_t = void; @@ -516,6 +587,44 @@ class PROTOBUF_EXPORT Printer { static constexpr absl::string_view kProtocCodegenTrace = "PROTOC_CODEGEN_TRACE"; + // Sink type for constructing substitutions to pass to WithVars() and Emit(). + class Sub { + public: + template + Sub(std::string key, Value&& value) + : key_(std::move(key)), + value_(std::forward(value)), + annotation_(absl::nullopt) {} + + // NOTE: This is an overload rather than taking optional + // with a default argument of nullopt, because we want to pick up + // AnnotationRecord's user-defined conversions. Because going from + // e.g. Descriptor* -> optional requires two user-defined + // conversions, this does not work. + template + Sub(Key&& key, Value&& value, AnnotationRecord annotation) + : key_(std::forward(key)), + value_(std::forward(value)), + annotation_(std::move(annotation)) {} + + Sub Annotate(AnnotationRecord annotation) && { + annotation_ = std::move(annotation); + return std::move(*this); + } + + Sub WithSuffix(std::string sub_suffix) && { + value_.consume_after = std::move(sub_suffix); + return std::move(*this); + } + + private: + friend class Printer; + + std::string key_; + Value value_; + absl::optional annotation_; + }; + // Options for controlling how the output of a Printer is formatted. struct Options { Options() = default; @@ -572,13 +681,14 @@ class PROTOBUF_EXPORT Printer { // Returns an RAII object that pops the lookup frame. template auto WithVars(const Map* vars) { - var_lookups_.emplace_back([vars](absl::string_view var) -> LookupResult { - auto it = vars->find(ToStringKey(var)); - if (it == vars->end()) { - return absl::nullopt; - } - return absl::string_view(it->second); - }); + var_lookups_.emplace_back( + [vars](absl::string_view var) -> absl::optional { + auto it = vars->find(ToStringKey(var)); + if (it == vars->end()) { + return absl::nullopt; + } + return ValueView(it->second); + }); return absl::MakeCleanup([this] { var_lookups_.pop_back(); }); } @@ -591,20 +701,19 @@ class PROTOBUF_EXPORT Printer { template , std::enable_if_t::value, int> = 0> auto WithVars(Map&& vars) { - var_lookups_.emplace_back([vars = std::forward(vars)]( - absl::string_view var) -> LookupResult { - auto it = vars.find(ToStringKey(var)); - if (it == vars.end()) { - return absl::nullopt; - } - return absl::string_view(it->second); - }); + var_lookups_.emplace_back( + [vars = std::forward(vars)]( + absl::string_view var) -> absl::optional { + auto it = vars.find(ToStringKey(var)); + if (it == vars.end()) { + return absl::nullopt; + } + return ValueView(it->second); + }); return absl::MakeCleanup([this] { var_lookups_.pop_back(); }); } - auto WithVars(std::initializer_list< - VarDefinition> - vars); + auto WithVars(std::initializer_list vars); // Looks up a variable set with WithVars(). // @@ -673,10 +782,7 @@ class PROTOBUF_EXPORT Printer { // documentation for more details. // // `format` MUST be a string constant. - void Emit(std::initializer_list< - VarDefinition> - vars, - absl::string_view format, + void Emit(std::initializer_list vars, absl::string_view format, SourceLocation loc = SourceLocation::current()); // Write a string directly to the underlying output, performing no formatting @@ -881,10 +987,8 @@ class PROTOBUF_EXPORT Printer { // Prints a codegen trace, for the given location in the compiler's source. void PrintCodegenTrace(absl::optional loc); - // The core implementation for "fully-elaborated" variable definitions. This - // is a private function to avoid users being able to set `allow_callbacks`. - template - auto WithDefs(std::initializer_list> vars); + // The core implementation for "fully-elaborated" variable definitions. + auto WithDefs(std::initializer_list vars, bool allow_callbacks); // Returns the start and end of the value that was substituted in place of // the variable `varname` in the last call to PrintImpl() (with @@ -899,10 +1003,8 @@ class PROTOBUF_EXPORT Printer { bool at_start_of_line_ = true; bool failed_ = false; - using LookupResult = - absl::optional>>; - - std::vector> var_lookups_; + std::vector(absl::string_view)>> + var_lookups_; std::vector< std::function(absl::string_view)>> @@ -918,39 +1020,33 @@ class PROTOBUF_EXPORT Printer { std::vector line_start_variables_; }; -template -auto Printer::WithDefs( - std::initializer_list> vars) { - absl::flat_hash_map>> - var_map; +inline auto Printer::WithDefs(std::initializer_list vars, + bool allow_callbacks) { + absl::flat_hash_map var_map; var_map.reserve(vars.size()); - absl::flat_hash_map annotation_map; + absl::flat_hash_map annotation_map; - for (auto& var : vars) { - auto result = var_map.insert({var.key, var.value}); + for (const auto& var : vars) { + GOOGLE_ABSL_CHECK(allow_callbacks || var.value_.AsCallback() == nullptr) + << "callback arguments are not permitted in this position"; + auto result = var_map.insert({var.key_, var.value_}); GOOGLE_ABSL_CHECK(result.second) - << "repeated variable in Emit() or WithVars() call: \"" << var.key + << "repeated variable in Emit() or WithVars() call: \"" << var.key_ << "\""; - if (var.annotation.has_value()) { - annotation_map.insert({var.key, *var.annotation}); + if (var.annotation_.has_value()) { + annotation_map.insert({var.key_, *var.annotation_}); } } - var_lookups_.emplace_back( - [map = std::move(var_map)](absl::string_view var) -> LookupResult { - auto it = map.find(var); - if (it == map.end()) { - return absl::nullopt; - } - if (auto* str = absl::get_if(&it->second)) { - return absl::string_view(*str); - } - - auto* f = absl::get_if>(&it->second); - GOOGLE_ABSL_CHECK(f != nullptr); - return *f; - }); + var_lookups_.emplace_back([map = std::move(var_map)](absl::string_view var) + -> absl::optional { + auto it = map.find(var); + if (it == map.end()) { + return absl::nullopt; + } + return ValueView(it->second); + }); bool has_annotations = !annotation_map.empty(); if (has_annotations) { @@ -973,10 +1069,8 @@ auto Printer::WithDefs( }); } -inline auto Printer::WithVars( - std::initializer_list> - vars) { - return WithDefs(vars); +inline auto Printer::WithVars(std::initializer_list vars) { + return WithDefs(vars, /*allow_callbacks=*/false); } } // namespace io } // namespace protobuf diff --git a/src/google/protobuf/io/printer_unittest.cc b/src/google/protobuf/io/printer_unittest.cc index ebf31eb117..754426dded 100644 --- a/src/google/protobuf/io/printer_unittest.cc +++ b/src/google/protobuf/io/printer_unittest.cc @@ -598,6 +598,27 @@ TEST_F(PrinterTest, EmitWithVars) { "};\n"); } +TEST_F(PrinterTest, EmitConsumeAfter) { + { + Printer printer(output()); + printer.Emit( + { + {"class", "Foo"}, + Printer::Sub{"var", "int x;"}.WithSuffix(";"), + }, + R"cc( + class $class$ { + $var$; + }; + )cc"); + } + + EXPECT_EQ(written(), + "class Foo {\n" + " int x;\n" + "};\n"); +} + TEST_F(PrinterTest, EmitWithSpacedVars) { { Printer printer(output());