ScriptNode : Support multi-line if in tolerantExec()

Changes.md

@@ -27,6 +27,7 @@ Fixes
   - Fixed loading of OpenEXR images with 32 bit float data and DWA compression [^1].
   - Fixed loading of secondary layers in OpenEXR images with DWA compression [^1].
 - GUI : Fixed potential crashes during shutdown [^1].
+- ScriptNode : Fixed execution of multi-line `if :` statements in `.gfr` files [^1].
 
 API
 ---

python/GafferTest/ScriptNodeTest.py

@@ -1763,5 +1763,79 @@ def testDeletingNodeRemovesFocus( self ) :
 		self.assertEqual( memberRemovals, [ ( s.focusSet(), n ) ] )
 		self.assertEqual( memberAdditions, [] )
 
+	def testExecuteWithConditionals( self ) :
+
+		script = Gaffer.ScriptNode()
+
+		script.execute(
+			inspect.cleandoc(
+				"""
+				if True :
+					script.addChild( Gaffer.Node( "True1" ) )
+				script.addChild( Gaffer.Node( "Unconditional1" ) )
+
+				if False :
+					script.addChild( Gaffer.Node( "False" ) )
+
+				if True :
+					script.addChild( Gaffer.Node( "True2" ) )
+					script.addChild( Gaffer.Node( "True3" ) )
+				script.addChild( Gaffer.Node( "Unconditional2" ) )
+
+				if(True):
+					script.addChild( Gaffer.Node( "True4" ) )
+
+				script.addChild( Gaffer.Node( "Unconditional3" ) )
+
+				if(True) :
+					script.addChild( Gaffer.Node( "True5" ) )
+
+				if (True):
+					script.addChild( Gaffer.Node( "True6" ) )
+
+				if True : script.addChild( Gaffer.Node( "True7" ) )
+				if False : script.addChild( Gaffer.Node( "False" ) )
+
+				"""
+			),
+			continueOnError = True
+		)
+
+		self.assertIn( "True1", script )
+		self.assertIn( "True2", script )
+		self.assertIn( "True3", script )
+		self.assertIn( "True4", script )
+		self.assertIn( "True5", script )
+		self.assertIn( "True6", script )
+		self.assertIn( "True7", script )
+		self.assertIn( "Unconditional1", script )
+		self.assertIn( "Unconditional2", script )
+		self.assertIn( "Unconditional3", script )
+		self.assertNotIn( "False", script )
+		self.assertNotIn( "True8", script )
+
+	def testLineNumbersAfterConditionalExecution( self ) :
+
+		script = Gaffer.ScriptNode()
+
+		with IECore.CapturingMessageHandler() as mh :
+			script.execute(
+				inspect.cleandoc(
+					"""
+					if True :
+						script.addChild( Gaffer.Node( "True" ) )
+					a = 10 * b
+					"""
+				),
+				continueOnError = True
+			)
+
+		self.assertIn( "True", script )
+
+		self.assertEqual( len( mh.messages ), 1 )
+		self.assertEqual( mh.messages[0].level, IECore.Msg.Level.Error )
+		self.assertTrue( "Line 3" in mh.messages[0].context )
+		self.assertTrue( "name 'b' is not defined" in mh.messages[0].message )
+
 if __name__ == "__main__":
 	unittest.main()

src/GafferModule/ScriptNodeBinding.cpp

@@ -65,6 +65,7 @@
 #include "fmt/format.h"
 
 #include <memory>
+#include <regex>
 
 using namespace boost;
 using namespace Gaffer;
@@ -85,12 +86,15 @@ const std::string formattedErrorContext( int lineNumber, const std::string &cont
 	);
 }
 
+const std::regex g_blockStartRegex( R"(^if[ \t(])" );
+const std::regex g_blockContinuationRegex( R"(^[ \t]+)" );
+
 // Execute the script one line at a time, reporting errors that occur,
 // but otherwise continuing with execution.
 bool tolerantExec( const std::string &pythonScript, boost::python::object globals, boost::python::object locals, const std::string &context )
 {
 	bool result = false;
-	int lineNumber = 1;
+	int lineNumber = 0;
 
 	const IECore::Canceller *canceller = Context::current()->canceller();
 
@@ -99,18 +103,56 @@ bool tolerantExec( const std::string &pythonScript, boost::python::object global
 	{
 		IECore::Canceller::check( canceller );
 
-		const std::string line( it->begin(), it->end() );
+		std::string toExecute( it->begin(), it->end() );
+		++it; ++lineNumber;
+
+		// Our serialisations have always been in a form that can be executed
+		// line-by-line. But certain third parties have used custom serialisers
+		// that output multi-line `if` statements that must be executed in a
+		// single call. Here we detect them and group them together.
+		//
+		// Notes :
+		//
+		// - Historically, we used a Python C AST API to support _any_ compound
+		//   statements here. But that is [no longer available](9d15bd21c4049d89118faf3ac27d01c5799b8264),
+		//   and using the `ast` module instead would be a significant performance
+		//   regression.
+		// - While using Python as our serialisation format is great from an
+		//   educational and reuse perspective, it has never been good from a
+		//   performance perspective. A likely future direction is to constrain
+		//   the serialisation syntax further such that it is still valid
+		//   Python, but we can parse and execute the majority of it directly in C++
+		//   for improvement performance.
+		// - We are therefore deliberately supporting only the absolute minimum of syntax
+		//   needed for the legacy third-party serialisations here, to give us
+		//   more flexibility in optimising the parsing in future.
+		if( std::regex_search( toExecute, g_blockStartRegex )  )
+		{
+			while( it != split_iterator<std::string::const_iterator>() )
+			{
+				const std::string line( it->begin(), it->end() );
+				if( std::regex_search( line, g_blockContinuationRegex ) )
+				{
+					toExecute += "\n" + line;
+					++it; ++lineNumber;
+				}
+				else
+				{
+					break;
+				}
+			}
+		}
+
 		try
 		{
-			exec( line.c_str(), globals, locals );
+			exec( toExecute.c_str(), globals, locals );
 		}
 		catch( const boost::python::error_already_set & )
 		{
 			const std::string message = IECorePython::ExceptionAlgo::formatPythonException( /* withTraceback = */ false );
 			IECore::msg( IECore::Msg::Error, formattedErrorContext( lineNumber, context ), message );
 			result = true;
 		}
-		++it; ++lineNumber;
 	}
 
 	return result;