Expand docstring checks to also check names of symbols in the interface

qrenderdoc/Code/pyrenderdoc/PythonContext.cpp

@@ -58,9 +58,9 @@ PyTypeObject **SbkPySide2_QtWidgetsTypes = NULL;
 #include "Code/QRDUtils.h"
 #include "PythonContext.h"
 
-// exported by generated files, used to check docstrings in interfaces
-bool CheckCoreDocstrings();
-bool CheckQtDocstrings();
+// exported by generated files, used to check interface compliance
+bool CheckCoreInterface();
+bool CheckQtInterface();
 
 // defined in SWIG-generated renderdoc_python.cpp
 extern "C" PyObject *PyInit__renderdoc(void);
@@ -443,13 +443,13 @@ PythonContext::~PythonContext()
   outputTick();
 }
 
-bool PythonContext::CheckDocstrings()
+bool PythonContext::CheckInterfaces()
 {
   bool errors = false;
 
   PyGILState_STATE gil = PyGILState_Ensure();
-  errors |= CheckCoreDocstrings();
-  errors |= CheckQtDocstrings();
+  errors |= CheckCoreInterface();
+  errors |= CheckQtInterface();
   PyGILState_Release(gil);
 
   return errors;

qrenderdoc/Code/pyrenderdoc/PythonContext.h

@@ -55,7 +55,7 @@ public:
   static void GlobalInit();
   static void GlobalShutdown();
 
-  bool CheckDocstrings();
+  bool CheckInterfaces();
 
   QString versionString();
 

qrenderdoc/Code/pyrenderdoc/interface_check.h

@@ -24,7 +24,101 @@
 
 #pragma once
 
-inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes)
+// verify the interface.
+// We check that docstrings aren't duplicated, which is a symptom of missing DOCUMENT()
+// macros around newly added classes/members.
+// For enums, verify that all constants are documented in the parent docstring
+// Generally we ensure naming is roughly OK:
+// * types, member functions, and enum values must match the regexp /[A-Z][a-zA-Z0-9]+/
+//   ie. we don't use underscore_seperated_words or mixedCase / camelCase.
+// * data members should be mixedCase / camelCase. So matching /[a-z][a-zA-Z0-9]+/
+// This isn't quite python standards but it fits best with the C++ code and the important
+// thing is that it's self-consistent.
+
+enum class NameType
+{
+  Type,
+  EnumValue,
+  Method,
+  Member,
+};
+
+inline bool checkname(const char *baseType, std::string name, NameType nameType)
+{
+  // skip __ prefixed names
+  if(name.length() > 2 && name[0] == '_' && name[1] == '_')
+    return false;
+
+  // skip any rdctype based types that are converted into equivalent python types
+  if((baseType && strstr(baseType, "rdcarray")) || name.find("rdcarray") != std::string::npos)
+    return false;
+  if((baseType && strstr(baseType, "bytebuf")) || name.find("bytebuf") != std::string::npos)
+    return false;
+  if((baseType && strstr(baseType, "rdcstr")) || name.find("rdcstr") != std::string::npos)
+    return false;
+  if((baseType && strstr(baseType, "StructuredBufferList")) ||
+     name.find("StructuredBufferList") != std::string::npos)
+    return false;
+  if((baseType && strstr(baseType, "StructuredChunkList")) ||
+     name.find("StructuredChunkList") != std::string::npos)
+    return false;
+  if((baseType && strstr(baseType, "StructuredObjectList")) ||
+     name.find("StructuredObjectList") != std::string::npos)
+    return false;
+
+  // allow the config to have different names
+  if((baseType && strstr(baseType, "PersistantConfig")) ||
+     name.find("PersistantConfig") != std::string::npos)
+    return false;
+
+  // skip swig internal type
+  if((baseType && strstr(baseType, "SwigPyObject")) || name.find("SwigPyObject") != std::string::npos)
+    return false;
+
+  // remove the module prefix, if this is a type name we're checking
+  if(!strncmp(name.c_str(), "renderdoc.", 10))
+    name.erase(0, 10);
+  if(!strncmp(name.c_str(), "qrenderdoc.", 11))
+    name.erase(0, 11);
+
+  // skip a few well-known members
+  if(name == "this" || name == "thisown")
+    return false;
+
+  bool member = (nameType == NameType::Member);
+
+  // look for invalid name
+  bool badfirstChar = false;
+  if(member)
+    badfirstChar = name[0] < 'a' || name[0] > 'z';
+  else
+    badfirstChar = name[0] < 'A' || name[0] > 'Z';
+
+  if(badfirstChar || name.find('_') != std::string::npos)
+  {
+    const char *nameTypeStr = "";
+
+    switch(nameType)
+    {
+      case NameType::EnumValue: nameTypeStr = "enum value"; break;
+      case NameType::Member: nameTypeStr = "member variable"; break;
+      case NameType::Method: nameTypeStr = "method"; break;
+      case NameType::Type: nameTypeStr = "type"; break;
+    }
+
+    snprintf(convert_error, sizeof(convert_error) - 1,
+             "Name of %s '%s.%s' does not match naming scheme.\n"
+             "Should start with %s letter and not contain underscores",
+             nameTypeStr, baseType, name.c_str(), member ? "lowercase" : "uppercase");
+    RENDERDOC_LogMessage(LogType::Error, "QTRD", __FILE__, __LINE__, convert_error);
+
+    return true;
+  }
+
+  return false;
+}
+
+inline bool check_interface(swig_type_info **swig_types, size_t numTypes)
 {
   // track all errors and fatal error at the end, so we see all of the problems at once instead of
   // requiring rebuilds over and over.
@@ -56,6 +150,8 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes)
       errors_found = true;
     }
 
