Deprecating llvm::Optional<X>::hasValue/getValue/getValueOr

JDevlieghere · 2022-08-11T05:06:27.180Z

nikic:

This means that we need to include <optional> in all headers, while currently it can usually be forward-declared.

How many files are doing that though? I could only find a handful in LLVM itself and even less in clang. Are there subprojects that are heavily relying on this?

jh7370 · 2022-08-11T08:43:14.275Z

Strong +1 to migrating to STL equivalents where possible (I’m assuming where performance of STL equivalents is poor, we consider it not possible here). It makes it substantially easier for users of LLVM to integrate it into other external projects, because they can use the same interfaces. Otherwise, we have to provide additional complexity to convert from STL to LLVM types and vice versa.

kazutakahirata · 2022-11-19T22:47:35.972Z

It looks like several people suggest migration to std::optional. I’ll start submitting patches. Thanks!

troggo · 2022-11-21T19:41:44.420Z

What’s the behaviour when accessing the value of a null std::optional? For llvm::Optional I believe both operator* and value() will assert in debug/LLVM_ENABLE_ASSERTIONS builds.

kazutakahirata · 2022-11-21T20:11:08.563Z

libcxx/include/optional says:

    _LIBCPP_INLINE_VISIBILITY
    _LIBCPP_AVAILABILITY_THROW_BAD_OPTIONAL_ACCESS
    constexpr value_type const& value() const&
    {
        if (!this->has_value())
            __throw_bad_optional_access();
        return this->__get();
    }

with __throw_bad_optional_access defined as:

_LIBCPP_NORETURN
inline _LIBCPP_INLINE_VISIBILITY
_LIBCPP_AVAILABILITY_THROW_BAD_OPTIONAL_ACCESS
void __throw_bad_optional_access() {
#ifndef _LIBCPP_NO_EXCEPTIONS
        throw bad_optional_access();
#else
        _VSTD::abort();
#endif
}

blangmuir · 2022-11-28T21:53:12.674Z

kazutakahirata:

It looks like several people suggest migration to std::optional. I’ll start submitting patches. Thanks!

kazutakahirata:

We need to decide what to do with custom storage classes like MapEntryOptionalStorage,

Was this issue addressed yet? I was hoping we would have a solution to custom storage before replacing llvm::Optional with std::optional.

kazutakahirata · 2022-11-29T08:05:11.154Z

blangmuir:

kazutakahirata:

We need to decide what to do with custom storage classes like MapEntryOptionalStorage,

Was this issue addressed yet? I was hoping we would have a solution to custom storage before replacing llvm::Optional with std::optional.

Not yet. We probably have to provide a complete template specialization.

template<>
class std::optional<MyClass> {
  :
  :
};

If you look at MapEntryOptionalStorage, it pretty much provides all important methods on its own anyway, relying on llvm::Optional only for syntax sugar like value_or, operator->, etc.

AFAICT, clang::FileEntryRef and DirectoryEntryRef are the only direct users of OptionalStorage, so duplicating much of std::optional is probably OK.

thakis · 2022-12-16T20:49:47.208Z

Hello, many patches to switch to std::optional have landed by now.

It looks like std::optional::value() isn’t available on macOS before macOS 10.13, so any call of that function breaks building for 10.12. The fix is easy (use operator* instead of value()), but that’s likely going to be a fairly annoying thing to fix going forward.

Any thoughts on what to do about that?

tschuett · 2022-12-16T21:05:46.637Z

Style guide?

if (op) 
  *op

Use has_value() only in unit tests.

MaskRay · 2022-12-16T22:20:32.512Z

std::optional::value() has undesired exception checking semantics and calls __throw_bad_optional_access in libc++. Moreover, the API is unavailable without _LIBCPP_NO_EXCEPTIONS on older Mach-O platforms (e.g. macOS<10.13), see _LIBCPP_AVAILABILITY_BAD_OPTIONAL_ACCESS in libcxx/.

At this point, the only practical approach is to migrate value() to operator* (and sometimes operator-> or value_or). We’ll have to find previous llvm::Optional::getValue call sites like
611ffcf4e4a2ab19063174f6990969f96e9078de and change them to operator*.

My language server says there are 400+ references.

… but that’s likely going to be a fairly annoying thing to fix going forward.

Hopefully not many people use value(). Many have learned to discourage std::vector::at in favor of operator[].

I think llvm::Optional::value needs LLVM_DEPRECATED to discourage uses.

s-barannikov · 2022-12-17T23:08:00.449Z

__throw_bad_optional_access calls abort unless exceptions are enabled so this part does not seem to be an issue.
I mean, this is not an argument to convert everything to *X and deprecate value(). The only real reason is that the API isn’t available on some older platforms.

pogo59 · 2022-12-19T13:48:22.047Z

I should make sure people are aware of Undo one llvm::Optional => std::optional · llvm/llvm-project@08c8280 · GitHub which we ran into with older gcc.

tschuett · 2022-12-19T16:45:19.319Z

The last RFC moved to GCC 7.1. So you have a pretty new GCC.

pogo59 · 2022-12-19T17:33:04.616Z

“Older” in the sense of “older than most people use” but yes, still officially supported (gcc 7.5 is from 2017).

tschuett · 2022-12-19T17:57:49.916Z

Sorry. It is totally valid to run gcc 7.5 against LLVM and complain about errors. Pretty new and older can be interpreted against the minimal requirements or the current year.

pogo59 · 2022-12-20T22:58:09.072Z

