[flang] Restore checking for some optional values before use

[klausler]

Summary

Recent commits (2098ad7f00324ee0f2a6538f418a6f81dfdd2edb and
15a9a72ee68166c0cff3f036cacd3c82be66c729) replaced usage of "o.value()"
on optionals with "*o". Those optional values are expected to be
present, but now, if they're not, compilation will proceed with
garbage data rather than crashing on an uncaught exception.
Add asserts for presence to restore the previous level of safety.

[vzakhari]

LGTM

[klausler]

Update patch.

[jeanPerier]

flang/lib/Lower/ConvertExpr.cpp

3673

lhsLoad.has_value()

[klausler]

Incorporate Jean's suggested change.

[tschuett]

Are asserts the right solution or do you need control flow to check whether optionals have a value?

flang/include/flang/Evaluate/tools.h

template <TypeCategory TOCAT, typename VALUE> struct ConvertToKindHelper {
   }
   int kind;
   VALUE value;
 };
 
 template <TypeCategory TOCAT, typename VALUE>
 common::IfNoLvalue<Expr<SomeKind<TOCAT>>, VALUE> ConvertToKind(
     int kind, VALUE &&x) {
-  return *common::SearchTypes(
-      ConvertToKindHelper<TOCAT, VALUE>{kind, std::move(x)});
+  auto result{common::SearchTypes(
+      ConvertToKindHelper<TOCAT, VALUE>{kind, std::move(x)})};
+  CHECK(result.has_value());
+  return *result;
 }
 
 // Given a type category CAT, SameKindExprs<CAT, N> is a variant that
 // holds an arrays of expressions of the same supported kind in that
 // category.
 template <typename A, int N = 2> using SameExprs = std::array<Expr<A>, N>;
 template <int N = 2> struct SameKindExprsHelper {
   template <typename A> using SameExprs = std::array<Expr<A>, N>;

flang/lib/Lower/Bridge.cpp

std::visit(
                     lhs, rhs);
               } else if (isDerivedCategory(lhsType->category())) {
                 // Fortran 2018 10.2.1.3 p13 and p14
                 // Recursively gen an assignment on each element pair.
                 fir::factory::genRecordAssignment(*builder, loc, lhs, rhs);
               } else {
                 llvm_unreachable("unknown category");
               }
