changeset 32893:2faec354b977 stable

improve symbol lookup in help and which functions (bug #65220) * help.h, help.cc (get_help_from_fcn): New static function. (help_system::get_which_info_from_fcn): New member function. (help_system::which, help_system::raw_help_from_symbol_table): Improve symbol lookup for names like "a.b.c". Consider packages, classes, methods, and properties.
author John W. Eaton <jwe@octave.org>
date Mon, 29 Jan 2024 00:08:00 -0500
parents 80d3a6abfc4d
children 9104b087dc26 82b67791bf1b
files libinterp/corefcn/help.cc libinterp/corefcn/help.h
diffstat 2 files changed, 284 insertions(+), 50 deletions(-) [+]
line wrap: on
line diff
--- a/libinterp/corefcn/help.cc	Wed Jan 31 23:19:10 2024 -0500
+++ b/libinterp/corefcn/help.cc	Mon Jan 29 00:08:00 2024 -0500
@@ -232,24 +232,15 @@
   return h;
 }
 
-std::string
-help_system::which (const std::string& name,
-                    std::string& type) const
+bool
+help_system::get_which_info_from_fcn (const std::string& name, const octave_value& ov_fcn, std::string& file, std::string& type) const
 {
-  std::string file;
-
-  if (name.empty ())
-    return file;
-
+  file = "";
   type = "";
 
-  symbol_table& symtab = m_interpreter.get_symbol_table ();
-
-  octave_value val = symtab.find_function (name);
-
-  if (val.is_defined ())
+  if (ov_fcn.is_function ())
     {
-      octave_function *fcn = val.function_value ();
+      octave_function *fcn = ov_fcn.function_value ();
 
       if (fcn)
         {
@@ -269,11 +260,10 @@
             }
           else
             {
-
               file = fcn->fcn_file_name ();
 
               if (! file.empty ())
-                type = val.is_user_script () ? "script" : "function";
+                type = ov_fcn.is_user_script () ? "script" : "function";
               else
                 {
                   if (fcn->is_user_function ())
@@ -285,6 +275,8 @@
                     }
                 }
             }
+
+          return true;
         }
       else
         {
@@ -293,24 +285,147 @@
           load_path& lp = m_interpreter.get_load_path ();
 
           file = lp.find_fcn_file (name);
+
+          if (file.empty ())
+            return false;
         }
     }
 
