[core] Check for existing type before removing whitespaces

This fixes #19022

In `TClassEdit::GetNormalizedName`, the call to
`gInterpreterHelper->ExistingTypeCheck` is the one where eventually any
alternative names for the class name being looked for will be detected and used
for replacement (in `TClassTable::Check`). The alternative names for a custom
class may appear in a dictionary when the user declared their class in the
`ROOT::Meta::Selection` namespace and made it derive from the
`KeepFirstTemplateArguments` type trait.

Previously, the call to `ExistingTypeCheck` only happened after the input
demangled type name passed through the normalization logic of
`TClassEdit::TSplitType`. This led to an unfortunate situation.

The demangled type name resulting from a previous `TClassEdit::DemangleName`
call is the name representation of the typeid of the class type for the current
execution, i.e. whatever was generate by the compiler. In the case of a class
with multiple template arguments, this usually leads to a representation of the
form `A<B, C>`, i.e. where the two template arguments are separated by a
whitespace after the comma.

Crucially, this representation is also the same one used by the type system to
populate the dictionary information relative to the alternative class name for
classes where the user wants to strip one or more template arguments. In the
example just made, the class alternative name for `A<B>` will be registered as
`A<B, C>` with the whitespace in the dictionary sources.

Since the call to `ExistingTypeCheck` was happening *after* the `TSplitType`
normalization, the input class name was lacking the extra whitespace. Thus, any
search in `TClassTable::Check` would fail because it does a hash-based lookup of
the string in the map of class alternatives.

Thus, this commit introduces a second call to `ExistingTypeCheck` earlier on in
the function, before the removal of the whitespace. This ensures that the name
lookup in the type system can work correctly.

Co-authored-by: Philippe Canal <[email protected]>

Author: vepadulano

core/clingutils/res/TClingUtils.h

