Skip to content

[clang][ASTImporter] Fix import of variable template redeclarations. #72841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 15, 2024
Merged

[clang][ASTImporter] Fix import of variable template redeclarations. #72841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99d6169
[clang][ASTImporter] Fix import of variable template redeclarations.
balazske Nov 20, 2023
87b62c7
removed changes related to VarTemplateSpecializationDecl
balazske Nov 22, 2023
1edb738
fixed wrong test code
balazske Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions clang/lib/AST/ASTImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6245,17 +6245,21 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
D->getTemplatedDecl()))
continue;
if (IsStructuralMatch(D, FoundTemplate)) {
// The Decl in the "From" context has a definition, but in the
// "To" context we already have a definition.
// FIXME Check for ODR error if the two definitions have
// different initializers?
VarTemplateDecl *FoundDef = getTemplateDefinition(FoundTemplate);
if (D->isThisDeclarationADefinition() && FoundDef)
// FIXME Check for ODR error if the two definitions have
// different initializers?
return Importer.MapImported(D, FoundDef);
if (FoundTemplate->getDeclContext()->isRecord() &&
D->getDeclContext()->isRecord())
return Importer.MapImported(D, FoundTemplate);

if (D->getDeclContext()->isRecord()) {
assert(FoundTemplate->getDeclContext()->isRecord() &&
"Member variable template imported as non-member, "
"inconsistent imported AST?");
Comment on lines +6252 to +6254
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this assert always valid? I thought about this for some time and I couldn't rule out that this would crash on some tricky code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing "To" AST can be somehow invalid or declarations can be in wrong scope, because previous wrong AST imports and structural equivalence problems, or here a wrong declaration may be found. This assertion looks to check for such problems (check FoundTemplate->getDeclContext()->isRecord() if D->getDeclContext()->isRecord()).

if (FoundDef)
return Importer.MapImported(D, FoundDef);
if (!D->isThisDeclarationADefinition())
return Importer.MapImported(D, FoundTemplate);
} else {
if (FoundDef && D->isThisDeclarationADefinition())
return Importer.MapImported(D, FoundDef);
}
FoundByLookup = FoundTemplate;
break;
}
Expand Down
53 changes: 53 additions & 0 deletions clang/unittests/AST/ASTImporterTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5050,6 +5050,59 @@ TEST_P(ImportFriendClasses, RecordVarTemplateDecl) {
EXPECT_EQ(ToTUX, ToX);
}

TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateDeclConflict) {
getToTuDecl(
R"(
template <class U>
constexpr int X = 1;
)",
Lang_CXX14);

Decl *FromTU = getTuDecl(
R"(
template <class U>
constexpr int X = 2;
)",
Lang_CXX14, "input1.cc");
auto *FromX = FirstDeclMatcher<VarTemplateDecl>().match(
FromTU, varTemplateDecl(hasName("X")));
auto *ToX = Import(FromX, Lang_CXX11);
// FIXME: This import should fail.
EXPECT_TRUE(ToX);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If import X should fail, What about this case in ASTImporterGenericRedeclTest.cpp

struct VariableTemplate {
  using DeclTy = VarTemplateDecl;
  static constexpr auto *Prototype = "template <class T> extern T X;";
  static constexpr auto *Definition =
      R"(
      template <class T> T X;
      template <> int X<int>;
      )";
  // There is no matcher for varTemplateDecl so use a work-around.
  BindableMatcher<Decl> getPattern() {
    return namedDecl(hasName("X"), unless(isImplicit()),
                     has(templateTypeParmDecl()));
  }
};

Storage of X in Prototype and Definition is different, it should fail when imported.
I added structural equivalence of VarTemplateDecl and makes this import failed in VarTemplateDeclConflict. is it also need to fix the testcase in ASTImporterGenericRedeclTest.cpp?

static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                     VarTemplateDecl *D1,
                                     VarTemplateDecl *D2) {
  if (!IsStructurallyEquivalent(Context, D1->getDeclName(), D2->getDeclName()))
    return false;

  // Check the templated declaration.
  if (!IsStructurallyEquivalent(Context, D1->getTemplatedDecl(),
                                D2->getTemplatedDecl()))
    return false;

  // Check template parameters.
  return IsStructurallyEquivalent(Context, D1->getTemplateParameters(),
                                  D2->getTemplateParameters());
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import should not fail (it is no error to compile code with a variable template that is declared as extern and has a non-extern definition, and the "extern" part could be in an include file), and the same does not fail with non-template variables. This case should be checked and the structural equivalence of variable templates is missing now, but that change could go into a new pull request.

}

TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateStaticDefinition) {
Decl *ToTU = getToTuDecl(
R"(
struct A {
template <class U>
static int X;
};
)",
Lang_CXX14);
auto *ToX = FirstDeclMatcher<VarTemplateDecl>().match(
ToTU, varTemplateDecl(hasName("X")));
ASSERT_FALSE(ToX->isThisDeclarationADefinition());

Decl *FromTU = getTuDecl(
R"(
struct A {
template <class U>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to drop the class V here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this code was incorrect. It is still a bug that the test did not fail, my next planned fixes should improve this situation.

static int X;
};
template <class U>
int A::X = 2;
)",
Lang_CXX14, "input1.cc");
auto *FromXDef = LastDeclMatcher<VarTemplateDecl>().match(
FromTU, varTemplateDecl(hasName("X")));
ASSERT_TRUE(FromXDef->isThisDeclarationADefinition());
auto *ToXDef = Import(FromXDef, Lang_CXX14);
EXPECT_TRUE(ToXDef);
EXPECT_TRUE(ToXDef->isThisDeclarationADefinition());
EXPECT_EQ(ToXDef->getPreviousDecl(), ToX);
}

TEST_P(ASTImporterOptionSpecificTestBase, VarTemplateParameterDeclContext) {
constexpr auto Code =
R"(
Expand Down