[clangd][Tweak] Make sure enclosing function doesnt have invalid children

[kadircet]

This is triggering crashes in our production setup, but I couldn't come up with a reduced test case.

crash is triggered in the loop:

for (const auto *S : EnclosingFunction->getBody()->children()) {
      if (SM.isBeforeInTranslationUnit(S->getSourceRange().getEnd(),
                                       ZoneRange.getEnd()))

in which S is nullptr.

[ilya-biryukov]

Having nullptr inside children() seems really weird. Should we fix those instead to never produce nullptr? Or is this something that is expected (I can come up with a few contracts where this would make sense, but that all looks really akward).

[hokein]

In D135257#3836511, @ilya-biryukov wrote:

Having nullptr inside children() seems really weird. Should we fix those instead to never produce nullptr? Or is this something that is expected (I can come up with a few contracts where this would make sense, but that all looks really akward).

I'm not too surprised to see this, given the code action is widely triggered (on broken code). It would be nice that clang never produce a nullptr, but we're not in a perfect world. And there are other places (e.g.) in the code base doing this nullptr check. So to me, this patch is an improvement making clangd more robust, it looks good from my side.

[kadircet]

In D135257#3836511, @ilya-biryukov wrote:

Having nullptr inside children() seems really weird. Should we fix those instead to never produce nullptr? Or is this something that is expected (I can come up with a few contracts where this would make sense, but that all looks really akward).

that would definitely be great, if I could find any examples that triggers this. my main hesitation with such solutions is, no matter how hard we try, due to the variety of codepaths that lead to stmt creation, either we'll miss some paths or someone in the future will introduce a new one without broken code in mind and hell will break lose again. so i personally feel like being defensive on the application side, especially when it isn't a huge performance hit, is healthier approach in the long term.

[ilya-biryukov]

I don't think it's unreasonable to protect against nullptr in individual fields. We need some value for it and null is a reasonable choice.
However, it feels completely wrong to have nullptr in collections. Those should be filtered out upon creation, if possible.
It seems perfectly fine for a collection to have less elements if some of them are invalid.

[ilya-biryukov]

I guess my question is: is there any fundamental reason why we think we need to allow nullptr children in Stmt? What are the places that actually need it?

A quick search shows there are quite a few places in our codebase (many google-internal) that don't check for nullptr and are subject to the same breakage.
It seems that having a standard primitive for iterating over only non-null children and using it in almost all of the places is appropriate. I am trying to understand what are the use-cases that have to see those null children.

[kadircet]

I wasn't claiming that there are legitimate reasons for having null children, I completely agree with your verdict on this one. The only one I can think of is cases like this, i.e. you don't want to perform some analysis if function body has invalid code in it. but i think that should rather be indicated in the container of children, rather than being inferred by traversing the children.

I am just saying that going out and fixing all the statement types that can contain sub-statements is a much bigger effort.

[ilya-biryukov]

To be clear: landing this change seems fine, I was not trying to block it, sorry if it seemed that way. I'm just trying to understand whether it would also make sense to change the defaults.
I believe we are on the same page here, it's just a matter of booking someone's time to do this.

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

const FunctionDecl *findEnclosingFunction(const Node *CommonAnc) {
   for (const Node *CurNode = CommonAnc; CurNode; CurNode = CurNode->Parent) {
     // Don't extract from lambdas
     if (CurNode->ASTNode.get<LambdaExpr>())
       return nullptr;
     if (const FunctionDecl *Func = CurNode->ASTNode.get<FunctionDecl>()) {
       // FIXME: Support extraction from templated functions.
       if (Func->isTemplated())
         return nullptr;
+      for (const auto *S : Func->getBody()->children()) {
+        // During apply phase, we perform semantic analysis (e.g. figure out
+        // what variables requires hoisting). We cannot perform those when the
+        // body has invalid statements, so fail up front.
+        if (!S)
+          return nullptr;
+      }
       return Func;
     }
   }
   return nullptr;
 }
 
 // Zone Range is the union of SourceRanges of all child Nodes in Parent since
 // all child Nodes are RootStmts