[cfe-dev] [RFC] Improving import failure checking strategy inside ASTImporter

[Aleksei Sidorin via cfe-dev]

Hello,

I'd like to discuss the idea of changing the way how we check for import 
issues  in ASTImporter and ASTNodeImporter.

1. Introduction

ASTImporter is now extensively used in Cross-TU analysis and is in 
active development. However, we find (and introduce) many errors related 
to how import failures are handled now.

Currently, ASTImporter::Import() and ASTNodeImporter::Visit*() methods 
return nullptr for pointers and default value in case of conflict. And 
this approach brings many issues due to the following reasons.

1. We have to check the return result and take actions explicitly.

The common strategy of failure handling is very simple - just return. 
While it is very simple, almost all the code of ASTImporter consists of 
such explicit checks making it much harder to read.

2. Both nullptr and default value are valid by itself so we have to 
place explicit checks to check if a failure happened. Example:

Decl *To = Importer.Import(From);
if (From && !To)
   return nullptr;

This doesn't look well and it's easy to forget where From can be nullptr 
and where it is impossible.

3. Error checking is not enforced.

We have to check for import errors every time Import() is called. And it 
is extremely easy to forget placing a check after a call. This error 
pattern is so common in ASTImporter that I was going to write a CSA 
check for this.

2. Possible solutions.

The problem with enforcing checks is the most annoying one but also 
looks to be the simplest - we can just replace the return value with 
Optional or ErrorOr so clients will be enforced to perform a failure 
checking. This replacement can include two changes.

1. A number of large commits that change the return type of 
ASTNodeImporter::Visit*() methods. And I'd like to hear the comments 
from existing users that have their own improvements of ASTImporter 
because they have to change their code not touched by the commit as well 
- there be some pain.

2. Introducing an alternative method in ASTImporter that returns 
ErrorOr. So, we can migrate to the new, say, ImportOrError(), by small 
patches.

This replacement also solves the problem (2) because value and error are 
now separated.

The problem of improving readability of these checks is a bit harder. 
The most straightforward approach is to create a macro like:

#define IMPORT_PTR_INTO_VAR(VarName, Type, From) \
   Type *VarName = nullptr;                                 \
   {                                                        \
     auto MacroImportRes = Importer.Import(From);           \
     if (!MacroImportRes)                                   \
       return MacroImportRes.getError();                    \
     VarName = *MacroImportRes;                             \
   }

Same for non-pointer type. So, the final code will look like this:

ErrorOr<Expr *> 
ASTNodeImporter::VisitCXXMemberCallExpr(CXXMemberCallExpr *E) {
   IMPORT_QUAL_TYPE_INTO_VAR(T, E->getType())
   IMPORT_PTR_INTO_VAR(ToFn, Expr, E->getCallee())

   SmallVector<Expr *, 4> ToArgs(E->getNumArgs());
   IMPORT_CONTAINER(E->arguments(), ToArgs);

   return new (Importer.getToContext()) CXXMemberCallExpr(
         Importer.getToContext(), ToFn, ToArgs, T, E->getValueKind(),
         Importer.Import(E->getRParenLoc()));
}

But macros don't look clean. Unfortunately, I still haven't found a 
better option so any suggestions are very welcome.

PS. The error handling pattern we wish to get is very close to exception 
handling in C++ because usually we don't want to perform local error 
handling but centralized instead. With a possibility to just throw an 
exception and catch it in the ASTImporter entry point (returning an 
ErrorOr), error handling in ASTImporter becomes almost trivial and 
pretty clean. But we don't use exceptions in LLVM and clang and I don't 
know a similar functionality in LLVM utilities.

-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics