Page MenuHomePhabricator

[ASTImporter] Fix name conflict handling
AcceptedPublic

Authored by martong on Mar 22 2019, 5:47 AM.

Details

Summary

There are numorous flaws about the name conflict handling, this patch
attempts fixes them. Changes in details:

  • HandleNameConflict return with a false DeclarationName

Hitherto we effectively never returned with a NameConflict error, even
if the preceding StructuralMatch indicated a conflict.
Because we just simply returned with the parameter Name in
HandleNameConflict and that name is almost always true when converted to
bool.

  • Add tests which indicate wrong NameConflict handling
  • Add to ConflictingDecls only if decl kind is different

Note, we might not indicate an ODR error when there is an existing record decl
and a enum is imported with same name. But there are other cases. E.g. think
about the case when we import a FunctionTemplateDecl with name f and we found a
simple FunctionDecl with name f. They overload. Or in case of a
ClassTemplateDecl and CXXRecordDecl, the CXXRecordDecl could be the 'templated'
class, so it would be false to report error. So I think we should report a
name conflict error only when we are 100% sure of that. That is why I think it
should be a general pattern to report the error only if the kind is the same.

  • Fix failing ctu test with EnumConstandDecl

In ctu-main.c we have the enum class 'A' which brings in the enum
constant 'x' with value 0 into the global namespace.
In ctu-other.c we had the enum class 'B' which brought in the same name
('x') as an enum constant but with a different enum value (42). This is clearly
an ODR violation in the global namespace. The solution was to rename the
second enum constant.

Event Timeline

martong created this revision.Mar 22 2019, 5:47 AM

Hi Gabor,

Thank you for addressing the problem!

lib/AST/ASTImporter.cpp
2256 ↗(On Diff #191864)

if body is surrounded by braces, so it's better to surround else too.

2260 ↗(On Diff #191864)

Do we push the same decl twice?

8532 ↗(On Diff #191864)

Empty DeclarationName can be valid sometimes. Should we return ErrorOr<DeclarationName> instead? This can also simplify caller code a bit.

martong marked 5 inline comments as done.Mar 26 2019, 5:33 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2260 ↗(On Diff #191864)

Uhh, thanks, that is rebase error I've made.

8532 ↗(On Diff #191864)

That's a good idea, thanks.
Though, I'd use Expected<T> rather, that would be more consistent with the rest of the code.

martong updated this revision to Diff 192263.Mar 26 2019, 5:34 AM
martong marked 2 inline comments as done.
  • Add braces around else
  • Remove falsely duplicated push_back
  • Use Expected<T> in HandleNameConflict

Thanks for the fixes! I'd like to clarify one moment, however.

lib/AST/ASTImporter.cpp
2154 ↗(On Diff #192263)

Should we write else Name = *NameOrError?

martong marked 2 inline comments as done.Apr 2 2019, 2:11 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2154 ↗(On Diff #192263)

Atm, we implement the simplest strategy for name conflict handling: we just return with the error.
However, on a long term we should use the returned name indeed. I mean when we really do implement a renaming strategy via HandleNameConflict.

Also, for that we'd have to double check that the Name is indeed used when we create the AST node. So I'd rather leave this else branch up to that point. Hopefully, by that time we'll have unittests which would exercise this else branch, now we just don't have any.

martong updated this revision to Diff 194527.Apr 10 2019, 8:37 AM
martong marked an inline comment as done.
  • Put back the call to GetOriginalDecl
martong edited the summary of this revision. (Show Details)Apr 10 2019, 8:38 AM
a_sidorin accepted this revision.May 4 2019, 2:56 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 4 2019, 2:56 AM
martong updated this revision to Diff 211312.Tue, Jul 23, 9:01 AM
  • Rebase to master

First round of review.

clang/lib/AST/ASTImporter.cpp
2691

Name -> SearchName

clang/unittests/AST/ASTImporterTest.cpp
2392

What about tests for name conflicts for:

NamespaceDecl
TypedefNameDecl
TypeAliasTemplateDecl
EnumConstantDecl
RecordDecl
VarDecl

Who were also modified above.

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

martong marked an inline comment as done.Fri, Aug 16, 3:23 AM

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

Hey Shafik, I was busy lately with some non-ASTImporter related tasks, but just recently I could drive myself back to it.
I am planning to add the test cases in the following work days, but the latest by the end of the next week.

martong updated this revision to Diff 215579.Fri, Aug 16, 6:18 AM
  • Name -> SearchName
  • Add tests for conflicting declarations
martong marked 2 inline comments as done.Fri, Aug 16, 6:20 AM
martong added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
2392

I added several new tests with a new test suite ConflictingDeclsTest, they cover all the modifications in ASTImporter.cpp except for VarTemplateDecls.
In case of VarTemplateDecls we don't have a proper structural eq check implemented yet, so I disabled that test.