+    errors_found |= checkname("renderdoc", typeobj->tp_name, NameType::Type);
+
     PyObject *dict = typeobj->tp_dict;
 
     // check the object's dict to see if this is an enum (or struct with constants).
@@ -74,8 +170,8 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes)
           PyObject *key = PyList_GetItem(keys, i);
           PyObject *value = PyDict_GetItem(dict, key);
 
-          // if this key is a string (it should be) and the value is an integer, it's a constant
-          if(PyUnicode_Check(key) && PyLong_Check(value))
+          // if this key is a string (it should be)
+          if(PyUnicode_Check(key))
           {
             char *str = NULL;
             Py_ssize_t len = 0;
@@ -84,12 +180,27 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes)
 
             if(str == NULL || len == 0)
             {
+              snprintf(convert_error, sizeof(convert_error) - 1,
+                       "Couldn't get member name for %i'th member of '%s'", (int)i, typeobj->tp_name);
               RENDERDOC_LogMessage(LogType::Error, "QTRD", __FILE__, __LINE__, convert_error);
               errors_found = true;
             }
             else
             {
-              constants.insert(std::string(str, str + len));
+              std::string name(str, str + len);
+
+              NameType nameType = NameType::Member;
+
+              // if the value is an integer, it's a constant
+              if(PyLong_Check(value))
+              {
+                constants.insert(name);
+                nameType = NameType::EnumValue;
+              }
+
+              // if it's a callable it's a method, ignore it
+              if(!PyCallable_Check(value) && !PyType_IsSubtype(value->ob_type, &PyStaticMethod_Type))
+                errors_found |= checkname(typeobj->tp_name, name, nameType);
             }
 
             Py_DecRef(bytes);