-  if (file.empty ())
+  return false;
+}
+
+// FIXME: There is a lot of duplication between the following function
+// and help_system::raw_help_from_symbol_table.  Some refactoring would
+// probably be useful.
+
+std::string
+help_system::which (const std::string& name,
+                    std::string& type) const
+{
+  std::string file;
+
+  if (name.empty ())
+    return file;
+
+  type = "";
+
+  symbol_table& symtab = m_interpreter.get_symbol_table ();
+
+  size_t pos = name.find ('.');
+
+  if (pos == std::string::npos)
     {
-      // File query.
+      // Simple name.  If not found, continue looking for packages and
+      // classes.
+
+      octave_value ov_fcn = symtab.find_function (name);
+
+      if (get_which_info_from_fcn (name, ov_fcn, file, type))
+        return file;
+    }
+
+  // NAME contains '.' and must match the following pattern:
+  //
+  //   (package.)*(package|classname).(property|method)*
+  //
+  // Start by looking up the full name.  It could be either a package or
+  // a class and we are done.  Otherwise, strip the final component and
+  // lookup that name.  If it is a package, look for a function.  If it
+  // is a class, look for a property or method.
+
+  cdef_manager& cdm = m_interpreter.get_cdef_manager ();
 
-      load_path& lp = m_interpreter.get_load_path ();
+  // FIXME: In the following search we may load classes.  Is that really
+  // what we want, or should we just search the loadpath for
+  // +pkga/+pkgb/classname/file.m, etc. and attempt to extract help text
+  // without actually installing packages and classes into the fcn_info
+  // table?
+
+  // Is NAME a class?
+
+  cdef_class cls = cdm.find_class (name, false, true);
+
+  if (cls.ok ())
+    {
+      // FIXME: Return documentation for the class or the class
+      // constructor?
+
+      file = cls.file_name ();
+      type = "classdef class";
+
+      return file;
+    }
+
+  cdef_package pkg = cdm.find_package (name, false, true);
 
-      // For compatibility: "file." queries "file".
-      if (name.size () > 1 && name[name.size () - 1] == '.')
-        file = lp.find_file (name.substr (0, name.size () - 1));
-      else
-        file = lp.find_file (name);
+  if (pkg.ok ())
+    {
+      // FIXME: How to get the fill name of a package?
+      file = pkg.get_name ();
+      type = "package";
+
+      return file;
+    }
+
+  // Strip final component (might be a property or method name).
+
+  pos = name.rfind ('.');
+  std::string prefix = name.substr (0, pos);
+  std::string nm = name.substr (pos+1);
+
+  // Is PREFIX the name of a class?
+
+  cls = cdm.find_class (prefix, false, true);
+
+  if (cls.ok ())
+    {
+      // FIXME: Should we only find public methods here?
+
+      octave_value ov_meth = cls.get_method (nm);
+
+      if (get_which_info_from_fcn (nm, ov_meth, file, type))
+        return file;
+
+      // FIXME: Should we only find public properties here?
+
+      cdef_property prop = cls.find_property (nm);
 
-      file = sys::env::make_absolute (file);
+      if (prop.ok ())
+        {
+          file = cls.file_name ();
+          type = "class property";
+
+          return file;
+        }
     }
 
+  // Or is PREFIX the name of a package?
+
+  pkg = cdm.find_package (prefix, false, true);
+
+  if (pkg.ok ())
+    {
+      octave_value ov_fcn = pkg.find (nm);
+
+      if (get_which_info_from_fcn (nm, ov_fcn, file, type))
+        return file;
+    }
+
+  // File query.
+
+  load_path& lp = m_interpreter.get_load_path ();
+
+  // For compatibility: "file." queries "file".
+  if (name.size () > 1 && name[name.size () - 1] == '.')
+    file = lp.find_file (name.substr (0, name.size () - 1));
+  else
+    file = lp.find_file (name);
+
+  file = sys::env::make_absolute (file);
+
+  if (! file.empty ())
+    type = "file";
+
   return file;
 }
 
@@ -545,51 +660,168 @@
   return retval;
 }
 
+static bool
+get_help_from_fcn (const std::string& fcn_nm, const octave_value& ov_fcn, std::string& help, std::string& what, bool& symbol_found)
+{
+  symbol_found = false;
+
+  help = "";
+  what = "";
+
+  if (ov_fcn.is_function ())
+    {
+      octave_function *fcn = ov_fcn.function_value ();
+
+      help = fcn->doc_string (fcn_nm);
+      what = fcn->fcn_file_name ();
+
+      if (what.empty ())
+        what = fcn->is_user_function () ? "command-line function" : "built-in function";
+
+      symbol_found = true;
+    }
+
+  return symbol_found;
+}
+
+// FIXME: There is a lot of duplication between the following function
+// and help_system::which.  Some refactoring would probably be useful.
+
 bool
