missing dangling reference warnings if lifetimebound is applied to a function declaration but not the definition

[zygoloid]

Testcase:

struct S {
    const int& x() const [[clang::lifetimebound]];
    static const int& y(const S &self [[clang::lifetimebound]]);
    int i;
};

#if DO_INLINE
inline const int& S::x() const { return i; }
inline const int& S::y(const S &self) { return self.i; }
#endif

void G() {
    auto& x = S().x();
    auto& y = S::y(S());
}

Clang warns that x and y are dangling with -UDO_INLINE but the warnings are silenced under -DDO_INLINE. It looks like these attributes are not being propagated from the declaration to the definition properly (presumably because they are type attributes rather than declaration attributes, and they don't change the canonical type).

[ilya-biryukov]

Just a drive-by comment, but may be useful if someone wants to pick this up.

I think that if the annotation is on one of the (re-)declarations (including definitions) we should just propagate it to the rest of the declarations. If a redeclaration provides inconsistent attributes, we should diagnose it.
That's how things like [[nodiscard]] work: https://gcc.godbolt.org/z/Te8GWzzKb.

[higher-performance]

I think we'd want to treat the cases slightly differently:

• For bound functions, we should only let the initial declaration be authoritative. The presence on an out-of-line definition should have no effect, and should generate a warning. The warning should be under the same exact flag (-Wdangling, etc.) and not a different one that can be separately suppressed, because this isn't a valid use case.
• For free functions, we should propagate the presence of the attribute from any declaration/definition to the initial declaration. If a subsequent declaration has the attribute when the initial one doesn't, we should generate a warning, but I think the categorization should depend on whether the differing declarations are in the same file. If they are in the same file, then we should warn as in the above scenario. However, if they are in different files, then we should either accept it silently, or generate any warnings under a different (pedantic?) flag. This is because it is useful for users to be able to add the attribute to a third-party header they have no control over. (Bonus points if "different files" means "different base name", so that the file extension can be ignored, but I'm not sure if there is precedence for this.)

[higher-performance]

I put up a partial fix in PR #107627.

[kinu]

Related (but separate) topic-- do we have an issue about how this should work with inheritance, or should we file it? E.g. if base class has a method with lifetimebound attribute, and how derived classes should be handled. In C++ parameter types should basically be invariant (the same) in C++, or could accept contravariant (e.g. it's possible that only Base class has lifetimebound)-- either way, we should clarify the behavior.

[higher-performance]

I'm pretty confident lifetimebound should look solely at the statically resolved member, and nothing else. i.e. it should never be inherited, nor should absence of it be inherited:

• Inheriting it doesn't really make sense, because the derived class (through omitting the attribute) would be providing a stronger guarantee, i.e. a lack of dependence. This is an entirely sensible thing a derived should be able to do, and is perfectly consistent with the Liskov substitution principle.

• Inheriting the absence of it (and thus invalidating its presence in an overridden member) would only makes sense if the attribute wasn't optional to begin with. If the attribute was required, then the base class would be guaranteeing independence, and thus the base class could not legally introduce a dependence. (In this case, a separate diagnosis would be justified, since it would violate LSP.) However, given that this isn't the case, the only valid interpretation is that the base class simply forgot to specify the attribute, and thus that should not affect its presence on the derived class member.

FWIW, I think this is consistent with how I'd expect [[nodiscard]] to be handled, but I haven't thought much about it. If it is inconsistent with that, though, I'd love to hear what is different.

[zygoloid]

In this case, a separate diagnosis would be justified, since it would violate LSP.

Yes, I think it'd make sense to warn if an overriding function adds a lifetimebound that was not present on the overridden function. That might be annoying if the overridden function is not within your control so you can't feasibly add the attribute to it, but I think it's probably still worth warning by default.

[zygoloid]

FWIW, I think this is consistent with how I'd expect [[nodiscard]] to be handled, but I haven't thought much about it. If it is inconsistent with that, though, I'd love to hear what is different.

Hmm. Thinking about this more, I think generally for this kind of checking, we can model a virtual function as if the base class function cast the this pointer to the derived class and called the derived class function. The derived override is then OK if that call would be OK, and if we'd warn on -- or reject -- that call, then we should warn on or reject the overrider.

That is, we should warn on this:

struct A {
  virtual int *f(int *p); 
};
struct B : A {
  int *f(int *p [[clang::lifetimebound]]); 
};

if and only if we warn on this:

int *B_f(int *p [[clang::lifetimebound]]);
int *A_f(int *p) {
  return B_f(p);
}

... which we currently don't. This is something I'd considered diagnosing previously, with some kind of "missing lifetimebound" warning for the case where a function's return value is lifetime-bound to a parameter but there's no corresponding annotation. This would give a const-like virality to these annotations, and might be something that people only want to enable as a lint check rather than as a compile-time warning (especially with -Werror).

And similarly we should warn on this:

struct A {
  virtual int f();
};
struct B : A {
  [[nodiscard]] int f();
};

if and only if we warn on this:

[[nodiscard]] int B_f();
int A_f() { return B_f(); }

... which again we currently don't. And maybe we should warn on this too -- though again, this is a weaker warning than the usual "nodiscard value discarded" warning, because this case is only potentially discarded. And again probably the right thing here would be to add a "missing nodiscard" warning in this case to suggest adding the attribute to the caller -- both in the virtual case and in the returned call case.