Why doesn't iterate over a container accessed directly through std::optional::value() work?

Question

I'm trying to iterate over a std::vector<X> contained in a struct T that I access through a std::optional<T>. Contrary to my expectations, the behavior is different if I first store the std::optional<T> into a copy versus if I iterate it directly:

#include <optional>
#include <string>
#include <vector>

// The rest is the same in both implementations    
struct Object
{
    std::string id;
    std::vector<SomeStructType> items;
};

Object readDataFile() { /*....*/ return {}; }
bool dataFileExists() { /*....*/ return true; }

std::optional<Object> getData()
{
    if (!dataFileExists()) {
        return std::nullopt;
    }

    Object obj = readDataFile();
    return obj;
}

int main()
{
    // Implementation 1 - works
    auto items = getData().value().items;
    for (auto const& item : items)
    {
        // will normally iterate over items
    }

    // Implementation 2 - for loop is skipped
    for (auto const& item : getData().value().items)
    {
        // this body will never execute
    }
}

I would expect those two to behave the same. Why does implementation 2 skip the for loop? `

The file read is the same in both cases, with items.size() == 1. I omitted some other nullopt checks and asserts in the provided code, but I don't expect them to be relevant.

Code always exists in some context. You should always provide code inside a function to provide such context. In this case you should provide [mcve]. — Marek R, Jun 30 '23 at 09:41
https://stackoverflow.com/questions/37635908/temporary-optional-in-the-range-based-for-loop-expression — pptaszni, Jun 30 '23 at 09:44

JeJo · Accepted Answer · 2023-06-30T10:03:35.107

would expect those two to behave the same. Why does implementation 2 skip the for loop?

In your second implementation getData() returns a temporary std::optional<Object> which will be destroyed end of the expression (in other words, even before the range-based for loop starts).

for (auto const &item : getData().value().items)
//                      ^^^^^^^^^ ---> temporary std::optional<Object>
{
}

The range-based for loop thus holds the ranges of the invalid items. Using this leads to your program's undefined behavior. In your case, the loop didn't get executed.

That being said, if the compiler warnings are turned on, you would have already been warned about it. For instance, clang 15 with -Wall -Wextra -pedantic-errors -fsanitize=address,undefined says:

<source>:25:29: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
    for (auto const& item : getData().value().items)
                            ^~~~~~~~~
1 warning generated.
ASM generation compiler returned: 0
<source>:25:29: warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
    for (auto const& item : getData().value().items)
                            ^~~~~~~~~
//...
=================================================================
==1==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7f1262400040 at pc 0x55ba03d4ae30 bp 0x7ffd89ca2a90 sp 0x7ffd89ca2a88
//...

You can increase the lifetime with some tweaks. See an example mentioned in this post.

Thank you. I'm using MSVC and its code analysis didn't point this out ... Better switch to clang-tidy I guess! — w128, Jun 30 '23 at 10:07

Why doesn't iterate over a container accessed directly through std::optional::value() work?

1 Answers1