Skip to content

[clang][ASTImporter] Fix import of template parameter default values. #100100

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
Jul 30, 2024
Merged

[clang][ASTImporter] Fix import of template parameter default values. #100100

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
e4440b8
[clang][ASTImporter] Fix import of template parameter default values.
balazske Jul 23, 2024
24204af
added explanation comment
balazske Jul 24, 2024
76b81f4
Merge branch 'main' into astimporter_templparmdecldefaultinherited
balazske Jul 29, 2024
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
57 changes: 57 additions & 0 deletions clang/lib/AST/ASTImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,54 @@ namespace clang {
Params, Importer.getToContext().getTranslationUnitDecl());
}

template <typename TemplateParmDeclT>
void tryUpdateTemplateParmDeclInheritedFrom(NamedDecl *RecentParm,
NamedDecl *NewParm) {
if (auto *ParmT = dyn_cast<TemplateParmDeclT>(RecentParm)) {
if (ParmT->hasDefaultArgument()) {
auto *P = cast<TemplateParmDeclT>(NewParm);
P->removeDefaultArgument();
P->setInheritedDefaultArgument(Importer.ToContext, ParmT);
}
}
}

// Update the parameter list `NewParams` of a template declaration
// by "inheriting" default argument values from `RecentParams`,
// which is the parameter list of an earlier declaration of the
// same template. (Note that "inheriting" default argument values
// is not related to object-oriented inheritance.)
//
// In the clang AST template parameters (NonTypeTemplateParmDec,
// TemplateTypeParmDecl, TemplateTemplateParmDecl) have a reference to the
// default value, if one is specified at the first declaration. The default
// value can be specified only once. The template parameters of the
// following declarations have a reference to the original default value
// through the "inherited" value. This value should be set for all imported
// template parameters that have a previous declaration (also a previous
// template declaration).
//
// In the `Visit*ParmDecl` functions the default value of these template
// arguments is always imported. At that location the previous declaration
// is not easily accessible, it is not possible to call
// `setInheritedDefaultArgument` at that place.
// `updateTemplateParametersInheritedFrom` is called later when the already
// imported default value is erased and changed to "inherited".
// It is important to change the mode to "inherited" otherwise false
// structural in-equivalences could be detected.
void updateTemplateParametersInheritedFrom(
Copy link
Contributor

@NagyDonat NagyDonat Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
void updateTemplateParametersInheritedFrom(
/// Update the parameter list `NewParams` of a template declaration
/// by "inheriting" default argument values from `RecentParams`,
/// which is the parameter list of an earlier declaration of the
/// same template. (Note that "inheriting" default argument values
/// this way is completely unrelated to the usual object-oriented
/// "inheritance" relationship between classes.)
///
/// Note that the standard specifies that the same parameter cannot
/// be given default arguments twice in the same scope; but in our
/// case the parameter list `NewParams` is freshly imported from a
/// different TU, so it's possible that both `RecentParams` and
/// `NewParams` specify a default argument for the same parameter.
/// In this case, the code will use the default argument taken from
/// `RecentParams`.
/// FIXME: Report an error if the earlier and the imported template
/// declarations specify different default arguments.
void updateTemplateParametersInheritedFrom(

I felt that it is important to explain that the "Inherited" within the name of this function is unrelated to the usual object-oriented "inheritance" between classes; and as I started to write this comment, I also ended up documenting the goals of this function. (I hope that I was correct -- please fix this comment block if I misunderstood something!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment was added but different because the case is not exactly as described here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarifications!

const TemplateParameterList &RecentParams,
TemplateParameterList &NewParams) {
for (auto [Idx, Param] : enumerate(RecentParams)) {
tryUpdateTemplateParmDeclInheritedFrom<NonTypeTemplateParmDecl>(
Param, NewParams.getParam(Idx));
tryUpdateTemplateParmDeclInheritedFrom<TemplateTypeParmDecl>(
Param, NewParams.getParam(Idx));
tryUpdateTemplateParmDeclInheritedFrom<TemplateTemplateParmDecl>(
Param, NewParams.getParam(Idx));
}
}

public:
explicit ASTNodeImporter(ASTImporter &Importer) : Importer(Importer) {}

Expand Down Expand Up @@ -6132,6 +6180,9 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateDecl(ClassTemplateDecl *D) {
}

D2->setPreviousDecl(Recent);

updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
**TemplateParamsOrErr);
}

return D2;
Expand Down Expand Up @@ -6446,6 +6497,9 @@ ExpectedDecl ASTNodeImporter::VisitVarTemplateDecl(VarTemplateDecl *D) {
ToTemplated->setPreviousDecl(PrevTemplated);
}
ToVarTD->setPreviousDecl(Recent);

updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
**TemplateParamsOrErr);
}

return ToVarTD;
Expand Down Expand Up @@ -6718,6 +6772,9 @@ ASTNodeImporter::VisitFunctionTemplateDecl(FunctionTemplateDecl *D) {
TemplatedFD->setPreviousDecl(PrevTemplated);
}
ToFunc->setPreviousDecl(Recent);

updateTemplateParametersInheritedFrom(*(Recent->getTemplateParameters()),
*Params);
}

return ToFunc;
Expand Down
125 changes: 125 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 @@ -9797,6 +9797,128 @@ TEST_P(ASTImporterOptionSpecificTestBase, ImportMultipleAnonymousEnumDecls) {
ASSERT_NE(ToEnumDeclA, ToEnumDeclB);
}

