[flang-commits] [flang] [flang][semantic] Implement semantic checks and new data structure for explicit-shape-bounds-spec (PR #203030)

[Eugene Epshteyn via flang-commits]

================
@@ -396,11 +408,165 @@ void ArraySpecAnalyzer::Analyze(const parser::ExplicitShapeSpec &x) {
       std::get<parser::SpecificationExpr>(x.t));
 }
 
+std::optional<ArraySpecAnalyzer::ExplicitShapeBoundsResult>
+ArraySpecAnalyzer::CheckExplicitShapeBoundsSpec(
+    const parser::ExplicitShapeBoundsSpec &x) {
+  const auto &lowerBoundOpt{std::get<0>(x.t)};
+  const auto &upperBound{std::get<1>(x.t)};
+
+  // Analyze, validate, fold, and wrap one bound expression in a Bound.
+  // Returns the Bound paired with the extent of the bound: std::nullopt
+  // for a scalar bound (which broadcasts to every dimension) or, for a
+  // rank-1 array bound, its constant extent (which may be zero).
+
+  // hasError should not be set to true unless there was an error
+  // diagnostic emitted beforehand.
+  bool hasError{false};
+  auto analyzeBound = [&](const auto &parseBound, bool isUpper)
+      -> std::optional<std::pair<Bound, std::optional<std::int64_t>>> {
+    MaybeExpr expr{AnalyzeExpr(context_, parseBound)};
+    // Analyzing the parser::Integer<> wrapper enforces the INTEGER type
+    // constraint (C885) and emits a diagnostic for a non-INTEGER bound,
+    // returning std::nullopt.
+    if (!expr) {
+      hasError = true;
+      return std::nullopt;
+    }
+    if (expr->Rank() > 1) {
+      context_.Say(parser::FindSourceLocation(parseBound),
+          "Integer array used as %s bounds in DECLARATION must be rank-1 "
+          "but is rank-%d"_err_en_US,
+          isUpper ? "upper" : "lower", expr->Rank());
+      hasError = true;
+      return std::nullopt;
+    }
+    auto folded{evaluate::Fold(context_.foldingContext(), std::move(*expr))};
+    // The parser::Integer<> constraint enforced above guarantees an INTEGER
+    // type, so unwrapping the folded result as an integer expression must
+    // succeed.
+    const auto *someInt{evaluate::UnwrapExpr<SomeIntExpr>(folded)};
+    CHECK(someInt);
+    auto asSI{evaluate::Fold(context_.foldingContext(),
+        evaluate::ConvertToType<evaluate::SubscriptInteger>(
+            common::Clone(*someInt)))};
+    if (folded.Rank() == 0) {
+      // Scalar bound: broadcasts to every dimension.
+      return std::make_pair(Bound{MaybeSubscriptIntExpr{std::move(asSI)}},
+          std::optional<std::int64_t>{});
+    }
+    // Rank-1: must have constant extent.
+    auto extents{
+        evaluate::GetConstantExtents(context_.foldingContext(), folded)};
+    if (!extents) {
+      context_.Say(parser::FindSourceLocation(parseBound),
+          "Rank-1 integer array used as %s bounds in DECLARATION must "
+          "have constant size"_err_en_US,
+          isUpper ? "upper" : "lower");
+      hasError = true;
+      return std::nullopt;
+    }
+    return std::make_pair(Bound{MaybeSubscriptIntExpr{std::move(asSI)}},
+        std::optional<std::int64_t>{(*extents)[0]});
+  };
+
+  // Upper bound (required)
+  auto ubResult{analyzeBound(upperBound, /*isUpper=*/true)};
+
+  // Lower bound (optional)
+  std::optional<std::pair<Bound, std::optional<std::int64_t>>> lbResult;
+  if (lowerBoundOpt) {
+    lbResult = analyzeBound(*lowerBoundOpt, /*isUpper=*/false);
+  }
+
+  if (hasError) {
+    return std::nullopt;
+  }
+
+  // A bound is rank-1 (array-valued) iff it has a concrete extent; a scalar
+  // bound (or an omitted lower bound) has none and merely broadcasts.
+  std::optional<std::int64_t> ubExtent{ubResult->second};
+  std::optional<std::int64_t> lbExtent;
+  if (lbResult) {
+    lbExtent = lbResult->second;
+  }
+  bool ubIsRank1{ubExtent.has_value()};
+  bool lbIsRank1{lbExtent.has_value()};
+
+  // C832: when both bounds are rank-1 arrays, they must have the same size.
+  // This must include the case where one of the sizes is zero.
+  if (ubIsRank1 && lbIsRank1 && *ubExtent != *lbExtent) {
+    context_.Say(parser::FindSourceLocation(x),
+        "DECLARATION bounds integer rank-1 arrays must have the same size; "
+        "lower bounds has %jd elements, upper bounds has %jd elements"_err_en_US,
+        *lbExtent, *ubExtent);
+    return std::nullopt;
+  }
+
+  // The rank of the entity is the size of whichever bound is the rank-1
+  // array (they are equal when both are).  A zero-size array yields rank 0,
+  // i.e. the entity is scalar.
+  std::int64_t numDims{ubIsRank1 ? *ubExtent : (lbIsRank1 ? *lbExtent : 0)};
+
+  std::optional<Bound> lb;
+  if (lbResult) {
+    lb.emplace(std::move(lbResult->first));
+  }
+  return ExplicitShapeBoundsResult{
+      std::move(ubResult->first), std::move(lb), numDims};
+}
+
 void ArraySpecAnalyzer::Analyze(const parser::ExplicitShapeBoundsSpec &x) {
-  context_.Say("TODO: Analyze overload for ExplicitShapeBoundsSpec"_todo_en_US);
-  // prevent CHECK abort in Analyze(ArraySpec), otherwise it'll abort before
-  // printing error message
-  arraySpec_.push_back(ShapeSpec::MakeExplicit(Bound{1}));
+  auto result{CheckExplicitShapeBoundsSpec(x)};
+  // Every path that results in result being false emits an error. In the event
+  // that we bail early without emitting an error, we silently pass the fallback
+  // Bound{1} WITHOUT failing. This check ensures that if we failed, we emitted
+  // an error message. This way we can pass the
+  //   CHECK(!arraySpec_.empty());
+  // in Analyze(ArraySpec). If we don't, it'll crash before getting to emit
+  // the real (user) error messages.
+  if (!result) {
+    CHECK(context_.AnyFatalError());
+    arraySpec_.push_back(ShapeSpec::MakeExplicit(Bound{1}));
+    return;
+  }
+  // For rank-1 bounds, emit N ShapeSpecs each wrapping a scalar
+  // RankOneBoundElement that extracts element [dim] from the rank-1
+  // expression.  This makes all downstream consumers see scalar bounds.
+  int numDims = static_cast<int>(result->numDims);
+  if (numDims == 0) {
----------------
eugeneepshteyn wrote:

I'm concerned that the user wouldn't get any notification that the impure function call is thrown away, especially if that call was supposed to have side-effects. I think there should be an error here like in the cases you mentioned above. 

I don't think this should block this PR, though. Perhaps we can file an issue that could be fixed later.

https://github.com/llvm/llvm-project/pull/203030