-              if (lhsIsWholeAllocatable)
+              if (lhsIsWholeAllocatable) {
+                assert(lhsRealloc.has_value());
                 fir::factory::finalizeRealloc(*builder, loc, *lhsMutableBox,
                                               /*lbounds=*/std::nullopt,
                                               /*takeLboundsIfRealloc=*/false,
                                               *lhsRealloc);
+              }
             },
 
             // [2] User defined assignment. If the context is a scalar
             // expression then call the procedure.
             [&](const Fortran::evaluate::ProcedureRef &procRef) {
               Fortran::lower::StatementContext &ctx =
                   explicitIterationSpace() ? explicitIterSpace.stmtContext()
                                            : stmtCtx;

flang/lib/Lower/ConvertCall.cpp

fir::ExtendedValue Fortran::lower::genCallOpAndResult(
   } else {
     // Standard procedure call with fir.call.
     auto call = builder.create<fir::CallOp>(loc, funcType.getResults(),
                                             funcSymbolAttr, operands);
     callResult = call.getResult(0);
     callNumResults = call.getNumResults();
   }
 
-  if (caller.mustSaveResult())
+  if (caller.mustSaveResult()) {
+    assert(allocatedResult.has_value());
     builder.create<fir::SaveResultOp>(loc, callResult,
                                       fir::getBase(*allocatedResult),
                                       arrayResultShape, resultLengths);
+  }
 
   if (allocatedResult) {
     allocatedResult->match(
         [&](const fir::MutableBoxValue &box) {
           if (box.isAllocatable()) {
             // 9.7.3.2 point 4. Finalize allocatables.
             fir::FirOpBuilder *bldr = &converter.getFirOpBuilder();
             stmtCtx.attachCleanup([bldr, loc, box]() {

flang/lib/Lower/ConvertExpr.cpp

ExtValue lowerScalarAssignment(const Fortran::lower::SomeExpr &lhs,
     // 3) Finalize the inner context.
     explicitSpace->finalizeContext();
     // 4) Thread the array value updated forward. Note: the lhs might be
     // ill-formed (performing scalar assignment in an array context),
     // in which case there is no array to thread.
     auto loc = getLoc();
     auto createResult = [&](auto op) {
       mlir::Value oldInnerArg = op.getSequence();
       std::size_t offset = explicitSpace->argPosition(oldInnerArg);
       explicitSpace->setInnerArg(offset, fir::getBase(lexv));
       finalizeElementCtx();
       builder.create<fir::ResultOp>(loc, fir::getBase(lexv));
     };
     if (mlir::Operation *defOp = fir::getBase(lexv).getDefiningOp()) {
       llvm::TypeSwitch<mlir::Operation *>(defOp)
           .Case([&](fir::ArrayUpdateOp op) { createResult(op); })
           .Case([&](fir::ArrayAmendOp op) { createResult(op); })
ExtValue lowerScalarUserAssignment(mlir::func::FuncOp userAssignment,
     auto lexv = genarr(lhs)(iters);
     bool isIllFormedLHS = false;
     // 3) Insert the call
     if (auto modifyOp = mlir::dyn_cast<fir::ArrayModifyOp>(
             fir::getBase(lexv).getDefiningOp())) {
       mlir::Value oldInnerArg = modifyOp.getSequence();
       std::size_t offset = explicitSpace->argPosition(oldInnerArg);
       explicitSpace->setInnerArg(offset, fir::getBase(lexv));
-      fir::ExtendedValue exv = arrayModifyToExv(
-          builder, loc, *explicitSpace->getLhsLoad(0), modifyOp.getResult(0));
+      auto lhsLoad = explicitSpace->getLhsLoad(0);
+      assert(lhsLoad.has_value());
+      fir::ExtendedValue exv =
+          arrayModifyToExv(builder, loc, *lhsLoad, modifyOp.getResult(0));
       genScalarUserDefinedAssignmentCall(builder, loc, userAssignment, exv,
                                          elementalExv);
     } else {
       // LHS is ill formed, it is a scalar with no references to FORALL
       // subscripts, so there is actually no array assignment here. The user
       // code is probably bad, but still insert user assignment call since it
       // was not rejected by semantics (a warning was emitted).
       isIllFormedLHS = true;
if (isPointerAssignment() && eleTy.isa<fir::BoxType>() &&
           fir::hasDynamicSize(eleTy))
         TODO(loc, "TARGET of pointer assignment with runtime size/shape");
       auto memrefTy = fir::boxMemRefType(eleTy.cast<fir::BoxType>());
       auto castTo = builder.createConvert(loc, memrefTy, origVal);
       origVal = builder.create<fir::EmboxOp>(loc, eleTy, castTo);
     }
     mlir::Value val = builder.createConvert(loc, eleTy, origVal);
     if (isBoundsSpec()) {
+      assert(lbounds.has_value());
       auto lbs = *lbounds;
       if (lbs.size() > 0) {
         // Rebox the value with user-specified shift.
         auto shiftTy = fir::ShiftType::get(eleTy.getContext(), lbs.size());
         mlir::Value shiftOp = builder.create<fir::ShiftOp>(loc, shiftTy, lbs);
         val = builder.create<fir::ReboxOp>(loc, eleTy, val, shiftOp,
                                            mlir::Value{});
       }
     } else if (isBoundsRemap()) {
+      assert(lbounds.has_value());
       auto lbs = *lbounds;
       if (lbs.size() > 0) {
         // Rebox the value with user-specified shift and shape.
+        assert(ubounds.has_value());
         auto shapeShiftArgs = flatZip(lbs, *ubounds);
         auto shapeTy = fir::ShapeShiftType::get(eleTy.getContext(), lbs.size());
         mlir::Value shapeShift =
             builder.create<fir::ShapeShiftOp>(loc, shapeTy, shapeShiftArgs);
         val = builder.create<fir::ReboxOp>(loc, eleTy, val, shapeShift,
                                            mlir::Value{});
       }
     }
for (const Fortran::evaluate::ArrayConstructorValue<A> &acv : x.values()) {
       mlir::Value eleSz = computeElementSize(exv, eleTy, resTy);
       mem = copyNeeded ? copyNextArrayCtorSection(exv, buffPos, buffSize, mem,
                                                   eleSz, eleTy, eleRefTy, resTy)
                        : fir::getBase(exv);
       if (fir::isa_char(seqTy.getEleTy()) && !charLen) {
         charLen = builder.createTemporary(loc, builder.getI64Type());
         mlir::Value castLen =
             builder.createConvert(loc, builder.getI64Type(), fir::getLen(exv));
+        assert(charLen.has_value());
         builder.create<fir::StoreOp>(loc, castLen, *charLen);
       }
     }
     stmtCtx.finalizeAndPop();
 
     builder.create<fir::ResultOp>(loc, mem);
     builder.restoreInsertionPoint(insPt);
     mem = loop.getResult(0);
     symMap.popImpliedDoBinding();
     llvm::SmallVector<mlir::Value> extents = {
         builder.create<fir::LoadOp>(loc, buffPos).getResult()};
 
     // Convert to extended value.
     if (fir::isa_char(seqTy.getEleTy())) {
+      assert(charLen.has_value());
       auto len = builder.create<fir::LoadOp>(loc, *charLen);
       return {fir::CharArrayBoxValue{mem, len, extents}, /*needCopy=*/false};
     }
     return {fir::ArrayBoxValue{mem, extents}, /*needCopy=*/false};
   }
 
   // To simplify the handling and interaction between the various cases, array
   // constructors are always lowered to the incremental construction code

flang/lib/Lower/ConvertExprToHLFIR.cpp

return HlfirDesignatorBuilder(getLoc(), getConverter(), getSymMap(),
         .gen(designator.u);
   }
 
   template <typename T>
   hlfir::EntityWithAttributes
   gen(const Fortran::evaluate::FunctionRef<T> &expr) {
     mlir::Type resType =
         Fortran::lower::TypeBuilder<T>::genType(getConverter(), expr);
-    return *Fortran::lower::convertCallToHLFIR(
+    auto result = Fortran::lower::convertCallToHLFIR(
         getLoc(), getConverter(), expr, resType, getSymMap(), getStmtCtx());
+    assert(result.has_value());
+    return *result;
   }
 
   template <typename T>
   hlfir::EntityWithAttributes gen(const Fortran::evaluate::Constant<T> &expr) {
     mlir::Location loc = getLoc();
     if constexpr (std::is_same_v<T, Fortran::evaluate::SomeDerived>) {
       TODO(loc, "lowering derived type constant to HLFIR");
     } else {

flang/lib/Lower/CustomIntrinsicCall.cpp

lowerIshftc(fir::FirOpBuilder &builder, mlir::Location loc,
   assert(numOperands == 3 && !isPresentCheck(0) && !isPresentCheck(1) &&
          isPresentCheck(2) &&
          "only ISHFTC SIZE arg is expected to be dynamically optional here");
   assert(retTy && "ISFHTC must have a return type");
   mlir::Type resultType = *retTy;
   llvm::SmallVector<fir::ExtendedValue> args;
   args.push_back(getOperand(0));
   args.push_back(getOperand(1));
+  auto iPC = isPresentCheck(2);
+  assert(iPC.has_value());
   args.push_back(builder
-                     .genIfOp(loc, {resultType}, *isPresentCheck(2),
+                     .genIfOp(loc, {resultType}, *iPC,
                               /*withElseRegion=*/true)
                      .genThen([&]() {
                        fir::ExtendedValue sizeExv = getOperand(2);
                        mlir::Value size = builder.createConvert(
                            loc, resultType, fir::getBase(sizeExv));
                        builder.create<fir::ResultOp>(loc, size);
                      })
                      .genElse([&]() {