@@ -150,6 +261,8 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes)
     {
       std::string method_doc = method->ml_doc;
 
+      checkname(typeobj->tp_name, method->ml_name, NameType::Method);
+
       size_t i = 0;
       while(method_doc[i] == '\n')
         i++;

qrenderdoc/Code/pyrenderdoc/qrenderdoc.i

@@ -113,30 +113,28 @@ TEMPLATE_ARRAY_INSTANTIATE_PTR(rdcarray, ICaptureViewer)
 
 %header %{
   #include <set>
-  #include "Code/pyrenderdoc/document_check.h"
+  #include "Code/pyrenderdoc/interface_check.h"
 
-  // verify that docstrings aren't duplicated, which is a symptom of missing DOCUMENT()
-  // macros around newly added classes/members.
-  // For enums, verify that all constants are documented in the parent docstring
-  static swig_type_info **docCheckTypes;
-  static size_t docCheckNumTypes = 0;
+  // check interface, see interface_check.h for more information
+  static swig_type_info **interfaceCheckTypes;
+  static size_t interfaceCheckNumTypes = 0;
 
-  bool CheckQtDocstrings()
+  bool CheckQtInterface()
   {
 #if defined(RELEASE)
     return false;
 #else
-    if(docCheckNumTypes == 0)
+    if(interfaceCheckNumTypes == 0)
       return false;
 
-    return check_docstrings(docCheckTypes, docCheckNumTypes);
+    return check_interface(interfaceCheckTypes, interfaceCheckNumTypes);
 #endif
   }
 %}
 
 %init %{
-  docCheckTypes = swig_type_initial;
-  docCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]);
+  interfaceCheckTypes = swig_type_initial;
+  interfaceCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]);
 %}
 
 // declare functions for using swig opaque wrap/unwrap of QWidget, for when pyside isn't available.

qrenderdoc/Code/pyrenderdoc/renderdoc.i

@@ -278,28 +278,26 @@ PyObject *PassObjectToPython(const char *type, void *obj)
 
 %header %{
   #include <set>
-  #include "Code/pyrenderdoc/document_check.h"
+  #include "Code/pyrenderdoc/interface_check.h"
 
-  // verify that docstrings aren't duplicated, which is a symptom of missing DOCUMENT()
-  // macros around newly added classes/members.
-  // For enums, verify that all constants are documented in the parent docstring
-  static swig_type_info **docCheckTypes;
-  static size_t docCheckNumTypes = 0;
+  // check interface, see interface_check.h for more information
+  static swig_type_info **interfaceCheckTypes;
+  static size_t interfaceCheckNumTypes = 0;
 
-  bool CheckCoreDocstrings()
+  bool CheckCoreInterface()
   {
 #if defined(RELEASE)
     return false;
 #else
-    if(docCheckNumTypes == 0)
+    if(interfaceCheckNumTypes == 0)
       return false;
 
-    return check_docstrings(docCheckTypes, docCheckNumTypes);
+    return check_interface(interfaceCheckTypes, interfaceCheckNumTypes);
 #endif
   }
 %}
 
 %init %{
-  docCheckTypes = swig_type_initial;
-  docCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]);
+  interfaceCheckTypes = swig_type_initial;
+  interfaceCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]);
 %}

qrenderdoc/Code/qrenderdoc.cpp

@@ -79,20 +79,20 @@ int main(int argc, char *argv[])
 
       bool errors = false;
 
-      qInfo() << "Checking python binding docstrings.";
+      qInfo() << "Checking python binding consistency.";
 
       {
         PythonContextHandle py;
-        errors = py.ctx().CheckDocstrings();
+        errors = py.ctx().CheckInterfaces();
       }
 
       if(errors)
       {
-        qCritical() << "Found errors in python binding docstrings. Please fix!";
+        qCritical() << "Found errors in python bindings. Please fix!";
         return 1;
       }
 
-      qInfo() << "Python binding docstrings are consistent.";
+      qInfo() << "Python bindings are consistent.";
       return 0;
     }
   }

qrenderdoc/qrenderdoc.pro

@@ -234,7 +234,7 @@ HEADERS += Code/CaptureContext.h \
     Code/Resources.h \
     Code/pyrenderdoc/PythonContext.h \
     Code/pyrenderdoc/pyconversion.h \
-    Code/pyrenderdoc/document_check.h \
+    Code/pyrenderdoc/interface_check.h \
     Code/Interface/QRDInterface.h \
     Code/Interface/Analytics.h \
     Code/Interface/CommonPipelineState.h \

qrenderdoc/qrenderdoc_local.vcxproj

@@ -882,7 +882,7 @@
     <ClInclude Include="Code\Interface\RemoteHost.h" />
     <ClInclude Include="Code\precompiled.h" />
     <ClInclude Include="Code\pyrenderdoc\container_handling.h" />
