[flang] Clean up TODO comments and fix one (DATA constant ambiguity)

[klausler]

Summary

Remove resolved & moot TODO comments in Common/, Parser/,
and Evaluate/. Address a pending one relating to parsing
ambiguity in DATA statement constants, handling it with
symbol table information in Semantics and adding a test.

[PeteSteinfeld]

All builds, tests, and looks good.

flang/include/flang/Evaluate/real.h

public:
   constexpr Real(const Word &bits) : word_{bits} {}
   constexpr Real &operator=(const Real &) = default;
   constexpr Real &operator=(Real &&) = default;
 
   constexpr bool operator==(const Real &that) const {
     return word_ == that.word_;
   }
 
-  // TODO: DIM, MAX, MIN, DPROD, FRACTION,
-  // INT/NINT, NEAREST, OUT_OF_RANGE,
-  // RRSPACING/SPACING, SCALE, SET_EXPONENT
-
   constexpr bool IsSignBitSet() const { return word_.BTEST(bits - 1); }
   constexpr bool IsNegative() const {
     return !IsNotANumber() && IsSignBitSet();
   }
   constexpr bool IsNotANumber() const {
     return Exponent() == maxExponent && !GetSignificand().IsZero();
   }
   constexpr bool IsQuietNaN() const {
ValueWithRealFlags<Real> Subtract(
     return Add(y.Negate(), rounding);
   }
   ValueWithRealFlags<Real> Multiply(
       const Real &, Rounding rounding = defaultRounding) const;
   ValueWithRealFlags<Real> Divide(
       const Real &, Rounding rounding = defaultRounding) const;
 
   // SQRT(x**2 + y**2) but computed so as to avoid spurious overflow
-  // TODO: needed for CABS
+  // TODO: not yet implemented; needed for CABS
   ValueWithRealFlags<Real> HYPOT(
       const Real &, Rounding rounding = defaultRounding) const;
 
   template <typename INT> constexpr INT EXPONENT() const {
     if (Exponent() == maxExponent) {
       return INT::HUGE();
     } else {
       return {UnbiasedExponent()};

flang/include/flang/Parser/dump-parse-tree.h

#include "llvm/Frontend/OpenACC/ACC.cpp.inc"
   NODE(parser, ComputedGotoStmt)
   NODE(parser, ConcurrentControl)
   NODE(parser, ConcurrentHeader)
   NODE(parser, ConnectSpec)
   NODE(ConnectSpec, CharExpr)
   NODE_ENUM(ConnectSpec::CharExpr, Kind)
   NODE(ConnectSpec, Newunit)
   NODE(ConnectSpec, Recl)
-  NODE(parser, ConstantValue)
   NODE(parser, ContainsStmt)
   NODE(parser, Contiguous)
   NODE(parser, ContiguousStmt)
   NODE(parser, ContinueStmt)
   NODE(parser, CriticalConstruct)
   NODE(parser, CriticalStmt)
   NODE(parser, CycleStmt)
   NODE(parser, DataComponentDefStmt)

flang/include/flang/Parser/parse-state.h

 #include <utility>
 
 namespace Fortran::parser {
 
 using common::LanguageFeature;
 
 class ParseState {
 public:
-  // TODO: Add a constructor for parsing a normalized module file.
   ParseState(const CookedSource &cooked)
       : p_{cooked.AsCharBlock().begin()}, limit_{cooked.AsCharBlock().end()} {}
   ParseState(const ParseState &that)
       : p_{that.p_}, limit_{that.limit_}, context_{that.context_},
         userState_{that.userState_}, inFixedForm_{that.inFixedForm_},
         anyErrorRecovery_{that.anyErrorRecovery_},
         anyConformanceViolation_{that.anyConformanceViolation_},
         deferMessages_{that.deferMessages_},

flang/include/flang/Parser/parse-tree.h

std::variant<SpecificationConstruct, Statement<common::Indirection<DataStmt>>,
       Statement<common::Indirection<FormatStmt>>,
       Statement<common::Indirection<EntryStmt>>,
       Statement<common::Indirection<StmtFunctionStmt>>, ErrorRecovery>
       u;
 };
 
 // R504 specification-part -> [use-stmt]... [import-stmt]... [implicit-part]
 //                            [declaration-construct]...
-// TODO: transfer any statements after the last IMPLICIT (if any)
-// from the implicit part to the declaration constructs
+// PARAMETER, FORMAT, and ENTRY statements that appear before any other
+// kind of declaration-construct will be parsed into the implicit-part,
+// even if there are no IMPLICIT statements.
 struct SpecificationPart {
   TUPLE_CLASS_BOILERPLATE(SpecificationPart);
   std::tuple<std::list<OpenACCDeclarativeConstruct>,
       std::list<OpenMPDeclarativeConstruct>,
       std::list<common::Indirection<CompilerDirective>>,
       std::list<Statement<common::Indirection<UseStmt>>>,
       std::list<Statement<common::Indirection<ImportStmt>>>, ImplicitPart,
       std::list<DeclarationConstruct>>

 struct LiteralConstant {
   UNION_CLASS_BOILERPLATE(LiteralConstant);
   std::variant<HollerithLiteralConstant, IntLiteralConstant,
       RealLiteralConstant, ComplexLiteralConstant, BOZLiteralConstant,
       CharLiteralConstant, LogicalLiteralConstant>
       u;
 };
 
-// R604 constant ->  literal-constant | named-constant
-// Renamed to dodge a clash with Constant<> template class.
-struct ConstantValue {
-  UNION_CLASS_BOILERPLATE(ConstantValue);
-  std::variant<LiteralConstant, NamedConstant> u;
-};
-
 // R807 access-spec -> PUBLIC | PRIVATE
 struct AccessSpec {
   ENUM_CLASS(Kind, Public, Private)
   WRAPPER_CLASS_BOILERPLATE(AccessSpec, Kind);
 };
 
 // R728 type-attr-spec ->
 //        ABSTRACT | access-spec | BIND(C) | EXTENDS ( parent-type-name )

 // Represents an analyzed expression
 using TypedExpr = common::ForwardOwningPointer<evaluate::GenericExprWrapper>;
 
 // R845 data-stmt-constant ->
 //        scalar-constant | scalar-constant-subobject |
 //        signed-int-literal-constant | signed-real-literal-constant |
 //        null-init | initial-data-target |
 //        structure-constructor
+// N.B. Parsing ambiguities abound here without recourse to symbols
+// (see comments on R845's parser).
 struct DataStmtConstant {
   UNION_CLASS_BOILERPLATE(DataStmtConstant);
   CharBlock source;
   mutable TypedExpr typedExpr;
-  std::variant<Scalar<ConstantValue>, Scalar<ConstantSubobject>,
-      SignedIntLiteralConstant, SignedRealLiteralConstant,
-      SignedComplexLiteralConstant, NullInit, InitialDataTarget,
-      StructureConstructor>
+  std::variant<LiteralConstant, SignedIntLiteralConstant,
+      SignedRealLiteralConstant, SignedComplexLiteralConstant, NullInit,
+      common::Indirection<Designator>, StructureConstructor>
       u;
 };
 
 // R844 data-stmt-repeat -> scalar-int-constant | scalar-int-constant-subobject
 // R607 int-constant -> constant
 // R604 constant -> literal-constant | named-constant
 // (only literal-constant -> int-literal-constant applies)
 struct DataStmtRepeat {

 WRAPPER_CLASS(BlockStmt, std::optional<Name>);
 
 // R1110 end-block-stmt -> END BLOCK [block-construct-name]
 WRAPPER_CLASS(EndBlockStmt, std::optional<Name>);
 
 // R1109 block-specification-part ->
 //         [use-stmt]... [import-stmt]...
 //         [[declaration-construct]... specification-construct]
-WRAPPER_CLASS(BlockSpecificationPart, SpecificationPart);
-// TODO: Because BlockSpecificationPart just wraps the more general
+// N.B. Because BlockSpecificationPart just wraps the more general
 // SpecificationPart, it can misrecognize an ImplicitPart as part of
 // the BlockSpecificationPart during parsing, and we have to detect and
 // flag such usage in semantics.
+WRAPPER_CLASS(BlockSpecificationPart, SpecificationPart);
 
 // R1107 block-construct ->
 //         block-stmt [block-specification-part] block end-block-stmt
 struct BlockConstruct {
   TUPLE_CLASS_BOILERPLATE(BlockConstruct);
   std::tuple<Statement<BlockStmt>, BlockSpecificationPart, Block,
       Statement<EndBlockStmt>>
       t;

 
 // R1132 end-do-stmt -> END DO [do-construct-name]
 WRAPPER_CLASS(EndDoStmt, std::optional<Name>);
 
 // R1131 end-do -> end-do-stmt | continue-stmt
 
 // R1119 do-construct -> do-stmt block end-do
 // R1120 do-stmt -> nonlabel-do-stmt | label-do-stmt
-// TODO: deprecated: DO loop ending on statement types other than END DO and
-// CONTINUE; multiple "label DO" loops ending on the same label
+// Deprecated, but supported: "label DO" loops ending on statements other
+// than END DO and CONTINUE, and multiple "label DO" loops ending on the
+// same label.
 struct DoConstruct {
   TUPLE_CLASS_BOILERPLATE(DoConstruct);
   const std::optional<LoopControl> &GetLoopControl() const;
   bool IsDoNormal() const;
   bool IsDoWhile() const;
   bool IsDoConcurrent() const;
   std::tuple<Statement<NonLabelDoStmt>, Block, Statement<EndDoStmt>> t;
 };

flang/lib/Evaluate/check-expression.cpp

if (semantics::IsNamedConstant(ultimate) || ultimate.owner().IsModule() ||
             ultimate.name().ToString() + "'";
       } else if (ultimate.has<semantics::ObjectEntityDetails>()) {
         return std::nullopt;
       } else {
         return "dummy procedure argument";
       }
     } else if (const auto *object{
                    ultimate.detailsIf<semantics::ObjectEntityDetails>()}) {
-      // TODO: what about EQUIVALENCE with data in COMMON?
-      // TODO: does this work for blank COMMON?
       if (object->commonBlock()) {
         return std::nullopt;
       }
     }
     for (const semantics::Scope *s{&scope_}; !s->IsGlobal();) {
       s = &s->parent();
       if (s == &ultimate.owner()) {
         return std::nullopt;

flang/lib/Evaluate/type.cpp

static bool IsDescriptor(const DeclTypeSpec *type) {
   }
   return false;
 }
 
 static bool IsDescriptor(const ObjectEntityDetails &details) {
   if (IsDescriptor(details.type())) {
     return true;
   }
-  // TODO: Automatic (adjustable) arrays - are they descriptors?
   for (const ShapeSpec &shapeSpec : details.shape()) {
     const auto &lb{shapeSpec.lbound().GetExplicit()};
     const auto &ub{shapeSpec.ubound().GetExplicit()};
     if (!lb || !ub || !IsConstantExpr(*lb) || !IsConstantExpr(*ub)) {
       return true;
     }
   }
   return false;

flang/lib/Parser/Fortran-parsers.cpp

 namespace Fortran::parser {
 
 // R601 alphanumeric-character -> letter | digit | underscore
 // R603 name -> letter [alphanumeric-character]...
 constexpr auto nonDigitIdChar{letter || otherIdChar};
 constexpr auto rawName{nonDigitIdChar >> many(nonDigitIdChar || digit)};
 TYPE_PARSER(space >> sourced(rawName >> construct<Name>()))
 
-// R604 constant ->  literal-constant | named-constant
-// Used only via R607 int-constant and R845 data-stmt-constant.
-// The look-ahead check prevents occlusion of constant-subobject in
-// data-stmt-constant.
-TYPE_PARSER(construct<ConstantValue>(literalConstant) ||
-    construct<ConstantValue>(namedConstant / !"%"_tok / !"("_tok))
-
 // R608 intrinsic-operator ->
 //        power-op | mult-op | add-op | concat-op | rel-op |
 //        not-op | and-op | or-op | equiv-op
 // R610 extended-intrinsic-op -> intrinsic-operator
 // These parsers must be ordered carefully to avoid misrecognition.
 constexpr auto namedIntrinsicOperator{
     ".LT." >> pure(DefinedOperator::IntrinsicOperator::LT) ||
     ".LE." >> pure(DefinedOperator::IntrinsicOperator::LE) ||
constexpr auto intrinsicOperator{
     namedIntrinsicOperator};
 
 // R609 defined-operator ->
 //        defined-unary-op | defined-binary-op | extended-intrinsic-op
 TYPE_PARSER(construct<DefinedOperator>(intrinsicOperator) ||
     construct<DefinedOperator>(definedOpName))
 
 // R505 implicit-part -> [implicit-part-stmt]... implicit-stmt
-// TODO: Can overshoot; any trailing PARAMETER, FORMAT, & ENTRY
-// statements after the last IMPLICIT should be transferred to the
-// list of declaration-constructs.
+// N.B. PARAMETER, FORMAT, & ENTRY statements that appear before any
+// other kind of declaration-construct will be parsed into the
+// implicit-part.
 TYPE_CONTEXT_PARSER("implicit part"_en_US,
     construct<ImplicitPart>(many(Parser<ImplicitPartStmt>{})))
 
 // R506 implicit-part-stmt ->
 //         implicit-stmt | parameter-stmt | format-stmt | entry-stmt
 TYPE_PARSER(first(
     construct<ImplicitPartStmt>(statement(indirect(Parser<ImplicitStmt>{}))),
     construct<ImplicitPartStmt>(statement(indirect(parameterStmt))),

 TYPE_PARSER(construct<DataStmtRepeat>(intLiteralConstant) ||
     construct<DataStmtRepeat>(scalar(integer(constantSubobject))))
 
 // R845 data-stmt-constant ->
 //        scalar-constant | scalar-constant-subobject |
 //        signed-int-literal-constant | signed-real-literal-constant |
 //        null-init | initial-data-target |
 //        constant-structure-constructor
-// null-init and a structure-constructor without parameters or components
-// are syntactically ambiguous in DATA, so "x()" is misparsed into a
-// null-init then fixed up later in expression semantics.
-// TODO: Some structure constructors can be misrecognized as array
-// references into constant subobjects.
-TYPE_PARSER(sourced(first(
-    construct<DataStmtConstant>(scalar(Parser<ConstantValue>{})),
-    construct<DataStmtConstant>(nullInit),
-    construct<DataStmtConstant>(scalar(constantSubobject)) / !"("_tok,
-    construct<DataStmtConstant>(Parser<StructureConstructor>{}),
+// N.B. scalar-constant and scalar-constant-subobject are ambiguous with
+// initial-data-target; null-init and structure-constructor are ambiguous
+// in the absence of parameters and components; structure-constructor with
+// components can be ambiguous with a scalar-constant-subobject.
+// So we parse literal constants, designator, null-init, and
+// structure-constructor, so that semantics can figure things out later
+// with the symbol table.
+TYPE_PARSER(sourced(first(construct<DataStmtConstant>(literalConstant),
     construct<DataStmtConstant>(signedRealLiteralConstant),
     construct<DataStmtConstant>(signedIntLiteralConstant),
     extension<LanguageFeature::SignedComplexLiteral>(
         construct<DataStmtConstant>(Parser<SignedComplexLiteralConstant>{})),
-    construct<DataStmtConstant>(initialDataTarget))))
+    construct<DataStmtConstant>(nullInit),
+    construct<DataStmtConstant>(indirect(designator) / !"("_tok),
+    construct<DataStmtConstant>(Parser<StructureConstructor>{}))))
 
 // R848 dimension-stmt ->
 //        DIMENSION [::] array-name ( array-spec )
 //        [, array-name ( array-spec )]...
 TYPE_CONTEXT_PARSER("DIMENSION statement"_en_US,
     construct<DimensionStmt>("DIMENSION" >> maybe("::"_tok) >>
         nonemptyList("expected array specifications"_err_en_US,
             construct<DimensionStmt::Declaration>(name, arraySpec))))

 
 // R912 part-ref -> part-name [( section-subscript-list )] [image-selector]
 TYPE_PARSER(construct<PartRef>(name,
     defaulted(
         parenthesized(nonemptyList(Parser<SectionSubscript>{})) / !"=>"_tok),
     maybe(Parser<ImageSelector>{})))
 
 // R913 structure-component -> data-ref
+// The final part-ref in the data-ref is not allowed to have subscripts.
 TYPE_PARSER(construct<StructureComponent>(
     construct<DataRef>(some(Parser<PartRef>{} / percentOrDot)), name))
 
 // R919 subscript -> scalar-int-expr
 constexpr auto subscript{scalarIntExpr};
 
 // R920 section-subscript -> subscript | subscript-triplet | vector-subscript
 // R923 vector-subscript -> int-expr
TYPE_PARSER(construct<AllocOpt>(
     construct<AllocOpt>(statOrErrmsg))
 
 // R929 stat-variable -> scalar-int-variable
 TYPE_PARSER(construct<StatVariable>(scalar(integer(variable))))
 
 // R932 allocation ->
 //        allocate-object [( allocate-shape-spec-list )]
 //        [lbracket allocate-coarray-spec rbracket]
-// TODO: allocate-shape-spec-list might be misrecognized as
-// the final list of subscripts in allocate-object.
 TYPE_PARSER(construct<Allocation>(Parser<AllocateObject>{},
     defaulted(parenthesized(nonemptyList(Parser<AllocateShapeSpec>{}))),
     maybe(bracketed(Parser<AllocateCoarraySpec>{}))))
 
 // R933 allocate-object -> variable-name | structure-component
 TYPE_PARSER(construct<AllocateObject>(structureComponent) ||
     construct<AllocateObject>(name / !"="_tok))
 

flang/lib/Parser/program-parsers.cpp

 //           OPERATOR ( use-defined-operator )
 TYPE_PARSER(construct<Rename>("OPERATOR (" >>
                 construct<Rename::Operators>(
                     definedOpName / ") => OPERATOR (", definedOpName / ")")) ||
     construct<Rename>(construct<Rename::Names>(name, "=>" >> name)))
 
 // R1412 only -> generic-spec | only-use-name | rename
 // R1413 only-use-name -> use-name
+// N.B. generic-spec and only-use-name are ambiguous; resolved with symbols
 TYPE_PARSER(construct<Only>(Parser<Rename>{}) ||
-    construct<Only>(indirect(genericSpec)) ||
-    construct<Only>(name)) // TODO: ambiguous, accepted by genericSpec
+    construct<Only>(indirect(genericSpec)) || construct<Only>(name))
 
 // R1416 submodule ->
 //         submodule-stmt [specification-part] [module-subprogram-part]
 //         end-submodule-stmt
 TYPE_CONTEXT_PARSER("submodule"_en_US,
     construct<Submodule>(statement(Parser<SubmoduleStmt>{}),
         limitedSpecificationPart, maybe(Parser<ModuleSubprogramPart>{}),
         unterminatedStatement(Parser<EndSubmoduleStmt>{})))

flang/lib/Semantics/data-to-inits.cpp

if (!expr) {
     }
   } else if (evaluate::IsNullPointer(*expr)) {
     exprAnalyzer_.Say("Initializer for '%s' must not be a pointer"_err_en_US,
         DescribeElement());
   } else if (evaluate::IsProcedure(*expr)) {
     exprAnalyzer_.Say("Initializer for '%s' must not be a procedure"_err_en_US,
         DescribeElement());
   } else if (auto designatorType{designator.GetType()}) {
-    if (auto converted{ConvertElement(*expr, *designatorType)}) {
+    if (expr->Rank() > 0) {
+      // Because initial-data-target is ambiguous with scalar-constant and
+      // scalar-constant-subobject at parse time, enforcement of scalar-*
+      // must be deferred to here.
+      exprAnalyzer_.Say(
+          "DATA statement value initializes '%s' with an array"_err_en_US,
+          DescribeElement());
+    } else if (auto converted{ConvertElement(*expr, *designatorType)}) {
       // value non-pointer initialization
       if (std::holds_alternative<evaluate::BOZLiteralConstant>(expr->u) &&
           designatorType->category() != TypeCategory::Integer) { // 8.6.7(11)
         exprAnalyzer_.Say(
             "BOZ literal should appear in a DATA statement only as a value for an integer object, but '%s' is '%s'"_en_US,
             DescribeElement(), designatorType->AsFortran());
       } else if (converted->second) {
         exprAnalyzer_.context().Say(

flang/lib/Semantics/rewrite-parse-tree.cpp

public:
   template <typename T> void Post(T &) {}
 
   void Post(parser::Name &);
   void Post(parser::SpecificationPart &);
   bool Pre(parser::ExecutionPart &);
   void Post(parser::IoUnit &);
   void Post(parser::ReadStmt &);
   void Post(parser::WriteStmt &);
-  void Post(parser::DataStmtConstant &);
 
   // Name resolution yet implemented:
   // TODO: Can some/all of these now be enabled?
   bool Pre(parser::EquivalenceStmt &) { return false; }
   bool Pre(parser::Keyword &) { return false; }
   bool Pre(parser::EntryStmt &) { return false; }
   bool Pre(parser::CompilerDirective &) { return false; }
 
void RewriteMutator::Post(parser::ReadStmt &x) {
   }
   FixMisparsedUntaggedNamelistName(x);
 }
 
 void RewriteMutator::Post(parser::WriteStmt &x) {
   FixMisparsedUntaggedNamelistName(x);
 }
 
-void RewriteMutator::Post(parser::DataStmtConstant &x) {
-  if (auto *scalar{std::get_if<parser::Scalar<parser::ConstantValue>>(&x.u)}) {
-    if (auto *named{std::get_if<parser::NamedConstant>(&scalar->thing.u)}) {
-      if (const Symbol * symbol{named->v.symbol}) {
-        if (!IsNamedConstant(*symbol) && symbol->attrs().test(Attr::TARGET)) {
-          x.u = parser::InitialDataTarget{
-              parser::Designator{parser::DataRef{parser::Name{named->v}}}};
-        }
-      }
-    }
-  }
-}
-
 bool RewriteParseTree(SemanticsContext &context, parser::Program &program) {
   RewriteMutator mutator{context};
   parser::Walk(program, mutator);
   return !context.AnyFatalError();
 }
 
 } // namespace Fortran::semantics

flang/test/Semantics/data01.f90

subroutine CheckValue
   data myname(3) / person(myAge, 'Abcd Ijkl') /
   integer, parameter :: a(5) =(/11, 22, 33, 44, 55/)
   integer :: b(5) =(/11, 22, 33, 44, 55/)
   integer :: i
   integer :: x, y, z
   !OK: constant array element
   data x / a(1) /
   !C886, C887
-  !ERROR: Must be a constant value
+  !ERROR: DATA statement value 'a(int(i,kind=8))' for 'y' is not a constant
   data y / a(i) /
-  !ERROR: Must be a constant value
+  !ERROR: DATA statement value 'b(1_8)' for 'z' is not a constant
   data z / b(1) /
 end

flang/test/Semantics/data06.f90

subroutine s1
   data rt/rfunc/
   integer :: jx, jy
   !WARNING: DATA statement value initializes 'jx' of type 'INTEGER(4)' with CHARACTER
   data jx/'abc'/
   !ERROR: DATA statement value could not be converted to the type 'INTEGER(4)' of the object 'jx'
   data jx/t1()/
   !ERROR: DATA statement value could not be converted to the type 'INTEGER(4)' of the object 'jx'
   data jx/.false./
-  !ERROR: must be a constant
+  !ERROR: DATA statement value 'jy' for 'jx' is not a constant
   data jx/jy/
 end subroutine

flang/test/Semantics/data10.f90

+! RUN: %S/test_errors.sh %s %t %f18
+type :: t
+  integer :: n
+end type
+type(t) :: x
+real, target, save :: a(1)
+real, parameter :: arrparm(1) = [3.14159]
+real, pointer :: p
+real :: y
+data x/t(1)/
+data p/a(1)/
+!ERROR: DATA statement value initializes 'y' with an array
+data y/arrparm/
+end