getRawCommentForAnyRedecl can fail to return definition comment

[HighCommander4]

While working on a clangd enhancement (#67802), @ckandeler and I have run into what I believe is a bug in ASTContext::getRawCommentForAnyRedecl(): when called on the definition of a function or class, it can fail to return the comment above the definition (i.e. incorrectly return null instead), if it was previously called on a (non-defining) declaration at a time when the definition wasn't yet added to the redecl chain.

I wrote a (failing) unit test demonstrating the issue at HighCommander4@9933075.

The bug is a regression from f31d8df, which introduced the cache CommentlessRedeclChains and related data structures. (I verified this with a local backout of the patch, which makes my unit test pass.)

@jkorous-apple and @gribozavr, as author and reviewer of the regressing patch, could you advise what an appropriate fix would be?

---

Here are some more details regarding the bug:

The failing unit test processes the following code:

void f();

// f is the best
void f() {}

with an ASTConsumer that calls ASTContext::getRawCommentForAnyRedecl() as soon as a declaration is parsed and given to the consumer.

(I assume this is a valid use of getRawCommentForAnyRedecl(), i.e. it's not the intention to require that the function only be called once the whole AST is built, otherwise I don't see what would be the purpose of having caches like CommentlessRedeclChains at all. Please let me know if I misunderstood.)

When getRawCommentForAnyRedecl() is first called, for the non-defining declaration of f(), the definition has not been parsed and added to the redecl chain yet. This results in CommentslessRedeclChains containing an entry mapping this declaration to itself here.

When the function is called a second time for the definition of f(), the redecl chain of that contains itself (the definition) as the first element, and the non-defining declaration as the second. The previously stored mapping is looked up here, and results in the loop skipping the definition here, and therefore never checking the definition for a comment.

Based on the above debugging, I don't quite understand how this caching mechanism is supposed to work.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Nathan Ridge (HighCommander4)

While working on a clangd enhancement (https://github.com//pull/67802), @ckandeler and I have run into what I believe is a bug in `ASTContext::getRawCommentForAnyRedecl()`: when called on the **definition** of a function or class, it can fail to return the comment above the definition (i.e. incorrectly return null instead), if it was previously called on a (non-defining) declaration at a time when the definition wasn't yet added to the redecl chain.

I wrote a (failing) unit test demonstrating the issue at HighCommander4@9933075.

The bug is a regression from f31d8df, which introduced the cache CommentlessRedeclChains and related data structures. (I verified this with a local backout of the patch, which makes my unit test pass.)

@jkorous-apple and @gribozavr, as author and reviewer of the regressing patch, could you advise what an appropriate fix would be?

---

Here are some more details regarding the bug:

The failing unit test processes the following code:

void f();

// f is the best
void f() {}

with an ASTConsumer that calls ASTContext::getRawCommentForAnyRedecl() as soon as a declaration is parsed and given to the consumer.

(I assume this is a valid use of getRawCommentForAnyRedecl(), i.e. it's not the intention to require that the function only be called once the whole AST is built, otherwise I don't see what would be the purpose of having caches like CommentlessRedeclChains at all. Please let me know if I misunderstood.)

When getRawCommentForAnyRedecl() is first called, for the non-defining declaration of f(), the definition has not been parsed and added to the redecl chain yet. This results in CommentslessRedeclChains containing an entry mapping this declaration to itself [here](https://searchfox.org/llvm/rev/b0ffaa79050460d724eec1b12363c439b43d5ae5/clang/lib/AST/ASTContext.cpp#464).

When the function is called a second time for the definition of f(), the redecl chain of that contains itself (the definition) as the first element, and the non-defining declaration as the second. The previously stored mapping is looked up [here](https://searchfox.org/llvm/rev/b0ffaa79050460d724eec1b12363c439b43d5ae5/clang/lib/AST/ASTContext.cpp#445), and results in the loop skipping the definition [here](https://searchfox.org/llvm/rev/b0ffaa79050460d724eec1b12363c439b43d5ae5/clang/lib/AST/ASTContext.cpp#455), and therefore never checking the definition for a comment.

Based on the above debugging, I don't quite understand how this caching mechanism is supposed to work.

[shafik]

CC @Bigcheese

[HighCommander4]

@jkorous-apple and @gribozavr, as author and reviewer of the regressing patch, could you advise what an appropriate fix would be?

I thought of a fix and submitted it with my unit test at #108475.