-help_system::raw_help_from_symbol_table (const std::string& nm,
-    std::string& h, std::string& w,
-    bool& symbol_found) const
+help_system::raw_help_from_symbol_table (const std::string& name, std::string& help, std::string& what, bool& symbol_found) const
 {
-  std::string meth_nm;
-
   symbol_table& symtab = m_interpreter.get_symbol_table ();
 
-  octave_value val = symtab.find_function (nm);
+  size_t pos = name.find ('.');
+
+  if (pos == std::string::npos)
+    {
+      // Simple name.  If not found, continue looking for packages and
+      // classes.
+
+      octave_value ov_fcn = symtab.find_function (name);
+
+      // FIXME: it seems like there is a lot of potential for confusion
+      // because is_function can also return true for
+      // octave_classdef_meta objects.
 
-  if (! val.is_defined ())
-    {
-      std::size_t pos = nm.rfind ('.');
+      if (! ov_fcn.is_classdef_meta ()
+          && get_help_from_fcn (name, ov_fcn, help, what, symbol_found))
+        return true;
+    }
+
+  // If NAME does not contain '.', then it should be a package or a
+  // class name.
+  //
+  // If NAME contains '.' it should match the following pattern:
+  //
+  //   (package.)*(package|classname).(property|method)*
+  //
+  // Start by looking up the full name.  It could be either a package or
+  // a class and we are done.  Otherwise, strip the final component and
+  // lookup that name.  If it is a package, look for a function.  If it
+  // is a class, look for a property or method.
+
+  cdef_manager& cdm = m_interpreter.get_cdef_manager ();
 
-      if (pos != std::string::npos)
+  // FIXME: In the following search we may load classes.  Is that really
+  // what we want, or should we just search the loadpath for
+  // +pkga/+pkgb/classname/file.m, etc. and attempt to extract help text
+  // without actually installing packages and classes into the fcn_info
+  // table?
+
+  // Is NAME a class?
+
+  cdef_class cls = cdm.find_class (name, false, true);
+
+  if (cls.ok ())
+    {
+      // Is the class documented?
+
+      help = cls.doc_string ();
+
+      if (! help.empty ())
         {
-          meth_nm = nm.substr (pos+1);
+          what = "class";
+
+          symbol_found = true;
+          return true;
+        }
+
+      // Look for constructor.
 
-          val = symtab.find_function (nm.substr (0, pos));
+      pos = name.rfind ('.');
+      std::string nm = name.substr (pos+1);
+
+      octave_value ov_meth = cls.get_method (nm);
+
+      if (get_help_from_fcn (nm, ov_meth, help, what, symbol_found))
+        {
+          what = "constructor";
+
+          return true;
         }
     }
 
-  if (val.is_defined ())
+  cdef_package pkg = cdm.find_package (name, false, true);
+
+  if (pkg.ok ())
     {
-      octave_function *fcn = val.function_value ();
+      help = "package " + name;
+      what = "package";
+
+      symbol_found = true;
+      return true;
+    }
+
+  // Strip final component (might be a property or method name).
+
+  pos = name.rfind ('.');
+  std::string prefix = name.substr (0, pos);
+  std::string nm = name.substr (pos+1);
+
+  // Is PREFIX the name of a class?
+
+  cls = cdm.find_class (prefix, false, true);
 
-      if (fcn)
+  if (cls.ok ())
+    {
+      // FIXME: Should we only find public methods here?
+
+      octave_value ov_meth = cls.get_method (nm);
+
+      if (get_help_from_fcn (nm, ov_meth, help, what, symbol_found))
+        return true;
+
+      // FIXME: Should we only find public properties here?
+
+      cdef_property prop = cls.find_property (nm);
+
+      if (prop.ok ())
         {
-          // FCN may actually be a classdef_meta object.
+          // FIXME: is it supposed to be possible to document
+          // properties?
+
+          help = prop.doc_string ();
+          what = "class property";
 
           symbol_found = true;
-
-          h = fcn->doc_string (meth_nm);
-
-          w = fcn->fcn_file_name ();
-
-          if (w.empty ())
-            w = fcn->is_user_function () ? "command-line function"
-                : "built-in function";
-
           return true;
         }
     }
 
+  // Or is PREFIX the name of a package?
+
+  pkg = cdm.find_package (prefix, false, true);
+
+  if (pkg.ok ())
+    {
+      octave_value ov_fcn = pkg.find (nm);
+
+      if (get_help_from_fcn (nm, ov_fcn, help, what, symbol_found))
+        return true;
+    }
+
   return false;
 }
 
--- a/libinterp/corefcn/help.h	Wed Jan 31 23:19:10 2024 -0500
+++ b/libinterp/corefcn/help.h	Mon Jan 29 00:08:00 2024 -0500
@@ -187,6 +187,8 @@
     return old_val;
   }
 
+  bool get_which_info_from_fcn (const std::string& name, const octave_value& ov_fcn, std::string& file, std::string& type) const;
+
   string_vector local_functions () const;
 
   bool raw_help_from_symbol_table (const std::string& nm,