-    <ClInclude Include="Code\pyrenderdoc\document_check.h" />
+    <ClInclude Include="Code\pyrenderdoc\interface_check.h" />
     <ClInclude Include="Code\pyrenderdoc\ext_refcounts.h" />
     <ClInclude Include="Code\pyrenderdoc\function_conversion.h" />
     <ClInclude Include="Code\pyrenderdoc\qt_conversion.h" />
@@ -1297,7 +1297,7 @@
   <ItemGroup>
     <CustomBuild Include="Code\pyrenderdoc\renderdoc.i">
       <FileType>Document</FileType>
-      <AdditionalInputs>%(Fullpath);container_handling.i;pyconversion.i;cosmetics.i;document_check.h;$(SolutionDir)renderdoc\api\replay\basic_types.h;$(SolutionDir)renderdoc\api\replay\capture_options.h;$(SolutionDir)renderdoc\api\replay\control_types.h;$(SolutionDir)renderdoc\api\replay\d3d11_pipestate.h;$(SolutionDir)renderdoc\api\replay\d3d12_pipestate.h;$(SolutionDir)renderdoc\api\replay\data_types.h;$(SolutionDir)renderdoc\api\replay\gl_pipestate.h;$(SolutionDir)renderdoc\api\replay\renderdoc_replay.h;$(SolutionDir)renderdoc\api\replay\replay_enums.h;$(SolutionDir)renderdoc\api\replay\shader_types.h;$(SolutionDir)renderdoc\api\replay\stringise.h;$(SolutionDir)renderdoc\api\replay\structured_data.h;$(SolutionDir)renderdoc\api\replay\vk_pipestate.h;%(AdditionalInputs)</AdditionalInputs>
+      <AdditionalInputs>%(Fullpath);container_handling.i;pyconversion.i;cosmetics.i;interface_check.h;$(SolutionDir)renderdoc\api\replay\basic_types.h;$(SolutionDir)renderdoc\api\replay\capture_options.h;$(SolutionDir)renderdoc\api\replay\control_types.h;$(SolutionDir)renderdoc\api\replay\d3d11_pipestate.h;$(SolutionDir)renderdoc\api\replay\d3d12_pipestate.h;$(SolutionDir)renderdoc\api\replay\data_types.h;$(SolutionDir)renderdoc\api\replay\gl_pipestate.h;$(SolutionDir)renderdoc\api\replay\renderdoc_replay.h;$(SolutionDir)renderdoc\api\replay\replay_enums.h;$(SolutionDir)renderdoc\api\replay\shader_types.h;$(SolutionDir)renderdoc\api\replay\stringise.h;$(SolutionDir)renderdoc\api\replay\structured_data.h;$(SolutionDir)renderdoc\api\replay\vk_pipestate.h;%(AdditionalInputs)</AdditionalInputs>
       <Command>"$(ProjectDir)3rdparty\swig\swig.exe" -v -Wextra -Werror -O -c++ -python -modern -modernargs -enumclass -fastunpack -py3 -builtin -I"$(SolutionDir)renderdoc\api\replay" -outdir "$(IntDir)generated" -o "$(IntDir)generated\%(Filename)_python.cxx" "%(FullPath)"</Command>
       <Message>Compiling SWIG interface</Message>
       <Outputs>$(IntDir)generated\%(Filename).py;$(IntDir)generated\%(Filename)_python.cxx;%(Outputs)</Outputs>

qrenderdoc/qrenderdoc_local.vcxproj.filters

@@ -1004,9 +1004,6 @@
     <ClInclude Include="Code\Interface\QRDInterface.h">
       <Filter>Code\Interface</Filter>
     </ClInclude>
-    <ClInclude Include="Code\pyrenderdoc\document_check.h">
-      <Filter>Code\pyrenderdoc</Filter>
-    </ClInclude>
     <ClInclude Include="Code\ReplayManager.h">
       <Filter>Code</Filter>
     </ClInclude>
@@ -1058,6 +1055,9 @@
     <ClInclude Include="$(IntDir)generated\ui_UpdateDialog.h">
       <Filter>Generated Files</Filter>
     </ClInclude>
+    <ClInclude Include="Code\pyrenderdoc\interface_check.h">
+      <Filter>Code\pyrenderdoc</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <None Include="Code\pyrenderdoc\pyconversion.h">