From 1c61a3c6349973977c9228ddfe9a075b4c55ea53 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 22 Mar 2024 15:07:09 +0000 Subject: [PATCH] ScriptNode : Support multi-line `if` in `tolerantExec()` Although we don't make use of any compound statements in our own serialisations, third parties have done so using custom serialisers. Parsing for those was broken by 9d15bd21c4049d89118faf3ac27d01c5799b8264 when we stopped using an AST for parsing. Here we do the absolute bare minimum necessary to get these legacy files loading. As noted in the comments, we do not intend to do more : rather our future intention is likely to be constrain the serialisation format further for improved performance. --- Changes.md | 1 + python/GafferTest/ScriptNodeTest.py | 74 ++++++++++++++++++++++++++ src/GafferModule/ScriptNodeBinding.cpp | 50 +++++++++++++++-- 3 files changed, 121 insertions(+), 4 deletions(-) diff --git a/Changes.md b/Changes.md index 0fcbf7727fd..e7239dcb5c5 100644 --- a/Changes.md +++ b/Changes.md @@ -23,6 +23,7 @@ Fixes - Fixed error message to include filename [^1]. - Expression : `setExpression()` now respects configs that provide backwards compatibility for old plug names. - Shuffle : Fixed default name for plugs constructed via the legacy `ChannelPlug( out, in )` constructor [^1]. +- ScriptNode : Fixed execution of multi-line `if :` statements in `.gfr` files [^1]. API --- diff --git a/python/GafferTest/ScriptNodeTest.py b/python/GafferTest/ScriptNodeTest.py index 7d22be9fea7..13e3f81bba0 100644 --- a/python/GafferTest/ScriptNodeTest.py +++ b/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() diff --git a/src/GafferModule/ScriptNodeBinding.cpp b/src/GafferModule/ScriptNodeBinding.cpp index 428f59e3497..7e095a51049 100644 --- a/src/GafferModule/ScriptNodeBinding.cpp +++ b/src/GafferModule/ScriptNodeBinding.cpp @@ -65,6 +65,7 @@ #include "fmt/format.h" #include +#include 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,10 +103,49 @@ 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() ) + { + 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 & ) { @@ -110,7 +153,6 @@ bool tolerantExec( const std::string &pythonScript, boost::python::object global IECore::msg( IECore::Msg::Error, formattedErrorContext( lineNumber, context ), message ); result = true; } - ++it; ++lineNumber; } return result;