@@ -160,25 +160,26 @@ class TNormalizedCtxt {
 class TClingLookupHelper : public TClassEdit::TInterpreterLookupHelper {
 public:
    typedef bool (*ExistingTypeCheck_t)(const std::string &tname, std::string &result);
+   typedef bool (*TClassTableCheck_t)(const std::string &tname, std::string &result);
    typedef bool (*AutoParse_t)(const char *name);
 
 private:
    cling::Interpreter *fInterpreter;
    TNormalizedCtxt    *fNormalizedCtxt;
    ExistingTypeCheck_t fExistingTypeCheck;
+   TClassTableCheck_t fTClassTableCheck;
    AutoParse_t         fAutoParse;
    bool               *fInterpreterIsShuttingDownPtr;
    const int          *fPDebug; // debug flag, might change at runtime thus *
    bool WantDiags() const { return fPDebug && *fPDebug > 5; }
 
 public:
-   TClingLookupHelper(cling::Interpreter &interpreter, TNormalizedCtxt &normCtxt,
-                      ExistingTypeCheck_t existingTypeCheck,
-                      AutoParse_t autoParse,
-                      bool *shuttingDownPtr,
+   TClingLookupHelper(cling::Interpreter &interpreter, TNormalizedCtxt &normCtxt, ExistingTypeCheck_t existingTypeCheck,
+                      TClassTableCheck_t TClassTableCheck, AutoParse_t autoParse, bool *shuttingDownPtr,
                       const int *pgDebug = nullptr);
    virtual ~TClingLookupHelper() { /* we're not owner */ }
 
+   bool TClassTableCheck(const std::string &tname, std::string &result) override;
    bool ExistingTypeCheck(const std::string &tname, std::string &result) override;
    void GetPartiallyDesugaredName(std::string &nameLong) override;
    bool IsAlreadyPartiallyDesugaredName(const std::string &nondef, const std::string &nameLong) override;

core/clingutils/src/TClingUtils.cxx

@@ -522,17 +522,16 @@ AnnotatedRecordDecl::AnnotatedRecordDecl(long index,
 
 ////////////////////////////////////////////////////////////////////////////////
 
-TClingLookupHelper::TClingLookupHelper(cling::Interpreter &interpreter,
-                                       TNormalizedCtxt &normCtxt,
-                                       ExistingTypeCheck_t existingTypeCheck,
-                                       AutoParse_t autoParse,
-                                       bool *shuttingDownPtr,
-                                       const int* pgDebug /*= 0*/):
-   fInterpreter(&interpreter),fNormalizedCtxt(&normCtxt),
-   fExistingTypeCheck(existingTypeCheck),
-   fAutoParse(autoParse),
-   fInterpreterIsShuttingDownPtr(shuttingDownPtr),
-   fPDebug(pgDebug)
+TClingLookupHelper::TClingLookupHelper(cling::Interpreter &interpreter, TNormalizedCtxt &normCtxt,
+                                       ExistingTypeCheck_t existingTypeCheck, TClassTableCheck_t TClassTableCheck,
+                                       AutoParse_t autoParse, bool *shuttingDownPtr, const int *pgDebug /*= 0*/)
+   : fInterpreter(&interpreter),
+     fNormalizedCtxt(&normCtxt),
+     fExistingTypeCheck(existingTypeCheck),
+     fTClassTableCheck(TClassTableCheck),
+     fAutoParse(autoParse),
+     fInterpreterIsShuttingDownPtr(shuttingDownPtr),
+     fPDebug(pgDebug)
 {
 }
 
@@ -549,6 +548,17 @@ bool TClingLookupHelper::ExistingTypeCheck(const std::string &tname,
    else return false;
 }
 
+bool TClingLookupHelper::TClassTableCheck(const std::string &tname, std::string &result)
+{
+   if (tname.empty())
+      return false;
+
+   if (fTClassTableCheck)
+      return fTClassTableCheck(tname, result);
+   else
+      return false;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 
 void TClingLookupHelper::GetPartiallyDesugaredName(std::string &nameLong)

core/dictgen/src/rootcling_impl.cxx

@@ -4498,7 +4498,7 @@ int RootClingMain(int argc,
 
    // We are now ready (enough is loaded) to init the list of opaque typedefs.
    ROOT::TMetaUtils::TNormalizedCtxt normCtxt(interp.getLookupHelper());
-   ROOT::TMetaUtils::TClingLookupHelper helper(interp, normCtxt, nullptr, nullptr, nullptr);
+   ROOT::TMetaUtils::TClingLookupHelper helper(interp, normCtxt, nullptr, nullptr, nullptr, nullptr);
    TClassEdit::Init(&helper);
 
    // flags used only for the pragma parser:

core/foundation/inc/TClassEdit.h

@@ -124,6 +124,7 @@ namespace TClassEdit {
       TInterpreterLookupHelper() { }
       virtual ~TInterpreterLookupHelper();
 
+      virtual bool TClassTableCheck(const std::string &, std::string &) = 0;
       virtual bool ExistingTypeCheck(const std::string & /*tname*/,
                                      std::string & /*result*/) = 0;
       virtual void GetPartiallyDesugaredName(std::string & /*nameLong*/) = 0;

core/foundation/src/TClassEdit.cxx

@@ -904,6 +904,21 @@ void TClassEdit::GetNormalizedName(std::string &norm_name, std::string_view name
    }
 
    AtomicTypeNameHandlerRAII nameHandler(norm_name);
+   if (gInterpreterHelper) {
+      // Early check whether there is an existing type corresponding to `norm_name`
+      // It is *crucial* to run this block here, before `norm_name` gets split
+      // and reconstructed in the following lines. The reason is that we need
+      // to make string comparisons in `ExistingTypeCheck` and they will give
+      // different results if `norm_name` loses whitespaces. A notable example
+      // is when looking for registered alternate names of a custom user class
+      // present in the class dictionary.
+      std::string typeresult;
+      if (gInterpreterHelper->TClassTableCheck(norm_name, typeresult)) {
+         if (!typeresult.empty()) {
+            norm_name = typeresult;
+         }
+      }
+   }
 
    // Remove the std:: and default template argument and insert the Long64_t and change basic_string to string.
    TClassEdit::TSplitType splitname(norm_name.c_str(),(TClassEdit::EModType)(TClassEdit::kLong64 | TClassEdit::kDropStd | TClassEdit::kDropStlDefault | TClassEdit::kKeepOuterConst));

core/metacling/src/TCling.cxx

@@ -139,6 +139,7 @@ clang/LLVM technology.
 #include <fstream>
 #include <sstream>
 #include <string>
+#include <string_view>
 #include <tuple>
 #include <typeinfo>
 #include <unordered_map>
@@ -903,6 +904,36 @@ namespace{
       bool fOldDiagValue;
    };
 
+   std::string StripModifiersAndArrayBrackets(const std::string &tname)
+   {
+      std::string_view view{tname};
+
+      // Remove leading "const "
+      if (view.substr(0, 6) == "const ") {
+         view.remove_prefix(6);
+      }
+
+      // Remove trailing *, &, and array brackets
+      while (!view.empty()) {
+         char last = view.back();
+         if (last == '*' || last == '&') {
+            view.remove_suffix(1);
+         } else if (last == ']') {
+            // Remove until matching '['
+            view.remove_suffix(1); // remove ']'
+            while (!view.empty() && view.back() != '[') {
+               view.remove_suffix(1);
+            }
+            if (!view.empty() && view.back() == '[') {
+               view.remove_suffix(1); // remove '['
+            }
+         } else {
+            break;
+         }
+      }
+
+      return std::string{view};
+   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -921,26 +952,8 @@ bool TClingLookupHelper__ExistingTypeCheck(const std::string &tname,
 {
    result.clear();
 
-   unsigned long offset = 0;
-   if (strncmp(tname.c_str(), "const ", 6) == 0) {
-      offset = 6;
-   }
-   unsigned long end = tname.length();
-   while( end && (tname[end-1]=='&' || tname[end-1]=='*' || tname[end-1]==']') ) {
-      if ( tname[end-1]==']' ) {
-         --end;
-         while ( end && tname[end-1]!='[' ) --end;
-      }
-      --end;
-   }
-   std::string innerbuf;
-   const char *inner;
-   if (end != tname.length()) {
-      innerbuf = tname.substr(offset,end-offset);
-      inner = innerbuf.c_str();
-   } else {
-      inner = tname.c_str()+offset;
-   }
+   const auto innerBuf = StripModifiersAndArrayBrackets(tname);
+   const auto &inner = innerBuf.c_str();
 
    //if (strchr(tname.c_str(),'[')!=0) fprintf(stderr,"DEBUG: checking on %s vs %s %lu %lu\n",tname.c_str(),inner,offset,end);
    if (gROOT->GetListOfClasses()->FindObject(inner)
@@ -953,20 +966,22 @@ bool TClingLookupHelper__ExistingTypeCheck(const std::string &tname,
    TDataType *type = (TDataType *)typeTable->THashTable::FindObject( inner );
    if (type) {
       // This is a raw type and an already loaded typedef.
-      const char *newname = type->GetFullTypeName();
-      if (type->GetType() == kLong64_t) {
-         newname = "Long64_t";
-      } else if (type->GetType() == kULong64_t) {
-         newname = "ULong64_t";
-      }
+      const char *newname = [type] {
+         if (type->GetType() == kLong64_t)
+            return "Long64_t";
+         else if (type->GetType() == kULong64_t)
+            return "ULong64_t";
+         else
+            return type->GetFullTypeName();
+      }();
+
       if (strcmp(inner,newname) == 0) {
          return true;
       }
-      if (offset) result = "const ";
-      result += newname;
-      if ( end != tname.length() ) {
-         result += tname.substr(end,tname.length()-end);
-      }
+
+      // replace the core part of the original type name with newname
+      result = tname;
+      result.replace(result.find(innerBuf), innerBuf.length(), newname);
       if (result == tname) result.clear();
       return true;
    }
@@ -1010,6 +1025,20 @@ bool TClingLookupHelper__ExistingTypeCheck(const std::string &tname,
    return false;
 }
 
+bool TClingLookupHelper__TClassTableCheck(const std::string &tname, std::string &result)
+{
+   result.clear();
+
+   const auto innerBuf = StripModifiersAndArrayBrackets(tname);
+   const auto &inner = innerBuf.c_str();
+
+   if (gROOT->GetListOfClasses()->FindObject(inner) || TClassTable::Check(inner, result)) {
+      // This is a known class.
+      return true;
+   }
+
+   return false;
+}
 ////////////////////////////////////////////////////////////////////////////////
 
 TCling::TUniqueString::TUniqueString(Long64_t size)
@@ -1586,10 +1615,9 @@ TCling::TCling(const char *name, const char *title, const char* const argv[], vo
 
    // We are now ready (enough is loaded) to init the list of opaque typedefs.
    fNormalizedCtxt = new ROOT::TMetaUtils::TNormalizedCtxt(fInterpreter->getLookupHelper());
-   fLookupHelper = new ROOT::TMetaUtils::TClingLookupHelper(*fInterpreter, *fNormalizedCtxt,
-                                                            TClingLookupHelper__ExistingTypeCheck,
-                                                            TClingLookupHelper__AutoParse,
-                                                            &fIsShuttingDown);
+   fLookupHelper = new ROOT::TMetaUtils::TClingLookupHelper(
+      *fInterpreter, *fNormalizedCtxt, TClingLookupHelper__ExistingTypeCheck, TClingLookupHelper__TClassTableCheck,
+      TClingLookupHelper__AutoParse, &fIsShuttingDown);
    TClassEdit::Init(fLookupHelper);
 
    // Disallow auto-parsing in rootcling