The discussions on the bug did not provide further insight (until now).

Nice! Having no false-positives is most important because this is enabled by default.

I'm seeing the same issue Not all CodeGen sections are inside any Frontend section! with python 3.7.1. Json:

Yes, it means that the automatic inference of Pointer/Owner from base classes has the same issues as all of those automatic inferences: Once can construct a case where they are not true.
So for having no false-positives at all, we should avoid this inference by default and not consider MutableArrayRef to be a gsl::Pointer.
Instead, we disable the analysis when it sees an unannotated class (like here, MutableArrayRef).

When I understand you correctly, you are thinking about
the cases

OwningArrayRef getRef();

Generally, ArrayRef is also a candidate. But there we have the complication that OwningArrayRef indirectly inherits from ArrayRef. One is a gsl::Owner, the other a gsl::Pointer, and we
need to make sure that we handle it correctly when OwningArrayRef is accessed through its base class which is then seen as a Pointer.
I would thus defer this part to a later PR.

Also it feels a bit weird to change the ownership semantics in a derived class, I bet that would violate the Liskov substitution principle.

And then we see that llvm::OwningArrayRef inherits from llvm::ArrayRef ... But maybe this direction (strengthening a Pointer into an Owner)
is okay. We'll need to go though the call rules, and see if something breaks when the caller passes an Owner to a function that takes a base class (i.e. Pointer).
My initial reaction is that this should work in general. An Owner is like a Pointer that points to {static}.

Yes, for libc++ we could add the annotations to its source code, and eventually this would be the cleaner solution.
Currently, that is neither sufficient (because one could use libstdc++, MSVC or an older libc++ version) nor necessary (the inference we need for libstdc++ / MSVC also works for libc++),
so it's currently low on our priority list.

In the false-positive example, after the DerivedToBase, we see a constructor call which I think is the copy constructor.

1. We should consider MutableArrayRef to be a gsl::Pointer according to the paper, because it publicly derives from one.
2. Also in the paper, the copy constructor does should copies the pset of the argument instead of making the pointer point at the argument.

What do you think?

This change might be the cause for the false-positive in https://github.com/mgehre/llvm-project/issues/45. Could you check?

I now add the attributes to all redeclarations to make the logic a bit easier to follow.

• Put OwnerAttr/PointerAttr on all redeclarations
• clang/lib/Sema/SemaAttr.cpp: Use Attribute::CreateImplicit as requested in review

I struggle myself to fully understand the motivation behind the bug report. This is certainly not code that I would write.
So I don't really care about that kind of code and I didn't want to be in the way of other people's way of writing C++.

I now start to wonder if we should instead put the attribute on all declarations (instead of just the canonical). Later redeclarations automatically inherit the attributes.
This would make both the checking easier (just check any declaration, no need to obtain the canonical first), and remove the special
case for adding to the definition of templated record for ClassTemplateDecls.

Add tests for rvalue-ref overloads

Fix commit

• Fix crash

• Add missing check

• Fix comments
• Add Pointer via typedef on ClassTemplateDecl

I will include your latest comments into D64448

Thank you very much for dedicating your time for the review. It improved the code a lot!

• Fix all comments

• Diagnose unions; improve other diagnostics; fix all comments

Upstreaming of this checker will happen with D63954 and following revisions.

In D64448#1595017, @lebedev.ri wrote:

Just passing-by thought:
These attributes that are being added implicitly, it will be possible to differentiate
whether an attribute was actually present in the code, or was added implicitly,
say for clang-tidy check purposes?
Is there some 'implicit' bit on these, or they have a location not in the source buffer?

Thanks for the question! The implicitly added attributes have an invalid source location.

• Remove type traits

This should fix all open comments.

• Merge branch 'lifetime-categories' into hardcode-lifetime-categories; Split test; Move test to AST dump; fix for comments;

I think that I have addressed all open comments.

• Disallow reference and array types as DerefType
• Add example to documentation.

Thank you for the review, Aaron!

• Don't crash when pretty-printing an Attribute with optional type argument
• Don't crash AST dump when Owner/Pointer has no Type argument
• gsl::Owner/gsl::Pointer: Make DerefType parameter optional
• Unify type and non-type attribute parsing; don't add OwnerAttr/PointerAttr twice
• Improve documentation

In D64448#1581771, @gribozavr wrote:

In D64448#1581719, @mgehre wrote:

In D64448#1577779, @gribozavr wrote:

I don't know how various versions of libstdc++ differ.

The current implementation passed the (partial) test case
https://github.com/mgehre/llvm-project/blob/lifetime-ci/lifetime-attr-test.cpp
for different standard libraries, namely libc++ 7.1.0, libc++ 8.0.1rc2, libstdc++ 4.6.4, libstdc++ 4.8.5,
libstdc++ 4.9.4, libstdc++ 5.4.0, libstdc++ 6.5.0, libstdc++ 7.3.0,
libstdc++ 8.3.0 and libstdc++ 9.1.0.

Yes, I saw the testcase -- but how do different libstdc++ versions differ?

I didn't know whether they would differ, but the test tells me that they don't differ significantly (i.e. in introducing new techniques).
Would you like me to test with other standard libraries (besides MSVC, which I already planned)?

• Implement comments

This should address all remaining comments.

Implement comments

In D64448#1577779, @gribozavr wrote:

For example, libc++ wraps everything in std in an inline namespace.

I believed that I had written a test for inline namespaces, but seems that I shouldn't be working in the middle of the night.
I will add one.

Move checks into AST matchers

• Address review comments

Friendly ping. Are there outstanding concerns that I could address?

• Make the list of attribute properties more consistent.

• Add newline at end of file

I understand that putting types on forward declared header (standard libraries / third party libraries) is not a approach we should favor.
Thank you for your good arguments.
API notes seems like a really good approach for this (I would like to help with the upstreaming), and in the meanwhile
I will work on hard-coding attributes for standard library types in clang for a follow-up PR. I think we could have them always attached (I will measure performance),
so that -Wlifetime will not change the AST.

• Address more review comments.
• git-clang-format

Address some review comments.