[clang] Wrong SourceRange returned by getNameInfo() for template instantiations of CXXDestructorDecls

[steakhal]

D64087 appears to break some downstream code that relies on the expectation summarized in this unittest:

TEST(Regression, CXXDestructorDeclsShouldHaveWellFormedRanges) {
  std::unique_ptr<ASTUnit> AST = buildASTFromCode(R"cpp(
template <typename T> struct Resource {
  ~Resource();
};
template <typename T>
Resource<T>::~Resource() {}
 
void instantiate_template() {
  Resource<int> x;
}
)cpp",
                                                  "foo.cpp");
  auto &SM = AST->getASTContext().getSourceManager();
 
  auto Matches = match(
      findAll(decl(hasDescendant(cxxDestructorDecl().bind("dtor")))),
      *AST->getASTContext().getTranslationUnitDecl(), AST->getASTContext());
 
  for (BoundNodes Match : Matches) {
    const auto *D = Match.getNodeAs<CXXDestructorDecl>("dtor");
    assert(D);
    auto Range = D->getNameInfo().getSourceRange();
    if (Range.getBegin() != Range.getEnd()) {
      ASSERT_TRUE(
          SM.isBeforeInTranslationUnit(Range.getBegin(), Range.getEnd()));
    }
  }
}

The test describes that for all CXXDestructorDecl the corresponding getNameInfo() should return a SourceRange where the begin location is before then the end location (if they were not the same of course).
This test passes before commit 256a0b2 [clang] Correct source locations for instantiations of function templates. (2023.09.12) but breaks after that.

IMO this expectation sounds right, and I believe it should hold.

[llvmbot]

@llvm/issue-subscribers-bug

Author: Balazs Benics (steakhal)

[D64087](https://reviews.llvm.org/D64087) appears to break some downstream code that relies on the expectation summarized in this unittest:

TEST(Regression, CXXDestructorDeclsShouldHaveWellFormedRanges) {
  std::unique_ptr<ASTUnit> AST = buildASTFromCode(R"cpp(
template <typename T> struct Resource {
  ~Resource();
};
template <typename T>
Resource<T>::~Resource() {}
 
void instantiate_template() {
  Resource<int> x;
}
)cpp",
                                                  "foo.cpp");
  auto &SM = AST->getASTContext().getSourceManager();
 
  auto Matches = match(
      findAll(decl(hasDescendant(cxxDestructorDecl().bind("dtor")))),
      *AST->getASTContext().getTranslationUnitDecl(), AST->getASTContext());
 
  for (BoundNodes Match : Matches) {
    const auto *D = Match.getNodeAs<CXXDestructorDecl>("dtor");
    assert(D);
    auto Range = D->getNameInfo().getSourceRange();
    if (Range.getBegin() != Range.getEnd()) {
      ASSERT_TRUE(
          SM.isBeforeInTranslationUnit(Range.getBegin(), Range.getEnd()));
    }
  }
}

The test describes that for all CXXDestructorDecl the corresponding getNameInfo() should return a SourceRange where the begin location is before then the end location (if they were not the same of course).
This test passes before commit 256a0b2 [clang] Correct source locations for instantiations of function templates. (2023.09.12) but breaks after that.

IMO this expectation sounds right, and I believe it should hold.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Balazs Benics (steakhal)

[D64087](https://reviews.llvm.org/D64087) appears to break some downstream code that relies on the expectation summarized in this unittest:

TEST(Regression, CXXDestructorDeclsShouldHaveWellFormedRanges) {
  std::unique_ptr<ASTUnit> AST = buildASTFromCode(R"cpp(
template <typename T> struct Resource {
  ~Resource();
};
template <typename T>
Resource<T>::~Resource() {}
 
void instantiate_template() {
  Resource<int> x;
}
)cpp",
                                                  "foo.cpp");
  auto &SM = AST->getASTContext().getSourceManager();
 
  auto Matches = match(
      findAll(decl(hasDescendant(cxxDestructorDecl().bind("dtor")))),
      *AST->getASTContext().getTranslationUnitDecl(), AST->getASTContext());
 
  for (BoundNodes Match : Matches) {
    const auto *D = Match.getNodeAs<CXXDestructorDecl>("dtor");
    assert(D);
    auto Range = D->getNameInfo().getSourceRange();
    if (Range.getBegin() != Range.getEnd()) {
      ASSERT_TRUE(
          SM.isBeforeInTranslationUnit(Range.getBegin(), Range.getEnd()));
    }
  }
}

The test describes that for all CXXDestructorDecl the corresponding getNameInfo() should return a SourceRange where the begin location is before then the end location (if they were not the same of course).
This test passes before commit 256a0b2 [clang] Correct source locations for instantiations of function templates. (2023.09.12) but breaks after that.

IMO this expectation sounds right, and I believe it should hold.

[tahonermann]

Thank you for the report, @steakhal, I agree that assertion should succeed. I'll address this soon.

[steakhal]

Thank you for the report, @steakhal, I agree that assertion should succeed. I'll address this soon.

Thanks for the reply. No need to rush, I'd only need this in clang-18.

[steakhal]

@tahonermann Hi, do you have news about this?

[tahonermann]

Hi @steakhal. No, I'm afraid I don't. I've been struggling to keep up with ... everything. Could you possibly check if the patches proposed in #68525 and #78716 address this issue?

[steakhal]

Hi @steakhal. No, I'm afraid I don't. I've been struggling to keep up with ... everything. Could you possibly check if the patches proposed in #68525 and #78716 address this issue?

No, neither of those two PRs fix this wrong source range.
Those aim to fix crashes. In this issue, I reported a wrong source range.

[dhoepfl]

In my local build of clang, I applied the appended patch to fix (suppress?) the problem. Feel free to use it if you think it is correct. (I cannot tell if it is correct, I do not know the code well enough.)

ASTContext.patch

[steakhal]

In my local build of clang, I applied the appended patch to fix (suppress?) the problem. Feel free to use it if you think it is correct. (I cannot tell if it is correct, I do not know the code well enough.)

ASTContext.patch

Thanks for sharing. However, it doesn't seem to resolve this issue.

[steakhal]

@tahonermann Do you think this can make it to clang-18?