Someone might want to look into Recent Optional commit(s) + clang-11 on ubuntu 20.04 cause broken compiler, many test failures · Issue #59622 · llvm/llvm-project · GitHub

slinder1 · 2023-01-18T22:57:42.029Z

I don’t think we can use operator* here, as there is nothing guaranteeing it asserts.

I’m sure I’m not the only developer who has historically used llvm::Optional::operator* without an additional assert because it was defined to do the assert internally. This implies that just swapping in std::optional for llvm::Optional is equivalent to deleting a lot of important asserts.

As an example of a change making this mistake see 467432899bc2f71842ed1b24d24c094da02af7d4 “MemoryLocation: convert Optional to std::optional”. Before that change, this code would always assert in get if it saw None:

static MemoryLocation get(const Instruction *Inst) {
  return *MemoryLocation::getOrNone(Inst);
  //     ^ Guaranteed to assert(hasValue())
}
static Optional<MemoryLocation> getOrNone(const Instruction *Inst);

The change should have retained the assert one way or another. Assuming the value method is an issue on some platforms, the most straightforward option is to add a temporary and use a plain assert:

static MemoryLocation get(const Instruction *Inst) {
  auto ML = MemoryLocation::getOrNone(Inst);
  assert(ML);
  return *ML;
}
static std::optional<MemoryLocation> getOrNone(const Instruction *Inst);

I think this harms readability on several fronts (stretches vertically, breaks up expressions needlessly, and forces the reader to consider the possibility that the temporary variable has some other purpose later in the scope), so I want to propose something I think is an improvement over the original llvm::Optional version: a Rust-like unwrap/expect, but implemented as a free-function. Then the above becomes:

static MemoryLocation get(const Instruction *Inst) {
  return unwrap(MemoryLocation::getOrNone(Inst));
}
static std::optional<MemoryLocation> getOrNone(const Instruction *Inst);

Bikeshedding on naming is welcome, but the point is just to have an assert-and-then-dereference style function that can be used uniformly for these cases. It could be as simple as a new header llvm/ADT/Unwrap.h with a single function:

namespace llvm {

/// Assert the truthiness of, and dereference, fallible
/// wrapper-like object \p C.
template <typename T>
inline auto unwrap(T &&Wrapper) -> decltype(*std::forward<T>(Wrapper)) {
  assert(Wrapper);
  return *std::forward<T>(Wrapper);
}

} // namespace llvm

For cases trivially dominated by a check we could still safely allow the bare dereference operator:

std::optional<int> I;
if (I)
  return *I; // Fine

Thoughts?

s-barannikov · 2023-01-19T00:01:32.746Z

slinder1:

I don’t think we can use operator* here, as there is nothing guaranteeing it asserts.

I’m pretty sure all standard library vendors do assert that it contains a value (at least in debug builds), but I can’t disagree that relying on that behavior is a bad idea. Every access to optional must be guarded by a check that it contains a value.

I think this harms readability on several fronts (stretches vertically, breaks up expressions needlessly, and forces the reader to consider the possibility that the temporary variable has some other purpose later in the scope)

I disagree. The assert (that should contain a message) tells the reader that there is a contract, and not that someone forgot to add a check.

a Rust-like unwrap/expect, but implemented as a free-function.

Introducing an asserting wrapper may sound like a good idea. However, if you find yourself using it often, this probably indicates a problem with your code. Likely, you don’t need optional if you don’t check that it contains a value (i.e. if it is not really optional).

slinder1 · 2023-01-19T00:39:58.719Z

I’m pretty sure all standard library vendors do assert that it contains a value (at least in debug builds), but I can’t disagree that relying on that behavior is a bad idea. Every access to optional must be guarded by a check that it contains a value.

Unfortunately, that doesn’t even seem to be the case. I compared using operator*, value, and an assert locally and my stdlib seems to happily let the undefined operator* return, and the process exits normally with a successful status. I’ve seen reports that the same is true of at least some versions of MSVC.

I disagree. The assert (that should contain a message) tells the reader that there is a contract, and not that someone forgot to add a check.

I agree that the intent should be explicit, I just think a new function that generically captures the intent is better than requiring an intervening assignment. I also don’t think every assert needs a message, in the same way that every line of code does not need a comment, but I would certainly be happy having a version which supports a message (Rust has unwrap for no-message and expect for message, we could do the same).

Introducing an asserting wrapper may sound like a good idea. However, if you find yourself using it often, this probably indicates a problem with your code. Likely, you don’t need optional if you don’t check that it contains a value (i.e. if it is not really optional).

I absolutely agree, but LLVM is a large codebase, and is constantly evolving. There are always more yaks to shave when refactoring, and when making incremental improvements it is fairly common to need to express this kind of invariant just to match an existing assumption in the code.

An example is D141968 [NFC] Consolidate llvm::CodeGenOpt::Level handling where most of the existing places constructing CodeGenOpt::Level involve asserts, but some handle the invalid IDs directly. In making the interface correct and idiomatic for the error-handling case it becomes needlessly more cumbersome to adapt the other cases.

s-barannikov · 2023-01-19T02:43:32.783Z

slinder1:

An example is D141968 [NFC] Consolidate llvm::CodeGenOpt::Level handling where most of the existing places constructing CodeGenOpt::Level involve asserts, but some handle the invalid IDs directly.

In this particular case I’d personally go with assert inside getLevel since its argument is preconditioned to be within the range. parseLevel is different, and having it return optional is the right thing.