struct ImportTemplateParmDeclDefaultValue
: public ASTImporterOptionSpecificTestBase {
protected:
void checkTemplateParams(RedeclarableTemplateDecl *D) {
auto *CanD = cast<RedeclarableTemplateDecl>(D->getCanonicalDecl());
auto *CanNonTypeP = cast<NonTypeTemplateParmDecl>(
CanD->getTemplateParameters()->getParam(0));
auto *CanTypeP =
cast<TemplateTypeParmDecl>(CanD->getTemplateParameters()->getParam(1));
auto *CanTemplateP = cast<TemplateTemplateParmDecl>(
CanD->getTemplateParameters()->getParam(2));
EXPECT_FALSE(CanNonTypeP->getDefaultArgStorage().isInherited());
EXPECT_FALSE(CanTypeP->getDefaultArgStorage().isInherited());
EXPECT_FALSE(CanTemplateP->getDefaultArgStorage().isInherited());
for (Decl *Redecl : D->redecls()) {
auto *ReD = cast<RedeclarableTemplateDecl>(Redecl);
if (ReD != CanD) {
auto *NonTypeP = cast<NonTypeTemplateParmDecl>(
ReD->getTemplateParameters()->getParam(0));
auto *TypeP = cast<TemplateTypeParmDecl>(
ReD->getTemplateParameters()->getParam(1));
auto *TemplateP = cast<TemplateTemplateParmDecl>(
ReD->getTemplateParameters()->getParam(2));
EXPECT_TRUE(NonTypeP->getDefaultArgStorage().isInherited());
EXPECT_TRUE(TypeP->getDefaultArgStorage().isInherited());
EXPECT_TRUE(TemplateP->getDefaultArgStorage().isInherited());
EXPECT_EQ(NonTypeP->getDefaultArgStorage().getInheritedFrom(),
CanNonTypeP);
EXPECT_EQ(TypeP->getDefaultArgStorage().getInheritedFrom(), CanTypeP);
EXPECT_EQ(TemplateP->getDefaultArgStorage().getInheritedFrom(),
CanTemplateP);
}
}
}

void testImport(RedeclarableTemplateDecl *FromD) {
RedeclarableTemplateDecl *ToD = Import(FromD, Lang_CXX14);
checkTemplateParams(ToD);
}

const char *CodeFunction =
R"(
template <class> struct X;

template <int A = 2, typename B = int, template<class> class C = X>
void f();
template <int A, typename B, template<class> class C>
void f();
template <int A, typename B, template<class> class C>
void f() {}
)";

const char *CodeClass =
R"(
template <class> struct X;

template <int A = 2, typename B = int, template<class> class C = X>
struct S;
template <int A, typename B, template<class> class C>
struct S;
template <int A, typename B, template<class> class C>
struct S {};
)";

const char *CodeVar =
R"(
template <class> struct X;

template <int A = 2, typename B = int, template<class> class C = X>
extern int V;
template <int A, typename B, template<class> class C>
extern int V;
template <int A, typename B, template<class> class C>
int V = A;
)";
};

TEST_P(ImportTemplateParmDeclDefaultValue, ImportFunctionTemplate) {
Decl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
auto *FromLastD = LastDeclMatcher<FunctionTemplateDecl>().match(
FromTU, functionTemplateDecl(hasName("f")));
testImport(FromLastD);
}

TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingFunctionTemplate) {
getToTuDecl(CodeFunction, Lang_CXX14);
Decl *FromTU = getTuDecl(CodeFunction, Lang_CXX14);
auto *FromLastD = LastDeclMatcher<FunctionTemplateDecl>().match(
FromTU, functionTemplateDecl(hasName("f")));
testImport(FromLastD);
}

TEST_P(ImportTemplateParmDeclDefaultValue, ImportClassTemplate) {
Decl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
auto *FromLastD = LastDeclMatcher<ClassTemplateDecl>().match(
FromTU, classTemplateDecl(hasName("S")));
testImport(FromLastD);
}

TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingClassTemplate) {
getToTuDecl(CodeClass, Lang_CXX14);
Decl *FromTU = getTuDecl(CodeClass, Lang_CXX14);
auto *FromLastD = LastDeclMatcher<ClassTemplateDecl>().match(
FromTU, classTemplateDecl(hasName("S")));
testImport(FromLastD);
}

TEST_P(ImportTemplateParmDeclDefaultValue, ImportVarTemplate) {
Decl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
auto *FromLastD = LastDeclMatcher<VarTemplateDecl>().match(
FromTU, varTemplateDecl(hasName("V")));
testImport(FromLastD);
}

TEST_P(ImportTemplateParmDeclDefaultValue, ImportExistingVarTemplate) {
getToTuDecl(CodeVar, Lang_CXX14);
Decl *FromTU = getTuDecl(CodeVar, Lang_CXX14);
auto *FromLastD = LastDeclMatcher<VarTemplateDecl>().match(
FromTU, varTemplateDecl(hasName("V")));
testImport(FromLastD);
}

INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
DefaultTestValuesForRunOptions);

Expand Down Expand Up @@ -9880,6 +10002,9 @@ INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportInjectedClassNameType,
INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportMatrixType,
DefaultTestValuesForRunOptions);

INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportTemplateParmDeclDefaultValue,
DefaultTestValuesForRunOptions);

// FIXME: Make ImportOpenCLPipe test work.
// INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportOpenCLPipe,
// DefaultTestValuesForRunOptions);
Expand Down
Loading