[clang-tidy]: `bugprone-unchecked-optional-access` false-positive when modifying optional field via setter method (by reference)

[BaLiKfromUA]

Hi folks!

Recently, I found an interesting bug related to accessing an optional field by reference via a method.

Initially, I thought that it might be in the same nature as #128437, but now I think it's much more complicated.

Let's assume we have next code:

class A {
private:
  std::optional<int> d_field;

public:
  const std::optional<int>& getField() const {
        return d_field;
  } 

  std::optional<int>& setField() {
        return d_field;
  } 

private:
  // no violations
  void do_something_with_optional() {
    if (!d_field.has_value()) {
        d_field = 42;
    }

    std::cout << d_field.value();
  }
};

Code below triggers warnings from check even though it's technically similar to A::do_something_with_optional():

// false positive
void foo1(A data) {
    if (!data.getField().has_value()) {
        data.setField() = 42;
    }

    std::cout << data.getField().value();
}

It also can be reproduced even by code like this:

// false positive
void foo2(A data) {
    data.setField() = 42;
    std::cout << data.getField().value();
}

My understanding why it doesn't work

IIRC the implementation of a given check, we don't analyze the access to non-const methods, even if they return optional field by reference. Plus, any access to them invalidate the stored state of related optional field that we have seen before.

Probably the fix is not simple since we cannot just rely on the method name (as I did in my previous fix), we need to understand that it returns exactly the same optional field as was modified before.

I believe the proper fix also might address the issue like #151287 when we call non-const method which doesn't modify optional data, but maybe I am wrong :)

I am happy to explore the solution but need an initial guidance.

Thanks!

Compiler explorer link with my experiments

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Valentyn Yukhymenko (BaLiKfromUA)

Hi folks!

Recently I found interesting bug related to accessing optional field via getter.

Initially I thought that it might be in the same nature as #128437 but now I think it's much more complicated.

Let's assume we have next code:

class A {
private:
  std::optional<int> d_field;

public:
  const std::optional<int>& getField() const {
        return d_field;
  } 

  std::optional<int>& setField() {
        return d_field;
  } 

private:
  // no violations
  void do_something_with_optional() {
    if (!d_field.has_value()) {
        d_field = 42;
    }

    std::cout << d_field.value();
  }
};

Code below triggers warnings from check even though it's technically similar to A::do_something_with_optional():

// false positive
void foo1(A data) {
    if (!data.getField().has_value()) {
        data.setField() = 42;
    }

    std::cout << data.getField().value();
}

It also can be reproduced even by code like this:

// false positive
void foo2(A data) {
    data.setField() = 42;
    std::cout << data.getField().value();
}

My understanding why it doesn't work

IIRC the implementation of a given check, we don't analyze the access to non-const methods, even if they return optional field by reference. Plus, any access to them invalidate the stored state of related optional field that we have seen before.

Probably the fix is not simple since we cannot just rely on the method name (as I did in my previous fix), we need to understand that it returns exactly the same optional field as was modified before.

I believe the proper fix also might address the issue like #151287 when we call non-const method which doesn't modify optional data, but maybe I am wrong :)

I am happy to explore the solution but need an initial guidance.

Thanks!

Compiler explorer link with my experiments

[BaLiKfromUA]

cc @jvoung because they helped me with previous "getter methods"-related problem :)

[jvoung]

Hi @BaLiKfromUA, right, the dataflow framework does not look into the bodies of methods to see what they are truly doing.

So it doesn't know that (a) getField() and setField() return a reference to the same object (b) that setField(), though non-const, does not modify the field to invalidate an earlier has_value() check on getField().

I don't think this would be an easy problem to solve well. The dataflow framework does have some "interpreter" to look into e.g., 1 level of call, but I think it was too expensive to do that always.

I think you'd need more real examples as well (at least to evaluate a solution), to really understand the types of non-const methods that are important, how big these non-const methods are, how many levels deep do you need to go to know that there is no possibility of invalidating an early check on has_value(), etc. (what if setField() called foo, which calls bar, which calls clear?), and the approach wouldn't slow things down too much.

Otherwise, there is a pretty simple workaround, which is to get the reference once (https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unchecked-optional-access.html#stabilize-function-results) instead of repeatedly?

E.g.,

void foo1(A data) {
    std::optional<int>& field = data.setField();
    if (!field.has_value()) {
        field = 42;
    }

    std::cout << field.value();
}

[BaLiKfromUA]

Thank you for the response!

I will try to make some experiments to get more data insights about this issue.

I agree about

I don't think this would be an easy problem to solve well. The dataflow framework does have some "interpreter" to look into e.g., 1 level of call, but I think it was too expensive to do that always.

But maybe we can start supporting some basic patterns as an opt-in functionality with performance impact.

Also will investigate if it's possible to automate proposed workaround from https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unchecked-optional-access.html#stabilize-function-results

Let's keep this issue opened for now in case of any updates from my side.