From b63c9a87dbb7185c6714b0ba5094de8abe544d1e Mon Sep 17 00:00:00 2001 From: chcurran <82187351+carlitosan@users.noreply.github.com> Date: Thu, 8 Jul 2021 15:08:55 -0700 Subject: [PATCH] compile time null checks avaiable for method node Signed-off-by: chcurran <82187351+carlitosan@users.noreply.github.com> --- .../Code/Include/ScriptCanvas/Core/Node.cpp | 5 + .../Code/Include/ScriptCanvas/Core/Node.h | 3 + .../Grammar/AbstractCodeModel.cpp | 56 +- .../ScriptCanvas/Grammar/AbstractCodeModel.h | 4 +- .../ScriptCanvas/Libraries/Core/Method.cpp | 42 +- .../ScriptCanvas/Libraries/Core/Method.h | 5 +- .../Include/ScriptCanvas/Results/ErrorText.h | 1 + ...nitTest_ParseErrorOnKnownNull.scriptcanvas | 947 ++++++++++++++++++ .../Tests/ScriptCanvas_RuntimeInterpreted.cpp | 10 + 9 files changed, 1046 insertions(+), 27 deletions(-) create mode 100644 Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseErrorOnKnownNull.scriptcanvas diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.cpp b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.cpp index cb1887252c..c6c9330e51 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.cpp +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.cpp @@ -2335,6 +2335,11 @@ namespace ScriptCanvas } } + bool Node::CanAcceptNullInput([[maybe_unused]] const Slot& executionSlot, [[maybe_unused]] const Slot& inputSlot) const + { + return true; + } + void Node::CollectVariableReferences(AZStd::unordered_set< ScriptCanvas::VariableId >& variableIds) const { for (const Slot& slot : m_slots) diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.h b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.h index bb598c01fd..614494883a 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.h +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Core/Node.h @@ -479,7 +479,10 @@ namespace ScriptCanvas Node(const Node&); // Needed just for DLL linkage. Does not perform a copy Node& operator=(const Node&); // Needed just for DLL linkage. Does not perform a copy + virtual bool CanAcceptNullInput(const Slot& executionSlot, const Slot& inputSlot) const; + virtual void CollectVariableReferences(AZStd::unordered_set< ScriptCanvas::VariableId >& variableIds) const; + virtual bool ContainsReferencesToVariables(const AZStd::unordered_set< ScriptCanvas::VariableId >& variableIds) const; Graph* GetGraph() const; diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp index 293ad236d3..80d2977c6d 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp @@ -670,6 +670,28 @@ namespace ScriptCanvas return AddVariable(Datum(type), rawName); } + void AbstractCodeModel::CheckForKnownNullDereference(ExecutionTreeConstPtr execution, const ExecutionInput& input, const Slot& inputSlot) + { + if (Data::IsValueType(inputSlot.GetDataType()) + || !execution->GetId().m_node + || !execution->GetId().m_slot + || (input.m_value && !input.m_value->m_datum.Empty())) + { + return; + } + + if (!input.m_value) + { + AddError(execution->GetId().m_node->GetEntityId(), nullptr, "Internal Error: CheckForKnownNullDereference called with input with no m_value"); + return; + } + + if (!execution->GetId().m_node->CanAcceptNullInput(*execution->GetId().m_slot, inputSlot)) + { + AddError(execution->GetId().m_node->GetEntityId(), nullptr, ParseErrors::NullInputKnown); + } + } + void AbstractCodeModel::CheckConversion(ConversionByIndex& conversion, VariableConstPtr source, size_t index, const Data::Type& targetType) { const Data::Type& sourceType = source->m_datum.GetType(); @@ -698,7 +720,7 @@ namespace ScriptCanvas } AZStd::string AbstractCodeModel::CheckUniqueInterfaceNames - (AZStd::string_view candidate + ( AZStd::string_view candidate , AZStd::string_view defaultName , AZStd::unordered_set& uniqueNames , const AZStd::unordered_set& nodelingsOut) @@ -4285,10 +4307,7 @@ namespace ScriptCanvas void AbstractCodeModel::ParseInputDatum(ExecutionTreePtr execution, const Slot& input) { - // \todo look for crossed lines in inferred functions, because sometimes, rather than the input - // being named of the result of the output that emitted it, it will be the name of the inferred function - // parameter ---> make a map of node output to function input names - AZ_Assert(execution->GetSymbol() != Symbol::FunctionDefinition, "Function definition input should have been handled already"); + AZ_Assert(execution->GetSymbol() != Symbol::FunctionDefinition, "Function definition input is not handled in AbstractCodeModel::ParseInputDatum"); auto nodes = execution->GetId().m_node->GetConnectedNodes(input); if (nodes.empty()) @@ -4298,17 +4317,6 @@ namespace ScriptCanvas execution->AddInput({ &input, variable, DebugDataSource::FromVariable(input.GetId(), input.GetDataType(), variable->m_sourceVariableId) }); CheckConversion(execution->ModConversions(), variable, execution->GetInputCount() - 1, input.GetDataType()); } - // This concept may never actually be possible -// else if (RequiresCreationFunction(input.GetDataType().GetType())) -// { -// AddError(execution, aznew NotYetImplemented( -// "1: finish input created by name when connected to other nodes" -// "2: add the name to the scope" -// "3: and check inputs be re-used, common constructors like zero/1, etc" -// "4: read the variable name if it is present instead of creating it" -// "5: check for entity references to self and other member slice variables" -// "6: mark the variable with RequiredCreationFunction()")); -// } else { auto variableDatum = input.FindDatum(); @@ -4345,26 +4353,30 @@ namespace ScriptCanvas } else { - // we don't support this, yet, but visually we could - // we could support both things, technically...auto-generated inputs, and defaults on the non-connected - // execution thread, or whatever makes possible sense - // \todo send enough information to reveal the data path in the editor - + // This isn't supported visually, yet, but technically, it could be. + // One could connect both latent execution and immediate execution to the same code execution path, + // but only one uses connected output, and the other uses defaults. + // It would require us to change the visualization of the shared path, based on which parent was clicked on in the editor. const auto& targetNode = *execution->GetId().m_node; const auto& targetSlot = input; for (auto sourceNodeAndSlot : nodes) { AddError(nullptr, aznew ScopedDataConnectionEvent - (execution->GetNodeId() + ( execution->GetNodeId() , targetNode , targetSlot , *sourceNodeAndSlot.first , *sourceNodeAndSlot.second)); } + + return; } } + + // Check for known null reads + CheckForKnownNullDereference(execution, execution->GetInput(execution->GetInputCount() - 1), input); } bool AbstractCodeModel::ParseInputThisPointer(ExecutionTreePtr execution) diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.h b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.h index 06854fdbfd..45e289739d 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.h +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.h @@ -207,8 +207,10 @@ namespace ScriptCanvas bool CheckCreateRoot(const Node& node); + void CheckForKnownNullDereference(ExecutionTreeConstPtr parent, const ExecutionInput& input, const Slot& inputSlot); + AZStd::string CheckUniqueInterfaceNames - (AZStd::string_view candidate + ( AZStd::string_view candidate , AZStd::string_view defaultName , AZStd::unordered_set& uniqueNames , const AZStd::unordered_set& nodelingsOut); diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.cpp b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.cpp index 32b5e5741f..4eafa82331 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.cpp +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.cpp @@ -25,6 +25,7 @@ namespace MethodCPP Unnamed2, PluralizeResults, AddedPrettyNameFieldToSerialization, + StoreInputSlotIdsToSupportNullCheck, // add your version above Current, }; @@ -110,6 +111,37 @@ namespace ScriptCanvas { namespace Core { + bool Method::CanAcceptNullInput([[maybe_unused]] const Slot& executionSlot, const Slot& inputSlot) const + { + if (m_method) + { + auto candidateID = inputSlot.GetId(); + auto slotIter = AZStd::find(m_inputSlots.begin(), m_inputSlots.end(), candidateID); + + if (slotIter != m_inputSlots.end()) + { + const size_t index = slotIter - m_inputSlots.begin(); + if (index < m_method->GetNumArguments()) + { + const auto* argument = m_method->GetArgument(index); + if (argument->m_traits & (AZ::BehaviorParameter::TR_REFERENCE | AZ::BehaviorParameter::TR_THIS_PTR)) + { + // references and this pointers cannot accept null input + return false; + } + + if (!(argument->m_traits & AZ::BehaviorParameter::TR_POINTER)) + { + // values cannot accept null input + return false; + } + } + } + } + + return true; + } + const AZ::BehaviorClass* Method::GetClass() const { return m_class; @@ -232,6 +264,11 @@ namespace ScriptCanvas if (addedSlot.IsValid()) { MethodHelper::SetSlotToDefaultValue(*this, addedSlot, config, argIndex); + m_inputSlots.push_back(addedSlot); + } + else + { + AZ_Warning("ScriptCanvas", false, "Failed to add method input slot to Method node: %s-%s", config.m_prettyClassName.c_str(), config.m_method.m_name.c_str()); } } } @@ -520,8 +557,8 @@ namespace ScriptCanvas if (m_method && m_method->HasResult()) { if (branchOnResultMethod.GetNumArguments() == 1 - && branchOnResultMethod.HasResult() - && Data::FromAZType(branchOnResultMethod.GetResult()->m_typeId) == Data::Type::Boolean()) + && branchOnResultMethod.HasResult() + && Data::FromAZType(branchOnResultMethod.GetResult()->m_typeId) == Data::Type::Boolean()) { AZ::Uuid methodResultType = m_method->GetResult()->m_typeId; AZ::Uuid branchOnResultMethodArgType = branchOnResultMethod.GetArgument(0)->m_typeId; @@ -797,6 +834,7 @@ namespace ScriptCanvas ->Field("className", &Method::m_className) ->Field("namespaces", &Method::m_namespaces) ->Field("resultSlotIDs", &Method::m_resultSlotIDs) + ->Field("inputSlots", &Method::m_inputSlots) ->Field("prettyClassName", &Method::m_classNamePretty) ; diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.h b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.h index db34da040c..3dec8e3e72 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.h +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Libraries/Core/Method.h @@ -45,6 +45,8 @@ namespace ScriptCanvas size_t GenerateFingerprint() const override; + bool CanAcceptNullInput(const Slot& executionSlot, const Slot& inputSlot) const override; + bool GetBranchOnResultCheckName(AZStd::string& exposedName, Grammar::LexicalScope& lexicalScope) const; virtual bool GetCheckedOperationInfo(AZ::CheckedOperationInfo& checkedInfo, AZStd::string& exposedName, Grammar::LexicalScope& lexicalScope) const; @@ -149,8 +151,6 @@ namespace ScriptCanvas bool IsExpectingResult() const; - AZ_INLINE AZStd::vector& ModResultSlotIds() { return m_resultSlotIDs; } - virtual void OnInitializeOutputPost(const MethodOutputConfig&) {} virtual void OnInitializeOutputPre(MethodOutputConfig&) {} @@ -176,6 +176,7 @@ namespace ScriptCanvas NamespacePath m_namespaces; const AZ::BehaviorMethod* m_method = nullptr; const AZ::BehaviorClass* m_class = nullptr; + AZStd::vector m_inputSlots; AZStd::vector m_resultSlotIDs; AZStd::recursive_mutex m_mutex; // post-serialization bool m_warnOnMissingFunction = true; diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Results/ErrorText.h b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Results/ErrorText.h index 339c0fc3a1..928d1d7202 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Results/ErrorText.h +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Results/ErrorText.h @@ -74,6 +74,7 @@ namespace ScriptCanvas constexpr const char* NotEnoughBranchesForReturn = "Not enough branches for defined out return values."; constexpr const char* NotEnoughInputForArithmeticOperator = "Not enough input for arithmetic operator"; constexpr const char* NullEntityInGraph = "Null entity pointer in graph"; + constexpr const char* NullInputKnown = "The input is known to be null, and the node does not accept it"; constexpr const char* ParseExecutionMultipleOutSyntaxSugarChildExecutionRemovedAndNotReplaced = "ParseExecutionMultipleOutSyntaxSugar: child execution node was removed, and not replaced."; constexpr const char* ParseExecutionMultipleOutSyntaxSugarMismatchOutSize = "ParseExecutionMultipleOutSyntaxSugar: mismatch in connect nodes vs source slots size"; constexpr const char* ParseExecutionMultipleOutSyntaxSugarNonNullChildExecutionFound = "ParseExecutionMultipleOutSyntaxSugar: non null child execution node"; diff --git a/Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseErrorOnKnownNull.scriptcanvas b/Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseErrorOnKnownNull.scriptcanvas new file mode 100644 index 0000000000..4453617996 --- /dev/null +++ b/Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseErrorOnKnownNull.scriptcanvas @@ -0,0 +1,947 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp b/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp index 107bb32cd6..538dfd2ef0 100644 --- a/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp +++ b/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp @@ -85,6 +85,16 @@ public: } }; +TEST_F(ScriptCanvasTestFixture, VerifySCUnitTestsRun) +{ + EXPECT_FALSE(true); // just convincing myself that these unit tests run on jenkins +} + +TEST_F(ScriptCanvasTestFixture, ParseErrorOnKnownNull) +{ + ExpectParseError("LY_SC_UnitTest_ParseErrorOnKnownNull"); +} + TEST_F(ScriptCanvasTestFixture, UseBehaviorContextClassConstant) { RunUnitTestGraph("LY_SC_UnitTest_UseBehaviorContextClassConstant");