Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class.

[clayborg]

Summary

Objective C may or may not be enabled when running expressions. There was code that was no allowing "Class" or "id" to be looked up in expressions, but it was preventing these names from being able to be looked up in a namespace or class context. This fixes the issue by passing the namespace_decl along and testing it to make sure we only stop ones at the root namespace level.

[aprantl]

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

582

For these tiny strings a StringRef == comparison is going to be more efficient than constructing and storing a pointer to a ConstString.

lldb/test/API/commands/expression/ignore/TestIgnoreName.py

37

assertEqual(expr_result.GetValue(), None, ...) is better here, because it will print the result of expr_result.GetValue() in the failure case.

39

same here

[shafik]

You also mentioned the id case failing as well. We should add that case to the test as well.

[teemperor]

LGTM minus some stylistic changes.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

582

There is no need for StringRef as ConstString allows direct comparison against string literals, so this works and is much faster: name == "id" || name == "Class"

lldb/test/API/commands/expression/ignore/TestIgnoreName.py

7

Left over from the original test case (TestCallUserAnonTypedef.py)

33

I don't think we usually check the error, but only if target is valid in any other test. So this whole test can just be this (at least after D77197 has landed):

def test(self):                                                             
    """Test that we can evaluate an expression that finds something inside  
       a namespace that uses an Objective C keyword.                        
    """                                                                     
    self.build()                                                            
    target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))          
    self.assertTrue(target, VALID_TARGET)                                   
    self.expect_expr("a::Class x; x", result_type="a::Class")

lldb/test/API/commands/expression/ignore/main.cpp

8

I think new test files should follow LLVM code style.

[clayborg]

Fixed patch reviewer suggestions and added a test for "id".

[teemperor]

I think this review got kind of stuck. I think the only missing thing is simplifying the self.dbg.CreateTarget call in the test, but otherwise this looks good to me. I'm just going to accept this as the bit of cleanup doesn't need another revision.

(Sorry for the delay, I kept clicking on this review and just saw unaddressed comments so I thought this wasn't done yet).

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h

 //===-- ClangASTSource.h ----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
protected:
   CompilerType GuardedCopyType(const CompilerType &src_type);
 
 public:
   /// Returns true if a name should be ignored by name lookup.
   ///
   /// \param[in] name
   ///     The name to be considered.
   ///
+  /// \param[in] namespace_decl
+  ///     The namespace decl that this name is being searched. This can be
+  ///     invalid if we are searching the root namespace.
+  ///
   /// \param[in] ignore_all_dollar_names
   ///     True if $-names of all sorts should be ignored.
   ///
   /// \return
   ///     True if the name is one of a class of names that are ignored by
   ///     global lookup for performance reasons.
-  bool IgnoreName(const ConstString name, bool ignore_all_dollar_names);
+  bool IgnoreName(const ConstString name,
+                  const CompilerDeclContext &namespace_decl,
+                  bool ignore_all_dollar_names);
 
 public:
   /// Copies a single Decl into the parser's AST context.
   ///
   /// \param[in] src_decl
   ///     The Decl to copy.
   ///
   /// \return

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

 //===-- ClangASTSource.cpp ------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
void ClangASTSource::FindExternalVisibleDecls(NameSearchContext &context) {
   }
 }
 
 clang::Sema *ClangASTSource::getSema() {
   return m_clang_ast_context->getSema();
 }
 
 bool ClangASTSource::IgnoreName(const ConstString name,
+                                const CompilerDeclContext &namespace_decl,
                                 bool ignore_all_dollar_names) {
-  static const ConstString id_name("id");
-  static const ConstString Class_name("Class");
-
-  if (m_ast_context->getLangOpts().ObjC)
-    if (name == id_name || name == Class_name)
+  if (m_ast_context->getLangOpts().ObjC) {
+    if (name == "id" || name == "Class") {
+      // Only disallow using "id" and "Class" if we are searching from the root
+      // namespace or translation unit level. If namespace_decl is valid, then
+      // this is not the root namespace.
+      if (namespace_decl.IsValid())
+        return false;
+      LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS),
+               "  CAS::IgnoreName(name=\"{0}\") Ignoring reserved keryword",
+               name);
       return true;
+    }
+  }
 
   StringRef name_string_ref = name.GetStringRef();
 
   // The ClangASTSource is not responsible for finding $-names.
   return name_string_ref.empty() ||
          (ignore_all_dollar_names && name_string_ref.startswith("$")) ||
          name_string_ref.startswith("_$");
 }
 
 void ClangASTSource::FindExternalVisibleDecls(
     NameSearchContext &context, lldb::ModuleSP module_sp,
     CompilerDeclContext &namespace_decl) {
   assert(m_ast_context);
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   SymbolContextList sc_list;
 
   const ConstString name(context.m_decl_name.getAsString().c_str());
-  if (IgnoreName(name, true))
+  if (IgnoreName(name, namespace_decl, true))
     return;
 
   if (!m_target)
     return;
 
   FillNamespaceMap(context, module_sp, namespace_decl);
 
   if (context.m_found_type)
if (!context.m_found_type) {
     FindDeclInObjCRuntime(context, name);
   }
 }
 
 void ClangASTSource::FillNamespaceMap(
     NameSearchContext &context, lldb::ModuleSP module_sp,
     const CompilerDeclContext &namespace_decl) {
   const ConstString name(context.m_decl_name.getAsString().c_str());
-  if (IgnoreName(name, true))
+  if (IgnoreName(name, namespace_decl, true))
     return;
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   if (module_sp && namespace_decl) {
     CompilerDeclContext found_namespace_decl;
 
     if (SymbolFile *symbol_file = module_sp->GetSymbolFile()) {

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

 //===-- ClangExpressionDeclMap.cpp ----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 void ClangExpressionDeclMap::FindExternalVisibleDecls(
     NameSearchContext &context, lldb::ModuleSP module_sp,
     const CompilerDeclContext &namespace_decl) {
   assert(m_ast_context);
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   const ConstString name(context.m_decl_name.getAsString().c_str());
-  if (IgnoreName(name, false))
+  if (IgnoreName(name, namespace_decl, false))
     return;
 
   // Only look for functions by name out in our symbols if the function doesn't
   // start with our phony prefix of '$'
 
   Target *target = nullptr;
   StackFrame *frame = nullptr;
   SymbolContext sym_ctx;

lldb/test/API/commands/expression/ignore/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/commands/expression/ignore/TestIgnoreName.py

+"""
+Test calling user an expression that uses an Objective C keyword "id" or
+"Class" when this identifier is inside of a namespace, class or structure.
+ClangASTSource::FindExternalVisibleDecls(...) might try to find one of these
+keywords inside of a non root decl context and this should succeed.
+"""
+
+
+import lldb
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestIgnoreName(TestBase):
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        """Test that we can evaluate an expression that finds something inside
+           a namespace that uses an Objective C keyword.
+        """
+        self.build()
+        exe_path = self.getBuildArtifact("a.out")
+        error = lldb.SBError()
+        triple = None
+        platform = None
+        add_dependent_modules = False
+        target = self.dbg.CreateTarget(exe_path, triple, platform,
+                                       add_dependent_modules, error)
+        self.assertTrue(target, VALID_TARGET)
+        self.expect_expr("a::Class x; x", result_type="a::Class")
+        self.expect_expr("a::id i; i", result_type="a::id")

lldb/test/API/commands/expression/ignore/main.cpp

+namespace a {
+  struct Class {
+  };
+  struct id {
+  };
+}
+
+int main(int argc, char **argv) {
+  a::Class c;
+  a::id i;
+  return 0;
+}