Add a new facility GrowingRange for a range that might grow during iteration. (#5641)

Use it to replace most existing modernize-loop-convert lints with
range-based for loops. As requested in review of #5475.

---------

Co-authored-by: Jon Ross-Perkins <jperkins@google.com>

common/BUILD

@@ -236,6 +236,22 @@ cc_test(
     ],
 )
 
+cc_library(
+    name = "growing_range",
+    hdrs = ["growing_range.h"],
+)
+
+cc_test(
+    name = "growing_range_test",
+    size = "small",
+    srcs = ["growing_range_test.cpp"],
+    deps = [
+        ":growing_range",
+        "//testing/base:gtest_main",
+        "@googletest//:gtest",
+    ],
+)
+
 cc_library(
     name = "hashing",
     srcs = ["hashing.cpp"],

common/growing_range.h

@@ -0,0 +1,68 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef CARBON_COMMON_GROWING_RANGE_H_
+#define CARBON_COMMON_GROWING_RANGE_H_
+
+#include <ranges>
+
+namespace Carbon {
+
+// A range adaptor for a random-access container such as `std::vector` or
+// `llvm::SmallVector` that might have elements appended during the iteration.
+// This adaptor avoids invalidation issues by tracking an index instead of an
+// iterator, and by returning by value from `operator*` instead of by reference.
+//
+// This class is intended only for use as the range in a range-based for loop,
+// and as such does not provide a complete range or iterator interface. Instead,
+// it provides only the interface required by the range-based for loop.
+template <typename ContainerT>
+  requires std::ranges::sized_range<ContainerT> &&
+           std::ranges::random_access_range<ContainerT>
+class GrowingRange {
+ public:
+  // An end sentinel for the range.
+  class End {};
+
+  // An iterator into a potentially-growing range. Tracks the container and the
+  // current index, and indexes the container on each dereference.
+  class Iterator {
+   public:
+    // Dereferences the iterator. These intentionally don't return by reference,
+    // to avoid handing out a reference that would be invalidated when the
+    // container grows during the traversal.
+    auto operator*() -> auto { return (*container_)[index_]; }
+    auto operator*() const -> auto { return (*container_)[index_]; }
+
+    friend auto operator!=(Iterator it, End /*end*/) -> bool {
+      return it.index_ != it.container_->size();
+    }
+
+    auto operator++() -> void { ++index_; }
+
+   private:
+    friend class GrowingRange;
+
+    explicit Iterator(const ContainerT* container)
+        : container_(container), index_(0) {}
+
+    const ContainerT* container_;
+    size_t index_;
+  };
+
+  explicit GrowingRange(const ContainerT& container) : container_(&container) {}
+
+  auto begin() const -> Iterator { return Iterator(container_); }
+  auto end() const -> End { return {}; }
+
+ private:
+  const ContainerT* container_;
+};
+
+template <typename ContainerT>
+GrowingRange(const ContainerT&) -> GrowingRange<ContainerT>;
+
+}  // namespace Carbon
+
+#endif  // CARBON_COMMON_GROWING_RANGE_H_

common/growing_range_test.cpp

@@ -0,0 +1,49 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "common/growing_range.h"
+
+#include <gtest/gtest.h>
+
+#include <vector>
+
+namespace Carbon {
+namespace {
+
+TEST(GrowingRangeTest, TestUnchanged) {
+  std::vector<int> v = {1, 2, 3, 4, 5};
+  int k = 0;
+  for (int n : GrowingRange(v)) {
+    EXPECT_EQ(n, ++k);
+  }
+}
+
+TEST(GrowingRangeTest, TestGrowWithRealloc) {
+  std::vector<int> expected = {3, 2, 2, 2, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0};
+
+  std::vector<int> v;
+  v.reserve(1);
+  v.push_back(3);
+  EXPECT_LT(v.capacity(), expected.size());
+
+  int i = 0;
+  for (int n : GrowingRange(v)) {
+    // Append n copies of n - 1.
+    v.insert(v.end(), n, n - 1);
+    EXPECT_EQ(n, expected[i++]);
+  }
+}
+
+TEST(GrowingRangeTest, TestNoReference) {
+  std::vector<int> v;
+  // Use `decltype(auto)` to capture the type of the element including whether
+  // it's a reference.
+  for (decltype(auto) elem : GrowingRange(v)) {
+    // The type of `elem` should be `int`, not `int&`.
+    static_assert(std::same_as<decltype(elem), int>);
+  }
+}
+
+}  // namespace
+}  // namespace Carbon

toolchain/check/BUILD

@@ -113,6 +113,7 @@ cc_library(
         "//common:concepts",
         "//common:emplace_by_calling",
         "//common:find",
+        "//common:growing_range",
         "//common:map",
         "//common:ostream",
         "//common:raw_string_ostream",
@@ -181,6 +182,7 @@ cc_library(
         "//common:check",
         "//common:error",
         "//common:find",
+        "//common:growing_range",
         "//common:map",
         "//common:ostream",
         "//common:vlog",

toolchain/check/check_unit.cpp

@@ -9,6 +9,7 @@
 #include <tuple>
 #include <utility>
 
+#include "common/growing_range.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/VirtualFileSystem.h"
@@ -454,11 +455,7 @@ auto CheckUnit::CheckRequiredDeclarations() -> void {
 auto CheckUnit::CheckRequiredDefinitions() -> void {
   CARBON_DIAGNOSTIC(MissingDefinitionInImpl, Error,
                     "no definition found for declaration in impl file");
-
-  // Note that more required definitions can be added during this loop.
-  // NOLINTNEXTLINE(modernize-loop-convert)
-  for (size_t i = 0; i != context_.definitions_required_by_decl().size(); ++i) {
-    SemIR::InstId decl_inst_id = context_.definitions_required_by_decl()[i];
+  for (SemIR::InstId decl_inst_id : context_.definitions_required_by_decl()) {
     SemIR::Inst decl_inst = context_.insts().Get(decl_inst_id);
     CARBON_KIND_SWITCH(context_.insts().Get(decl_inst_id)) {
       case CARBON_KIND(SemIR::ClassDecl class_decl): {
@@ -497,12 +494,10 @@ auto CheckUnit::CheckRequiredDefinitions() -> void {
     }
   }
 
-  // Note that more required definitions can be added during this loop.
-  // NOLINTNEXTLINE(modernize-loop-convert)
-  for (size_t i = 0; i != context_.definitions_required_by_use().size(); ++i) {
+  for (auto [loc, specific_id] :
+       GrowingRange(context_.definitions_required_by_use())) {
     // This is using the location for the use. We could track the
     // list of enclosing locations if this was used from a generic.
-    auto [loc, specific_id] = context_.definitions_required_by_use()[i];
     if (!ResolveSpecificDefinition(context_, loc, specific_id)) {
       CARBON_DIAGNOSTIC(MissingGenericFunctionDefinition, Error,
                         "use of undefined generic function");

toolchain/check/import_ref.cpp

@@ -10,6 +10,7 @@
 #include <utility>
 
 #include "common/check.h"
+#include "common/growing_range.h"
 #include "toolchain/base/kind_switch.h"
 #include "toolchain/check/context.h"
 #include "toolchain/check/eval.h"
@@ -3278,9 +3279,8 @@ auto ImportRefResolver::PerformPendingWork() -> void {
     // state.
     // TODO: Import the generic eval block rather than calling
     // RebuildGenericEvalBlock to rebuild it so that order doesn't matter.
-    // NOLINTNEXTLINE(modernize-loop-convert)
-    for (size_t i = 0; i != pending_generics().size(); ++i) {
-      FinishPendingGeneric(*this, pending_generics()[i]);
+    for (auto generic_id : GrowingRange(pending_generics())) {
+      FinishPendingGeneric(*this, generic_id);
     }
     pending_generics().clear();
 

toolchain/lower/BUILD

@@ -47,6 +47,7 @@ cc_library(
     ],
     deps = [
         "//common:check",
+        "//common:growing_range",
         "//common:map",
         "//common:raw_string_ostream",
         "//common:vlog",

toolchain/lower/context.cpp

@@ -5,6 +5,7 @@
 #include "toolchain/lower/context.h"
 
 #include "common/check.h"
+#include "common/growing_range.h"
 #include "common/vlog.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 #include "toolchain/lower/file_context.h"
@@ -43,12 +44,8 @@ auto Context::GetFileContext(const SemIR::File* file,
 
 auto Context::LowerPendingDefinitions() -> void {
   // Lower function definitions for generics.
-  // This cannot be a range-based loop, as new definitions can be added
-  // while building other definitions.
-  // NOLINTNEXTLINE(modernize-loop-convert)
-  for (size_t i = 0; i != specific_function_definitions_.size(); ++i) {
-    auto [file_context, function_id, specific_id] =
-        specific_function_definitions_[i];
+  for (auto [file_context, function_id, specific_id] :
+       GrowingRange(specific_function_definitions_)) {
     file_context->BuildFunctionDefinition(function_id, specific_id);
   }
 }