[flang][semantic] rewrite for explicit-shape-bounds-spec

[ivanrodriguez3753]

This PR is part 1 of 3 for a stack PR implementing rank-1 integer arrays with constant extent being used as explicit shape bounds. There is also an associated llvm-test-suite PR.

Part 1: #188447
Part 2: ivanrodriguez3753#1
Part 3: ivanrodriguez3753#2
llvm-test-suite: llvm/llvm-test-suite#411

Implement parse-tree rewrite in Sema. Emit TODO messages for semantic analysis, while still exiting gracefully and printing other error messages when applicable. future_ERROR messages in LIT test indicate expected error messages once semantic analysis is implemented in an upcoming PR. The feature in question is in section 8.5.8.2 of the 2023 working draft, revision J3/25-007r1, R814/R818/R819.

[ivanrodriguez3753]

@vzakhari @tarunprabhu

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-flang-parser

@llvm/pr-subscribers-flang-semantics

Author: ivanrodriguez3753

Changes

Implement parse-tree rewrite in Sema. Emit TODO messages for semantic analysis, while still exiting gracefully and printing other error messages when applicable. future_ERROR messages in LIT test indicate expected error messages once semantic analysis is implemented in an upcoming PR. The feature in question is in section 8.5.8.2 of the 2023 working draft, revision J3/25-007r1, R814/R818/R819.

---

Full diff: https://github.com/llvm/llvm-project/pull/188447.diff

5 Files Affected:

• (modified) flang/include/flang/Parser/dump-parse-tree.h (+1)
• (modified) flang/include/flang/Parser/parse-tree.h (+15-4)
• (modified) flang/lib/Parser/unparse.cpp (+1)
• (modified) flang/lib/Semantics/resolve-names-utils.cpp (+74-2)
• (added) flang/test/Semantics/declaration_explicit_array_bounds.f90 (+85)

diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 84c7b8d2a5349..9f4a826fe3f6a 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -346,6 +346,7 @@ class ParseTreeDumper {
   NODE(parser, ExitStmt)
   NODE(parser, ExplicitCoshapeSpec)
   NODE(parser, ExplicitShapeSpec)
+  NODE(parser, ExplicitShapeBoundsSpec)
   NODE(parser, Expr)
   NODE(Expr, Parentheses)
   NODE(Expr, UnaryPlus)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index a0106cac84620..23e965366a299 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -1324,13 +1324,24 @@ WRAPPER_CLASS(ImpliedShapeSpec, std::list<AssumedImpliedSpec>);
 EMPTY_CLASS(AssumedRankSpec);
 
 // R815 array-spec ->
-//        explicit-shape-spec-list | assumed-shape-spec-list |
-//        deferred-shape-spec-list | assumed-size-spec | implied-shape-spec |
+//        explicit-shape-spec-list | explicit-shape-bounds-spec |
+//        assumed-shape-spec-list | deferred-shape-spec-list |
+//        assumed-size-spec | implied-shape-spec |
 //        implied-shape-or-assumed-size-spec | assumed-rank-spec
+
+using ExplicitBoundsExpr = IntExpr;
+
+struct ExplicitShapeBoundsSpec {
+  TUPLE_CLASS_BOILERPLATE(ExplicitShapeBoundsSpec);
+  std::tuple<std::optional<ExplicitBoundsExpr>, ExplicitBoundsExpr>
+  t;
+};
+
 struct ArraySpec {
   UNION_CLASS_BOILERPLATE(ArraySpec);
-  std::variant<std::list<ExplicitShapeSpec>, std::list<AssumedShapeSpec>,
-      DeferredShapeSpecList, AssumedSizeSpec, ImpliedShapeSpec, AssumedRankSpec>
+  std::variant<std::list<ExplicitShapeSpec>, ExplicitShapeBoundsSpec, 
+      std::list<AssumedShapeSpec>, DeferredShapeSpecList, 
+      AssumedSizeSpec, ImpliedShapeSpec, AssumedRankSpec>
       u;
 };
 
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index c31eac0b3ff68..91b5bf8adf37c 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -571,6 +571,7 @@ class UnparseVisitor {
     common::visit(
         common::visitors{
             [&](const std::list<ExplicitShapeSpec> &y) { Walk(y, ","); },
+            [&](const ExplicitShapeBoundsSpec &y) { /*TODO: unreachable until after semantic analysis is done */ },
             [&](const std::list<AssumedShapeSpec> &y) { Walk(y, ","); },
             [&](const DeferredShapeSpecList &y) { Walk(y); },
             [&](const AssumedSizeSpec &y) { Walk(y); },
diff --git a/flang/lib/Semantics/resolve-names-utils.cpp b/flang/lib/Semantics/resolve-names-utils.cpp
index ef34c89182f7f..ad454620d9973 100644
--- a/flang/lib/Semantics/resolve-names-utils.cpp
+++ b/flang/lib/Semantics/resolve-names-utils.cpp
@@ -203,6 +203,7 @@ class ArraySpecAnalyzer {
   }
   void Analyze(const parser::AssumedShapeSpec &);
   void Analyze(const parser::ExplicitShapeSpec &);
+  void Analyze(const parser::ExplicitShapeBoundsSpec &);
   void Analyze(const parser::AssumedImpliedSpec &);
   void Analyze(const parser::DeferredShapeSpecList &);
   void Analyze(const parser::AssumedRankSpec &);
@@ -237,15 +238,79 @@ ArraySpec ArraySpecAnalyzer::Analyze(const parser::ComponentArraySpec &x) {
   CHECK(!arraySpec_.empty());
   return arraySpec_;
 }
+
+static bool shouldRewriteShapeSpecListToExplicitBounds(
+    SemanticsContext &context, const parser::ArraySpec &x) {
+  auto &explicitShapeSpecList{std::get<std::list<parser::ExplicitShapeSpec>>(
+      const_cast<parser::ArraySpec&>(x).u)};
+  
+  if (explicitShapeSpecList.size() != 1) {
+    return false;
+  }
+
+  auto &explicitShapeSpec{explicitShapeSpecList.front()};
+  const auto &upperBound{std::get<1>(explicitShapeSpec.t)};
+  const auto &lowerBoundOpt{std::get<0>(explicitShapeSpec.t)};
+  
+  bool foundArray{false};
+  
+  if (MaybeExpr analyzedExpr = AnalyzeExpr(context, upperBound.v.thing.thing.value());
+      analyzedExpr && (analyzedExpr->Rank() > 0)) {
+    foundArray = true;
+  }
+  
+  if (lowerBoundOpt) {
+    const auto &lowerBound{*lowerBoundOpt};
+    if (MaybeExpr analyzedExpr = AnalyzeExpr(context, lowerBound.v.thing.thing.value());
+        analyzedExpr && (analyzedExpr->Rank() > 0)) {
+      foundArray = true;
+    }
+  }
+  
+  return foundArray;
+}
+
+static void rewriteShapeSpecListToExplicitBounds(const parser::ArraySpec &x) {
+  auto &explicitShapeSpecList{std::get<std::list<parser::ExplicitShapeSpec>>(
+      const_cast<parser::ArraySpec&>(x).u)};
+  auto &mutableArraySpec{const_cast<parser::ArraySpec&>(x)};
+  auto &mutableExplicitShapeSpec{explicitShapeSpecList.front()};
+  
+  auto &mutableUpperBound{std::get<1>(mutableExplicitShapeSpec.t)};
+  parser::IntExpr upperIntExpr{std::move(mutableUpperBound.v.thing)};
+
+  auto &lowerBoundOpt{std::get<0>(mutableExplicitShapeSpec.t)};
+  std::optional<parser::IntExpr> lowerIntExpr;
+  if (lowerBoundOpt) {
+    auto &mutableLowerBound{std::get<0>(mutableExplicitShapeSpec.t)};
+    if(mutableLowerBound) {
+      lowerIntExpr = std::move(mutableLowerBound->v.thing);
+    }
+  }
+  
+  parser::ExplicitShapeBoundsSpec boundsSpec{
+    std::make_tuple(std::move(lowerIntExpr), std::move(upperIntExpr))};
+  mutableArraySpec.u = std::move(boundsSpec);
+
+  return;
+}
+
 ArraySpec ArraySpecAnalyzer::Analyze(const parser::ArraySpec &x) {
+  if(std::get_if<std::list<parser::ExplicitShapeSpec>>(&x.u) && 
+     shouldRewriteShapeSpecListToExplicitBounds(context_, x)) {
+    rewriteShapeSpecListToExplicitBounds(x);
+  }
   common::visit(common::visitors{
                     [&](const parser::AssumedSizeSpec &y) {
                       Analyze(
                           std::get<std::list<parser::ExplicitShapeSpec>>(y.t));
                       Analyze(std::get<parser::AssumedImpliedSpec>(y.t));
                     },
-                    [&](const parser::ImpliedShapeSpec &y) { Analyze(y.v); },
-                    [&](const auto &y) { Analyze(y); },
+                    [&](const parser::ImpliedShapeSpec &y) { 
+                      Analyze(y.v); },
+                    [&](const auto &y) { 
+                      Analyze(y); 
+                    },
                 },
       x.u);
   CHECK(!arraySpec_.empty());
@@ -279,6 +344,13 @@ void ArraySpecAnalyzer::Analyze(const parser::ExplicitShapeSpec &x) {
   MakeExplicit(std::get<std::optional<parser::SpecificationExpr>>(x.t),
       std::get<parser::SpecificationExpr>(x.t));
 }
+
+void ArraySpecAnalyzer::Analyze(const parser::ExplicitShapeBoundsSpec &x) {
+  context_.Say("TODO: Analyze overload for ExplicitShapeBoundsSpec"_err_en_US);
+  // prevent CHECK abort in Analyze(ArraySpec), otherwise it'll abort before printing error message
+  arraySpec_.push_back(ShapeSpec::MakeExplicit(Bound{1}));
+}
+
 void ArraySpecAnalyzer::Analyze(const parser::AssumedImpliedSpec &x) {
   MakeImplied(x.v);
 }
diff --git a/flang/test/Semantics/declaration_explicit_array_bounds.f90 b/flang/test/Semantics/declaration_explicit_array_bounds.f90
new file mode 100644
index 0000000000000..385b20028d895
--- /dev/null
+++ b/flang/test/Semantics/declaration_explicit_array_bounds.f90
@@ -0,0 +1,85 @@
+! RUN: %python %S/test_errors.py %s %flang_fc1 -Wautomatic-in-main-program -Wsaved-local-in-spec-expr
+! An explicit-shape-spec or explicit-shape-bounds-spec whose bounds are not constant expressions
+! shall appear only in a subprogram, derived type definition, BLOCK construct, or interface body.
+module data 
+  integer :: rank1_array_module(3) = [5, 5, 5]
+  !future_ERROR: Automatic data object 'gg2' may not appear in a module
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: gg2(rank1_array_module)
+  integer, allocatable :: nonconstsize(:)
+  !future_ERROR: Rank-1 integer array used as lower bounds in DECLARATION must have constant size
+  !future_ERROR: Rank-1 integer array used as upper bounds in DECLARATION must have constant size
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: gg3(nonconstsize : nonconstsize)
+end module 
+program declaration_array_bounds
+  implicit none
+
+  ! Valid cases (no errors expected)
+
+  ! Array upper bound only
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: c([3, 4, 5])
+
+  ! Array lower and upper bounds, same size
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: d((/2, 3/) : [10, 20])
+
+  ! Scalar lower, array upper
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: e(2 : [10, 20])
+
+  ! Array lower, scalar upper
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: f([2, 3] : 10)
+
+  ! Using non-literal PARAMETER variables
+  integer, parameter :: rank1_parameter_array(3) = [5,5,5]
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: g(rank1_parameter_array)
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: ggg(rank1_parameter_array * 2 : rank1_parameter_array - 1)
+
+
+  ! Negative cases (erros expected)
+  integer :: rank1_array(3) = [5,5,5]
+  ! Use existing error message for constness checking
+  !future_PORTABILITY: specification expression refers to local object 'rank1_array' (initialized and saved) [-Wsaved-local-in-spec-expr]
+  !future_PORTABILITY: Automatic data object 'gg' should not appear in the specification part of a main program [-Wautomatic-in-main-program]
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: gg(rank1_array)
+  integer :: scalar
+  !future_ERROR: Invalid specification expression: reference to local entity 'scalar'
+  !future_PORTABILITY: Automatic data object 'gggg' should not appear in the specification part of a main program [-Wautomatic-in-main-program]
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: gggg(rank1_parameter_array : scalar)
+
+  !ERROR: Must have INTEGER type, but is REAL(4)
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: h([1.2,2.2,3.2]:[1,2,3])
+  !future_ERROR: DECLARATION bounds integer rank-1 arrays must have the same size; lower bounds has 3 elements, upper bounds has 2 elements
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: i([1,2,3]:[3,3])
+
+  ! Test error for rank > 1, fulfilling constness
+  integer, parameter :: rank2_parameter_array(2,2) = reshape([[1,2],[3,4]], [2,2])
+  !future_ERROR: Integer array used as upper bounds in DECLARATION must be rank-1 but is rank-2
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: j(rank2_parameter_array)
+  ! Test combined bounds error, first bound as before but second bound as wrong rank
+  ! and nonconst
+  integer :: rank3_array(2,2,2)
+  !future_ERROR: Integer array used as lower bounds in DECLARATION must be rank-1 but is rank-2
+  !future_ERROR: Integer array used as upper bounds in DECLARATION must be rank-1 but is rank-3
+  !ERROR: TODO: Analyze overload for ExplicitShapeBoundsSpec
+  integer :: k(rank2_parameter_array : rank3_array)
+
+  ! Test that any comma list is parsed as ExplicitShapeSpecList and not rewritten 
+  ! to ExplicitShapeBonudsSpec, giving error messages expecting same number of 
+  ! aruments as rank of test_array and scalar integers
+  !ERROR: Must be a scalar value, but is a rank-1 array
+  !ERROR: Must be a scalar value, but is a rank-1 array
+  !ERROR: Must be a scalar value, but is a rank-1 array
+  !ERROR: Must have INTEGER type, but is REAL(4)
+  integer :: test_array([1,2,3] : [2,3,4], 3, [1,2,3], 5.2)
+end program 
\ No newline at end of file

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[eugeneepshteyn]

Hmm, so how would it look unparsed?

[ivanrodriguez3753]

For this particular grammar rule, it is impossible to meaningfully test unparse before semantic analysis work is done. As per my own conclusions, affirmed by @klausler's comment in a draft PR I've since closed, "syntactic distinction that can't be determined at parse time." This is a case of purposely misparsing due to an ambiguity and fixing up the tree later by rewriting (we need type information to differentiate identifiers that are scalar integers versus rank-1 integer arrays). In other words, the following test case gives the exact same output with and without my change under -fdebug-dump-parse-tree-no-sema, since my change does not touch the "parser proper". I've tested with trunk and with this branch:

> cat unparse.f90
program main 
  implicit none 
  integer :: dims(2) = [3,4]
  integer :: arr1(dims)
  integer :: arr2([3,4])
end program 
> flang -fc1 -fdebug-dump-parse-tree-no-sema unparse.f90
======================== Flang: parse tree dump ========================
Program -> ProgramUnit -> MainProgram
| ProgramStmt -> Name = 'main'
| SpecificationPart
| | ImplicitPart -> ImplicitPartStmt -> ImplicitStmt -> 
| | DeclarationConstruct -> SpecificationConstruct -> TypeDeclarationStmt
| | | DeclarationTypeSpec -> IntrinsicTypeSpec -> IntegerTypeSpec -> 
| | | EntityDecl
| | | | Name = 'dims'
| | | | ArraySpec -> ExplicitShapeSpec
| | | | | SpecificationExpr -> Scalar -> Integer -> Expr -> LiteralConstant -> IntLiteralConstant = '2'
| | | | Initialization -> Constant -> Expr -> ArrayConstructor -> AcSpec
| | | | | AcValue -> Expr -> LiteralConstant -> IntLiteralConstant = '3'
| | | | | AcValue -> Expr -> LiteralConstant -> IntLiteralConstant = '4'
| | DeclarationConstruct -> SpecificationConstruct -> TypeDeclarationStmt
| | | DeclarationTypeSpec -> IntrinsicTypeSpec -> IntegerTypeSpec -> 
| | | EntityDecl
| | | | Name = 'arr1'
| | | | ArraySpec -> ExplicitShapeSpec
| | | | | SpecificationExpr -> Scalar -> Integer -> Expr -> Designator -> DataRef -> Name = 'dims'
| | DeclarationConstruct -> SpecificationConstruct -> TypeDeclarationStmt
| | | DeclarationTypeSpec -> IntrinsicTypeSpec -> IntegerTypeSpec -> 
| | | EntityDecl
| | | | Name = 'arr2'
| | | | ArraySpec -> ExplicitShapeSpec
| | | | | SpecificationExpr -> Scalar -> Integer -> Expr -> ArrayConstructor -> AcSpec
| | | | | | AcValue -> Expr -> LiteralConstant -> IntLiteralConstant = '3'
| | | | | | AcValue -> Expr -> LiteralConstant -> IntLiteralConstant = '4'
| ExecutionPart -> Block
| EndProgramStmt -> 

The output for -fdebug-dump-parse-tree is a graceful compiler abort with the expected TODO messages I put in, which is tested in the LIT test I included.

> flang -fc1 -fdebug-dump-parse-tree unparse.f90
error: Semantic errors in unparse.f90
unparse.f90:4:3: error: TODO: Analyze overload for ExplicitShapeBoundsSpec
    integer :: arr1(dims)
    ^^^^^^^^^^^^^^^^^^^^^
unparse.f90:5:3: error: TODO: Analyze overload for ExplicitShapeBoundsSpec
    integer :: arr2([3,4])
    ^^^^^^^^^^^^^^^^^^^^^^
> $trunk_flang -fc1 -fdebug-dump-parse-tree unparse.f90 
error: Semantic errors in unparse.f90
unparse.f90:4:19: error: Must be a scalar value, but is a rank-1 array
    integer :: arr1(dims)
                    ^^^^
unparse.f90:5:19: error: Must be a scalar value, but is a rank-1 array
    integer :: arr2([3,4])
                    ^^^^^

In my own developer branch that has this feature working end to end (compile to runnable executable), the output of -fdebug-dump-parse-tree correctly shows the new node type and tree structure. I did not include the unparse implementation in this PR because it is unreachable, as commented in the code.
Perhaps the name of the PR, "parser support", is misnamed. This PR and the other related ones are all technically changes to Semantics, but they are just tree rewrites. So, I purposely excluded unparse tests, they can be included with the PRs that implement the "actual" semantic analysis. I suppose I could manually dump the tree at the TODO in this PR, but that is unwieldy and will be useless once semantic analysis PRs come in. Furthermore, testing the parse tree structure, even with semantic analysis, seems kind of useless to me given that the error messages I'm anticipating (labeled future_ERROR in this PR's test flang/test/Semantics/declaration_explicit_array_bounds.f90) would never fire if the parse tree structure was wrong. But that discussion can be deferred until the followup PR that implements sema.

[eugeneepshteyn]

Could you please give some examples with explicit-shape-bounds-spec and how do you expect them to look unparsed once semantic analysis is done? Unparsing is hugely important once you start serializing things into module files, so I'm curious how it will look there and how it will be processed when module is loaded.

[ivanrodriguez3753]

All comments have been addressed. I will use this PR and its suggestions/corrections as a skeleton for #188445 and #188446 once this is merged. Thank you @eugeneepshteyn for the review!

[eugeneepshteyn]

I think this work should become a set of stacked PRs, so that they could be tested with the right order and eventually get merged together. I'd like to see more of the TODO sections implemented to get more context.

[eugeneepshteyn]

We try to avoid unwrapping complex structures like this: upperBound.v.thing.thing.value().

Please look into Unwrap<parser::Expr> or similar.

[ivanrodriguez3753]

Sure. I've changed the code to use parser::UnwrapRef<parser::Expr> in this function shouldRewriteShapeSpecListToExplicitBounds. I cannot avoid the use of .thing in rewriteShapeSpecListToExplicitBounds because the rewrite needs the correct level of indirection. That is, the new node kind ExplicitShapeBoundsSpec expects an IntExpr (or rather a tuple of them).

[eugeneepshteyn]

This is only used within ExplicitShapeBoundsSpec for now, but in the other PRs it'll be also used elsewhere, right?

[ivanrodriguez3753]

It's not used anywhere else, but the spec has a rule for it. I find it unnecessary and will remove it.

[eugeneepshteyn]

Is const_cast here necessary? shouldRewriteShapeSpecListToExplicitBounds() doesn't seem to modify anything, just read information.

[ivanrodriguez3753]

It's not, you're right. I must have left that in as an oversight since I originally had shouldRewriteShapeSpecListToExplicitBounds and rewriteShapeSpecListToExplicitBounds as a single function.

[eugeneepshteyn]

The other PRs have this implementation, right? Are you going to turn your PRs into stacked PRs?

[eugeneepshteyn]

Could you please give some examples with explicit-shape-bounds-spec and how do you expect them to look unparsed once semantic analysis is done? Unparsing is hugely important once you start serializing things into module files, so I'm curious how it will look there and how it will be processed when module is loaded.

[akuhlens]

This generally looks good to me, but if you already have a end to end development of this it would be good to just see the entire stack so that we know where you are going. I left comments where I see room for improvement otherwise.

[akuhlens]

This should be "..."_todo_en_US.

[akuhlens]

Oh! I would change the title to `[flang][semantic] rewrite for explicit-shape-bounds-spec"

[eugeneepshteyn]

Have you checked RewriteParseTree()? Wouldn't that mechanism be more appropriate for mutating the tree? (Sorry, if this was discussed before, it's hard to keep track of all the conversations, especially if they span months...)

[ivanrodriguez3753]

I spent the whole morning seeing whether or not this can be done cleanly. I think the answer is, no. In semantics.cpp, we see that name resolution happens before parse tree rewriting:

static bool PerformStatementSemantics(
    SemanticsContext &context, parser::Program &program) {
  printf("calling ResolveNames from semantics.cpp\n");
  ResolveNames(context, program, context.globalScope());
  printf("calling RewriteParseTree from semantics.cpp\n");
  RewriteParseTree(context, program);

If we comment out the block above and prevent the rewrite, we get semantic errors:

rodriiva@pe35hopper:/hpcdc/project/pe_users/rodriiva/sandbox/CPE-13671/pr_test> flang -fc1 main.f90
calling ResolveNames from semantics.cpp
hit type error for Must be a scalar value...
calling RewriteParseTree from semantics.cpp
error: Semantic errors in main.f90
main.f90:2:18: error: Must be a scalar value, but is a rank-1 array
    integer :: arr([1,2])
                   ^^^^^

(the "hit type error" was inserted in expression.h so we can see the ordering accurately since the real error is on a different output stream). We could loosen SpecificationExpr to IntExpr instead of ScalarIntExpr to avoid this, but I think a lot of existing code assumes SpecificationExpr is a wrapper of ScalarIntExpr.
In resolve-names-utils.cpp, handlers for bounds are called and arraySpec_ is populated. If we delay the rewrite to RewriteParseTree, we would have already called MakeExplicit passing in array valued expression. Later, when accessing with GetBound, I suspect it would return a null optional, and the Fortran::semantics::Bound class wouldn't work as expected.

I think the main hurdle is that ArraySpec and the number of bounds used to be determinable context free, by the parser, and now we need symbol information since we can't rely on the number of , (minus 1) in a declaration.

[eugeneepshteyn]

Please summarize this as a comment in the code, so that the future maintainer understands why this logic is here and why RewriteParseTree() could not be used.

[eugeneepshteyn]

The style guide doesn't like unnecessary "return" from void functions.

[eugeneepshteyn]

I don't think std::make_tuple is necessary here. TUPLE_CLASS_BOILERPLATE takes care of the forwarding.

[eugeneepshteyn]

Typo: "ExplicitShapeBonudsSpec" -> "...Bounds"

[eugeneepshteyn]

Typo "erros" -> "errors"

[eugeneepshteyn]

One typo and one request for comment, otherwise I think this is pretty much ready to go.

[eugeneepshteyn]

Please summarize this as a comment in the code, so that the future maintainer understands why this logic is here and why RewriteParseTree() could not be used.

[github-actions]

🐧 Linux x64 Test Results

• 4676 tests passed
• 206 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🪟 Windows x64 Test Results

• 4312 tests passed
• 250 tests skipped

✅ The build succeeded and all tests passed.

[eugeneepshteyn]

LGTM, pending the green CI.

@ivanrodriguez3753 , you should be able to run the same tests locally via ninja check-flang or similar. (I would hope that your changes should work the same on any OS, if they don't we may have a problem.)

[ivanrodriguez3753]

LGTM, pending the green CI.

@ivanrodriguez3753 , you should be able to run the same tests locally via ninja check-flang or similar. (I would hope that your changes should work the same on any OS, if they don't we may have a problem.)

The single test failure was my new test, the diff changed since I changed the suffix on the string literal used in the error message, and overlooked updating the test. Fixing that now and re-running on my machine.

[ivanrodriguez3753]

Please use this commit message when merging, as I don't have write access to the repo:

[flang][semantic] parser node types and rewrite for explicit-shape-bounds-spec

This commit lays the groundwork for semantic analysis of rank-1 integer array expressions being used as bounds in a declaration with explicit bounds.

[github-actions]

@ivanrodriguez3753 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!