From 946deb4b388b89b3942f2fae57475689b6d4d670 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Mon, 26 Oct 2026 18:56:08 -0700 Subject: [PATCH] Add GetPositionFromByteOffset to cel::Source PiperOrigin-RevId: 1007724732 --- common/BUILD | 1 + common/source.cc | 88 ++++++++++++++++++++--- common/source.h | 18 ++++- common/source_test.cc | 110 ++++++++++++++++++++++++++++- policy/BUILD | 1 - policy/cel_policy_parse_context.cc | 16 ----- policy/cel_policy_parse_context.h | 8 --- policy/yaml_policy_parser.cc | 39 +++------- 8 files changed, 214 insertions(+), 67 deletions(-) diff --git a/common/BUILD b/common/BUILD index 2a645bb9a..e9439cf53 100644 --- a/common/BUILD +++ b/common/BUILD @@ -611,6 +611,7 @@ cc_test( "//internal:testing", "@com_google_absl//absl/status", "@com_google_absl//absl/strings:cord", + "@com_google_absl//absl/strings:string_view", ], ) diff --git a/common/source.cc b/common/source.cc index e7e09d919..0d770b228 100644 --- a/common/source.cc +++ b/common/source.cc @@ -163,10 +163,17 @@ namespace common_internal { class SourceImpl : public Source { public: + struct OffsetEntry { + int32_t byte_offset; + SourcePosition codepoint; + }; + SourceImpl(std::string description, - absl::InlinedVector line_offsets) + absl::InlinedVector line_offsets, + std::vector byte_offsets = {}) : description_(std::move(description)), - line_offsets_(std::move(line_offsets)) {} + line_offsets_(std::move(line_offsets)), + byte_offsets_(std::move(byte_offsets)) {} absl::string_view description() const final { return description_; } @@ -174,9 +181,50 @@ class SourceImpl : public Source { return absl::MakeConstSpan(line_offsets_); } + absl::optional GetPositionFromByteOffset( + SourcePosition byte_offset) const final { + // handle out of bounds and all ascii. + if (byte_offset < 0) { + return absl::nullopt; + } + if (byte_offsets_.empty()) { + if (byte_offset > content().size()) { + return absl::nullopt; + } + return byte_offset; + } + if (byte_offset > byte_offsets_.back().byte_offset) { + return absl::nullopt; + } + + // binary search for the next alignment shift after the given byte offset. + auto it = std::upper_bound( + byte_offsets_.begin(), byte_offsets_.end(), byte_offset, + [](SourcePosition val, const OffsetEntry& entry) { + return val < entry.byte_offset; + }); + if (it == byte_offsets_.begin()) { + return byte_offset; + } + const OffsetEntry& prev = *(it - 1); + if (it == byte_offsets_.end()) { + // matched the EOF entry. + return prev.codepoint; + } + + // the total shift at the next multi-byte character. + int32_t next_delta = it->byte_offset - it->codepoint; + + // The offset is either in the previous multi-byte character or in the + // run of ascii characters after it. if negative, then in, otherwise + // shifted down. + return std::max(prev.codepoint, byte_offset - next_delta); + } + private: const std::string description_; const absl::InlinedVector line_offsets_; + const std::vector byte_offsets_; }; namespace { @@ -201,8 +249,9 @@ class Latin1Source final : public SourceImpl { public: Latin1Source(std::string description, absl::InlinedVector line_offsets, - std::vector text) - : SourceImpl(std::move(description), std::move(line_offsets)), + std::vector byte_offsets, std::vector text) + : SourceImpl(std::move(description), std::move(line_offsets), + std::move(byte_offsets)), text_(std::move(text)) {} ContentView content() const override { @@ -217,8 +266,10 @@ class BasicPlaneSource final : public SourceImpl { public: BasicPlaneSource(std::string description, absl::InlinedVector line_offsets, + std::vector byte_offsets, std::vector text) - : SourceImpl(std::move(description), std::move(line_offsets)), + : SourceImpl(std::move(description), std::move(line_offsets), + std::move(byte_offsets)), text_(std::move(text)) {} ContentView content() const override { @@ -233,8 +284,10 @@ class SupplementalPlaneSource final : public SourceImpl { public: SupplementalPlaneSource(std::string description, absl::InlinedVector line_offsets, + std::vector byte_offsets, std::vector text) - : SourceImpl(std::move(description), std::move(line_offsets)), + : SourceImpl(std::move(description), std::move(line_offsets), + std::move(byte_offsets)), text_(std::move(text)) {} ContentView content() const override { @@ -329,6 +382,7 @@ absl::StatusOr NewSourceImpl(std::string description, const T& text, std::vector data16; std::vector data32; absl::InlinedVector line_offsets; + std::vector byte_offsets; while (index < text_size) { if (offset >= max_codepoints) { return absl::InvalidArgumentError(absl::StrCat( @@ -350,6 +404,7 @@ absl::StatusOr NewSourceImpl(std::string description, const T& text, ++offset; continue; } + byte_offsets.push_back({static_cast(index), offset}); if (code_point <= 0xff) { data8.reserve(text_size); Traits::AppendTo(data8, text, index); @@ -399,6 +454,9 @@ absl::StatusOr NewSourceImpl(std::string description, const T& text, if (code_point == '\n') { line_offsets.push_back(offset + 1); } + if (code_units > 1) { + byte_offsets.push_back({static_cast(index), offset}); + } if (code_point <= 0xff) { data8.push_back(static_cast(code_point)); Traits::Advance(it, code_units); @@ -430,8 +488,10 @@ absl::StatusOr NewSourceImpl(std::string description, const T& text, goto supplemental; } line_offsets.push_back(offset + 1); + byte_offsets.push_back({static_cast(text_size), offset}); return std::make_unique( - std::move(description), std::move(line_offsets), std::move(data8)); + std::move(description), std::move(line_offsets), std::move(byte_offsets), + std::move(data8)); basic: while (index < text_size) { if (offset >= max_codepoints) { @@ -448,6 +508,9 @@ absl::StatusOr NewSourceImpl(std::string description, const T& text, if (code_point == '\n') { line_offsets.push_back(offset + 1); } + if (code_units > 1) { + byte_offsets.push_back({static_cast(index), offset}); + } if (code_point <= 0xffff) { data16.push_back(static_cast(code_point)); Traits::Advance(it, code_units); @@ -467,8 +530,10 @@ absl::StatusOr NewSourceImpl(std::string description, const T& text, goto supplemental; } line_offsets.push_back(offset + 1); + byte_offsets.push_back({static_cast(text_size), offset}); return std::make_unique( - std::move(description), std::move(line_offsets), std::move(data16)); + std::move(description), std::move(line_offsets), std::move(byte_offsets), + std::move(data16)); supplemental: while (index < text_size) { if (offset >= max_codepoints) { @@ -485,14 +550,19 @@ absl::StatusOr NewSourceImpl(std::string description, const T& text, if (code_point == '\n') { line_offsets.push_back(offset + 1); } + if (code_units > 1) { + byte_offsets.push_back({static_cast(index), offset}); + } data32.push_back(code_point); Traits::Advance(it, code_units); index += code_units; ++offset; } line_offsets.push_back(offset + 1); + byte_offsets.push_back({static_cast(text_size), offset}); return std::make_unique( - std::move(description), std::move(line_offsets), std::move(data32)); + std::move(description), std::move(line_offsets), std::move(byte_offsets), + std::move(data32)); } } // namespace diff --git a/common/source.h b/common/source.h index 190c750cc..5f517e0fa 100644 --- a/common/source.h +++ b/common/source.h @@ -144,6 +144,20 @@ class Source { absl::optional GetPosition( const SourceLocation& location) const; + // Maps a 0-based byte offset in the original UTF-8 source text to a + // `SourcePosition` (0-based code point offset). Returns an empty + // `absl::optional` when `byte_offset` is invalid or the information required + // to perform the mapping is not present. + // + // If `byte_offset` falls within a multi-byte UTF-8 sequence, returns the + // code point position of that sequence. + // + // The offset for the size in bytes is mapped to the size in code points. + virtual absl::optional GetPositionFromByteOffset( + SourcePosition byte_offset) const { + return absl::nullopt; + } + absl::optional Snippet(int32_t line) const; // Formats an annotated snippet highlighting an error at location, e.g. @@ -193,9 +207,11 @@ class Source { // `SourceSubrange` is a view of a subrange fo an underlying `Source` object. // Intended to be used when the CEL expression is embedded in a larger text -// representation (such as a.celpolicy). +// representation (such as a celpolicy). // // The parent `Source` must outlive this object. +// +// Note: `GetPositionFromByteOffset` is not supported by `SourceSubrange`. class SourceSubrange final : public Source { public: SourceSubrange(const Source& source ABSL_ATTRIBUTE_LIFETIME_BOUND, diff --git a/common/source_test.cc b/common/source_test.cc index 8315fd517..167d88481 100644 --- a/common/source_test.cc +++ b/common/source_test.cc @@ -19,6 +19,7 @@ #include "absl/status/status.h" #include "absl/strings/cord.h" +#include "absl/strings/string_view.h" #include "internal/testing.h" namespace cel { @@ -306,10 +307,117 @@ TEST(CordSource, CodepointLimitExceeded) { absl::StatusCode::kInvalidArgument, ::testing::HasSubstr("expression is larger than codepoint limit 5"))); - ASSERT_OK_AND_ASSIGN(auto source, + ASSERT_OK_AND_ASSIGN(SourcePtr source, NewSource(absl::Cord("12345"), "test", options)); EXPECT_THAT(source->content().ToString(), ::testing::Eq("12345")); } +TEST(Source, GetPositionFromByteOffsetAscii) { + ASSERT_OK_AND_ASSIGN(SourcePtr empty_source, NewSource("")); + EXPECT_THAT(empty_source->GetPositionFromByteOffset(-1), Eq(std::nullopt)); + EXPECT_THAT(empty_source->GetPositionFromByteOffset(0), Optional(Eq(0))); + EXPECT_THAT(empty_source->GetPositionFromByteOffset(1), Eq(std::nullopt)); + + ASSERT_OK_AND_ASSIGN(SourcePtr source, NewSource("hello")); + EXPECT_THAT(source->GetPositionFromByteOffset(-1), Eq(std::nullopt)); + for (SourcePosition i = 0; i <= 5; ++i) { + EXPECT_THAT(source->GetPositionFromByteOffset(i), Optional(Eq(i))); + } + EXPECT_THAT(source->GetPositionFromByteOffset(6), Eq(std::nullopt)); +} + +TEST(Source, GetPositionFromByteOffsetLatin1) { + // "a£b": 'a' (1 byte: 0), '£' (2 bytes: 1..2), 'b' (1 byte: 3). Total 4 + // bytes, 3 codepoints. + ASSERT_OK_AND_ASSIGN(SourcePtr source, NewSource("a£b")); + EXPECT_THAT(source->GetPositionFromByteOffset(-1), Eq(std::nullopt)); + EXPECT_THAT(source->GetPositionFromByteOffset(0), Optional(Eq(0))); + EXPECT_THAT(source->GetPositionFromByteOffset(1), Optional(Eq(1))); + EXPECT_THAT(source->GetPositionFromByteOffset(2), Optional(Eq(1))); + EXPECT_THAT(source->GetPositionFromByteOffset(3), Optional(Eq(2))); + EXPECT_THAT(source->GetPositionFromByteOffset(4), Optional(Eq(3))); + EXPECT_THAT(source->GetPositionFromByteOffset(5), Eq(std::nullopt)); +} + +TEST(Source, GetPositionFromByteOffsetBasicPlane) { + // "a€b": 'a' (1 byte: 0), '€' (3 bytes: 1..3), 'b' (1 byte: 4). Total 5 + // bytes, 3 codepoints. + ASSERT_OK_AND_ASSIGN(SourcePtr source, NewSource("a€b")); + EXPECT_THAT(source->GetPositionFromByteOffset(-1), Eq(std::nullopt)); + EXPECT_THAT(source->GetPositionFromByteOffset(0), Optional(Eq(0))); + EXPECT_THAT(source->GetPositionFromByteOffset(1), Optional(Eq(1))); + EXPECT_THAT(source->GetPositionFromByteOffset(2), Optional(Eq(1))); + EXPECT_THAT(source->GetPositionFromByteOffset(3), Optional(Eq(1))); + EXPECT_THAT(source->GetPositionFromByteOffset(4), Optional(Eq(2))); + EXPECT_THAT(source->GetPositionFromByteOffset(5), Optional(Eq(3))); + EXPECT_THAT(source->GetPositionFromByteOffset(6), Eq(std::nullopt)); +} + +TEST(Source, GetPositionFromByteOffsetSupplementalPlaneAndMixed) { + // "a£€𐍈b": + // cp 0: 'a' (byte 0, 1 byte) + // cp 1: '£' (bytes 1..2, 2 bytes) + // cp 2: '€' (bytes 3..5, 3 bytes) + // cp 3: '𐍈' (bytes 6..9, 4 bytes) + // cp 4: 'b' (byte 10, 1 byte) + // Total: 11 bytes, 5 codepoints. + constexpr absl::string_view kText = "a£€𐍈b"; + ASSERT_OK_AND_ASSIGN(SourcePtr source, NewSource(kText)); + ASSERT_OK_AND_ASSIGN(SourcePtr cord_source, NewSource(absl::Cord(kText))); + + for (const Source* s : {source.get(), cord_source.get()}) { + EXPECT_THAT(s->GetPositionFromByteOffset(-1), Eq(std::nullopt)); + EXPECT_THAT(s->GetPositionFromByteOffset(0), Optional(Eq(0))); + EXPECT_THAT(s->GetPositionFromByteOffset(1), Optional(Eq(1))); + EXPECT_THAT(s->GetPositionFromByteOffset(2), Optional(Eq(1))); + EXPECT_THAT(s->GetPositionFromByteOffset(3), Optional(Eq(2))); + EXPECT_THAT(s->GetPositionFromByteOffset(4), Optional(Eq(2))); + EXPECT_THAT(s->GetPositionFromByteOffset(5), Optional(Eq(2))); + EXPECT_THAT(s->GetPositionFromByteOffset(6), Optional(Eq(3))); + EXPECT_THAT(s->GetPositionFromByteOffset(7), Optional(Eq(3))); + EXPECT_THAT(s->GetPositionFromByteOffset(8), Optional(Eq(3))); + EXPECT_THAT(s->GetPositionFromByteOffset(9), Optional(Eq(3))); + EXPECT_THAT(s->GetPositionFromByteOffset(10), Optional(Eq(4))); + EXPECT_THAT(s->GetPositionFromByteOffset(11), Optional(Eq(5))); + EXPECT_THAT(s->GetPositionFromByteOffset(12), Eq(std::nullopt)); + } +} + +TEST(Source, GetPositionFromByteOffsetStartsAndEndsWithMultibyte) { + // "£a€b𐍈": + // cp 0: '£' (bytes 0..1, 2 bytes) + // cp 1: 'a' (byte 2, 1 byte) + // cp 2: '€' (bytes 3..5, 3 bytes) + // cp 3: 'b' (byte 6, 1 byte) + // cp 4: '𐍈' (bytes 7..10, 4 bytes) + // Total: 11 bytes, 5 codepoints. + constexpr absl::string_view kText = "£a€b𐍈"; + ASSERT_OK_AND_ASSIGN(SourcePtr source, NewSource(kText)); + ASSERT_OK_AND_ASSIGN(SourcePtr cord_source, NewSource(absl::Cord(kText))); + + for (const Source* s : {source.get(), cord_source.get()}) { + EXPECT_THAT(s->GetPositionFromByteOffset(-1), Eq(std::nullopt)); + EXPECT_THAT(s->GetPositionFromByteOffset(0), Optional(Eq(0))); + EXPECT_THAT(s->GetPositionFromByteOffset(1), Optional(Eq(0))); + EXPECT_THAT(s->GetPositionFromByteOffset(2), Optional(Eq(1))); + EXPECT_THAT(s->GetPositionFromByteOffset(3), Optional(Eq(2))); + EXPECT_THAT(s->GetPositionFromByteOffset(4), Optional(Eq(2))); + EXPECT_THAT(s->GetPositionFromByteOffset(5), Optional(Eq(2))); + EXPECT_THAT(s->GetPositionFromByteOffset(6), Optional(Eq(3))); + EXPECT_THAT(s->GetPositionFromByteOffset(7), Optional(Eq(4))); + EXPECT_THAT(s->GetPositionFromByteOffset(8), Optional(Eq(4))); + EXPECT_THAT(s->GetPositionFromByteOffset(9), Optional(Eq(4))); + EXPECT_THAT(s->GetPositionFromByteOffset(10), Optional(Eq(4))); + EXPECT_THAT(s->GetPositionFromByteOffset(11), Optional(Eq(5))); + EXPECT_THAT(s->GetPositionFromByteOffset(12), Eq(std::nullopt)); + } +} + +TEST(SourceSubrange, GetPositionFromByteOffsetReturnsNullopt) { + ASSERT_OK_AND_ASSIGN(SourcePtr source, NewSource("hello world")); + SourceSubrange subrange(*source, SourceRange{0, 5}); + EXPECT_THAT(subrange.GetPositionFromByteOffset(0), Eq(std::nullopt)); +} + } // namespace } // namespace cel diff --git a/policy/BUILD b/policy/BUILD index 9b7fe4202..070b55f44 100644 --- a/policy/BUILD +++ b/policy/BUILD @@ -84,7 +84,6 @@ cc_library( ":cel_policy_parser", "//common:source", "//internal:status_macros", - "//internal:utf8", "//policy/internal:yaml_string_element_scanner", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", diff --git a/policy/cel_policy_parse_context.cc b/policy/cel_policy_parse_context.cc index a627bfd50..0c12c7f0c 100644 --- a/policy/cel_policy_parse_context.cc +++ b/policy/cel_policy_parse_context.cc @@ -14,13 +14,11 @@ #include "policy/cel_policy_parse_context.h" -#include #include #include #include #include "absl/log/absl_check.h" -#include "common/source.h" #include "policy/cel_policy.h" #include "policy/cel_policy_parse_result.h" @@ -47,18 +45,4 @@ void CelPolicyParseContext::ReportError(CelPolicyElementId element_id, issues_.push_back(CelPolicyIssue(element_id, message)); } -SourcePosition CelPolicyParseContext::GetCodepointPosition( - SourcePosition byte_offset) const { - if (byte_offset < 0) { - return -1; - } - if (byte_to_codepoint_mapping_.empty()) { - return byte_offset; - } - if (static_cast(byte_offset) >= byte_to_codepoint_mapping_.size()) { - return byte_to_codepoint_mapping_.back(); - } - return byte_to_codepoint_mapping_[byte_offset]; -} - } // namespace cel diff --git a/policy/cel_policy_parse_context.h b/policy/cel_policy_parse_context.h index 7e9aa3fe3..6482fa1ae 100644 --- a/policy/cel_policy_parse_context.h +++ b/policy/cel_policy_parse_context.h @@ -20,7 +20,6 @@ #include #include "absl/strings/string_view.h" -#include "common/source.h" #include "policy/cel_policy.h" #include "policy/cel_policy_parse_result.h" @@ -54,18 +53,11 @@ class CelPolicyParseContext { CelPolicyElementId next_element_id() { return next_element_id_++; } - void set_byte_to_codepoint_mapping(std::vector mapping) { - byte_to_codepoint_mapping_ = std::move(mapping); - } - - SourcePosition GetCodepointPosition(SourcePosition byte_offset) const; - private: std::shared_ptr policy_source_; CelPolicyElementId next_element_id_ = 0; std::vector issues_; std::unique_ptr policy_; - std::vector byte_to_codepoint_mapping_; }; } // namespace cel diff --git a/policy/yaml_policy_parser.cc b/policy/yaml_policy_parser.cc index d40cbd8d6..1af71196e 100644 --- a/policy/yaml_policy_parser.cc +++ b/policy/yaml_policy_parser.cc @@ -14,12 +14,10 @@ #include "policy/yaml_policy_parser.h" -#include #include #include #include #include -#include #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -27,7 +25,6 @@ #include "absl/strings/string_view.h" #include "common/source.h" #include "internal/status_macros.h" -#include "internal/utf8.h" #include "policy/cel_policy.h" #include "policy/cel_policy_parse_context.h" #include "policy/cel_policy_parse_result.h" @@ -45,8 +42,13 @@ namespace { SourcePosition GetMarkCodepointPosition(const CelPolicyParseContext& ctx, const YAML::Mark& mark) { - if (mark.is_null() || mark.pos < 0) return -1; - return ctx.GetCodepointPosition(mark.pos); + if (mark.is_null() || mark.pos < 0 || + ctx.policy_source().content() == nullptr) { + return -1; + } + const Source& source = *ctx.policy_source().content(); + return source.GetPositionFromByteOffset(mark.pos).value_or( + source.content().size()); } } // namespace @@ -102,35 +104,10 @@ absl::Status YamlPolicyParser::ParsePolicy(CelPolicyParseContext& ctx) const { return absl::OkStatus(); } - // TODO(b/542282964): Fold this mapping into cel::Source decoding. - std::string text = source->content().ToString(); - std::vector mapping; - mapping.resize(text.size() + 1, 0); - size_t byte_offset = 0; - SourcePosition codepoint = 0; - absl::string_view view = text; - while (!view.empty()) { - auto [code_point, code_units] = cel::internal::Utf8Decode(view); - if (code_units == 0) break; - for (size_t i = 0; i < code_units; ++i) { - if (byte_offset + i < mapping.size()) { - mapping[byte_offset + i] = codepoint; - } - } - byte_offset += code_units; - view.remove_prefix(code_units); - codepoint++; - } - for (size_t i = byte_offset; i < mapping.size(); ++i) { - mapping[i] = codepoint; - } - - ctx.set_byte_to_codepoint_mapping(std::move(mapping)); - ctx.policy().set_description(ValueString(-1, source->description())); YAML::Node node; try { - node = YAML::Load(text); + node = YAML::Load(source->content().ToString()); } catch (YAML::Exception& e) { if (!e.mark.is_null()) { ctx.policy_source().NoteSourcePosition(