From b560696b8267cbc8eba08397b4ada3a65bd87829 Mon Sep 17 00:00:00 2001 From: Merlin <36685500+MerlinVR@users.noreply.github.com> Date: Thu, 25 Feb 2021 17:04:39 -0800 Subject: [PATCH] Fix issue with recursion on binary short circuit operators in some cases - Fix issue where the intermediate value for the left hand side of the binary expression could get stomped by recursive calls which would result in an incorrect final evaluation of the expression if it's called recursively. Reported by Genesis - Add test for this case --- .../UdonSharp/Editor/UdonSharpASTVisitor.cs | 2 +- .../Tests/IntegrationTestScene.unity | 118 ------------------ .../TestScripts/FlowControl/RecursionTest.cs | 19 +++ 3 files changed, 20 insertions(+), 119 deletions(-) diff --git a/Assets/UdonSharp/Editor/UdonSharpASTVisitor.cs b/Assets/UdonSharp/Editor/UdonSharpASTVisitor.cs index cf2bc88e..2963823f 100644 --- a/Assets/UdonSharp/Editor/UdonSharpASTVisitor.cs +++ b/Assets/UdonSharp/Editor/UdonSharpASTVisitor.cs @@ -1338,7 +1338,7 @@ private void HandleBinaryShortCircuitConditional(BinaryExpressionSyntax node) JumpLabel rhsEnd = visitorContext.labelTable.GetNewJumpLabel("conditionalShortCircuitEnd"); - SymbolDefinition resultValue = visitorContext.topTable.CreateUnnamedSymbol(typeof(bool), SymbolDeclTypeFlags.Internal); + SymbolDefinition resultValue = visitorContext.topTable.CreateUnnamedSymbol(typeof(bool), SymbolDeclTypeFlags.Internal | SymbolDeclTypeFlags.NeedsRecursivePush); using (ExpressionCaptureScope lhsCaptureScope = new ExpressionCaptureScope(visitorContext, null)) { diff --git a/Assets/UdonSharp/Tests/IntegrationTestScene.unity b/Assets/UdonSharp/Tests/IntegrationTestScene.unity index 1ea10430..87e0a326 100644 --- a/Assets/UdonSharp/Tests/IntegrationTestScene.unity +++ b/Assets/UdonSharp/Tests/IntegrationTestScene.unity @@ -1759,36 +1759,6 @@ Transform: m_Father: {fileID: 1061635231} m_RootOrder: 1 m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} ---- !u!1 &880159132 -GameObject: - m_ObjectHideFlags: 0 - m_CorrespondingSourceObject: {fileID: 0} - m_PrefabInstance: {fileID: 0} - m_PrefabAsset: {fileID: 0} - serializedVersion: 6 - m_Component: - - component: {fileID: 880159133} - m_Layer: 0 - m_Name: GameObject (1) - m_TagString: Untagged - m_Icon: {fileID: 0} - m_NavMeshLayer: 0 - m_StaticEditorFlags: 0 - m_IsActive: 1 ---- !u!4 &880159133 -Transform: - m_ObjectHideFlags: 0 - m_CorrespondingSourceObject: {fileID: 0} - m_PrefabInstance: {fileID: 0} - m_PrefabAsset: {fileID: 0} - m_GameObject: {fileID: 880159132} - m_LocalRotation: {x: 0, y: 0, z: 0, w: 1} - m_LocalPosition: {x: 0, y: 0, z: 0} - m_LocalScale: {x: 1, y: 1, z: 1} - m_Children: [] - m_Father: {fileID: 921714181} - m_RootOrder: 1 - m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} --- !u!1 &908833310 GameObject: m_ObjectHideFlags: 0 @@ -1870,94 +1840,6 @@ Transform: m_Father: {fileID: 1722523552} m_RootOrder: 7 m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} ---- !u!1 &920481274 -GameObject: - m_ObjectHideFlags: 0 - m_CorrespondingSourceObject: {fileID: 0} - m_PrefabInstance: {fileID: 0} - m_PrefabAsset: {fileID: 0} - serializedVersion: 6 - m_Component: - - component: {fileID: 920481275} - m_Layer: 0 - m_Name: GameObject - m_TagString: Untagged - m_Icon: {fileID: 0} - m_NavMeshLayer: 0 - m_StaticEditorFlags: 0 - m_IsActive: 1 ---- !u!4 &920481275 -Transform: - m_ObjectHideFlags: 0 - m_CorrespondingSourceObject: {fileID: 0} - m_PrefabInstance: {fileID: 0} - m_PrefabAsset: {fileID: 0} - m_GameObject: {fileID: 920481274} - m_LocalRotation: {x: 0, y: 0, z: 0, w: 1} - m_LocalPosition: {x: 0, y: 0, z: 0} - m_LocalScale: {x: 1, y: 1, z: 1} - m_Children: [] - m_Father: {fileID: 921714181} - m_RootOrder: 0 - m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} ---- !u!1 &921714179 -GameObject: - m_ObjectHideFlags: 0 - m_CorrespondingSourceObject: {fileID: 0} - m_PrefabInstance: {fileID: 0} - m_PrefabAsset: {fileID: 0} - serializedVersion: 6 - m_Component: - - component: {fileID: 921714181} - - component: {fileID: 921714180} - m_Layer: 0 - m_Name: EventsTest - m_TagString: Untagged - m_Icon: {fileID: 0} - m_NavMeshLayer: 0 - m_StaticEditorFlags: 0 - m_IsActive: 0 ---- !u!114 &921714180 -MonoBehaviour: - m_ObjectHideFlags: 0 - m_CorrespondingSourceObject: {fileID: 0} - m_PrefabInstance: {fileID: 0} - m_PrefabAsset: {fileID: 0} - m_GameObject: {fileID: 921714179} - m_Enabled: 1 - m_EditorHideFlags: 0 - m_Script: {fileID: 11500000, guid: 45115577ef41a5b4ca741ed302693907, type: 3} - m_Name: - m_EditorClassIdentifier: - interactTextPlacement: {fileID: 0} - interactText: Use - interactTextGO: {fileID: 0} - proximity: 2 - SynchronizePosition: 0 - AllowCollisionOwnershipTransfer: 0 - serializedProgramAsset: {fileID: 11400000, guid: 5afc9522565b1dc4a8221b17f1586999, - type: 2} - programSource: {fileID: 11400000, guid: 4945ca52571b9a240a2a1ac6eba96994, type: 2} - serializedPublicVariablesBytesString: Ai8AAAAAATIAAABWAFIAQwAuAFUAZABvAG4ALgBDAG8AbQBtAG8AbgAuAFUAZABvAG4AVgBhAHIAaQBhAGIAbABlAFQAYQBiAGwAZQAsACAAVgBSAEMALgBVAGQAbwBuAC4AQwBvAG0AbQBvAG4AAAAAAAYBAAAAAAAAACcBBAAAAHQAeQBwAGUAAWgAAABTAHkAcwB0AGUAbQAuAEMAbwBsAGwAZQBjAHQAaQBvAG4AcwAuAEcAZQBuAGUAcgBpAGMALgBMAGkAcwB0AGAAMQBbAFsAVgBSAEMALgBVAGQAbwBuAC4AQwBvAG0AbQBvAG4ALgBJAG4AdABlAHIAZgBhAGMAZQBzAC4ASQBVAGQAbwBuAFYAYQByAGkAYQBiAGwAZQAsACAAVgBSAEMALgBVAGQAbwBuAC4AQwBvAG0AbQBvAG4AXQBdACwAIABtAHMAYwBvAHIAbABpAGIAAQEJAAAAVgBhAHIAaQBhAGIAbABlAHMALwEAAAABaAAAAFMAeQBzAHQAZQBtAC4AQwBvAGwAbABlAGMAdABpAG8AbgBzAC4ARwBlAG4AZQByAGkAYwAuAEwAaQBzAHQAYAAxAFsAWwBWAFIAQwAuAFUAZABvAG4ALgBDAG8AbQBtAG8AbgAuAEkAbgB0AGUAcgBmAGEAYwBlAHMALgBJAFUAZABvAG4AVgBhAHIAaQBhAGIAbABlACwAIABWAFIAQwAuAFUAZABvAG4ALgBDAG8AbQBtAG8AbgBdAF0ALAAgAG0AcwBjAG8AcgBsAGkAYgABAAAABgIAAAAAAAAAAi8CAAAAAWEAAABWAFIAQwAuAFUAZABvAG4ALgBDAG8AbQBtAG8AbgAuAFUAZABvAG4AVgBhAHIAaQBhAGIAbABlAGAAMQBbAFsAVQBuAGkAdAB5AEUAbgBnAGkAbgBlAC4ARwBhAG0AZQBPAGIAagBlAGMAdAAsACAAVQBuAGkAdAB5AEUAbgBnAGkAbgBlAC4AQwBvAHIAZQBNAG8AZAB1AGwAZQBdAF0ALAAgAFYAUgBDAC4AVQBkAG8AbgAuAEMAbwBtAG0AbwBuAAIAAAAGAgAAAAAAAAAnAQQAAAB0AHkAcABlAAEXAAAAUwB5AHMAdABlAG0ALgBTAHQAcgBpAG4AZwAsACAAbQBzAGMAbwByAGwAaQBiACcBCgAAAFMAeQBtAGIAbwBsAE4AYQBtAGUAAQsAAABvAHQAaABlAHIATwBiAGoAZQBjAHQAJwEEAAAAdAB5AHAAZQABLgAAAFUAbgBpAHQAeQBFAG4AZwBpAG4AZQAuAEcAYQBtAGUATwBiAGoAZQBjAHQALAAgAFUAbgBpAHQAeQBFAG4AZwBpAG4AZQAuAEMAbwByAGUATQBvAGQAdQBsAGUACwEFAAAAVgBhAGwAdQBlAAAAAAAHBQIvAwAAAAFXAAAAVgBSAEMALgBVAGQAbwBuAC4AQwBvAG0AbQBvAG4ALgBVAGQAbwBuAFYAYQByAGkAYQBiAGwAZQBgADEAWwBbAFQATQBQAHIAbwAuAFQAZQB4AHQATQBlAHMAaABQAHIAbwAsACAAVQBuAGkAdAB5AC4AVABlAHgAdABNAGUAcwBoAFAAcgBvAF0AXQAsACAAVgBSAEMALgBVAGQAbwBuAC4AQwBvAG0AbQBvAG4AAwAAAAYCAAAAAAAAACcBBAAAAHQAeQBwAGUAARcAAABTAHkAcwB0AGUAbQAuAFMAdAByAGkAbgBnACwAIABtAHMAYwBvAHIAbABpAGIAJwEKAAAAUwB5AG0AYgBvAGwATgBhAG0AZQABCAAAAHQAZQB4AHQATQBlAHMAaAAnAQQAAAB0AHkAcABlAAEXAAAAUwB5AHMAdABlAG0ALgBPAGIAagBlAGMAdAAsACAAbQBzAGMAbwByAGwAaQBiAC0BBQAAAFYAYQBsAHUAZQAHBQcFBwU= - publicVariablesUnityEngineObjects: - - {fileID: 1478963619} - publicVariablesSerializationDataFormat: 0 ---- !u!4 &921714181 -Transform: - m_ObjectHideFlags: 0 - m_CorrespondingSourceObject: {fileID: 0} - m_PrefabInstance: {fileID: 0} - m_PrefabAsset: {fileID: 0} - m_GameObject: {fileID: 921714179} - m_LocalRotation: {x: 0, y: 0, z: 0, w: 1} - m_LocalPosition: {x: 0, y: 0.24, z: 0} - m_LocalScale: {x: 1, y: 1, z: 1} - m_Children: - - {fileID: 920481275} - - {fileID: 880159133} - m_Father: {fileID: 0} - m_RootOrder: 6 - m_LocalEulerAnglesHint: {x: 0, y: 0, z: 0} --- !u!1 &987059980 GameObject: m_ObjectHideFlags: 0 diff --git a/Assets/UdonSharp/Tests/TestScripts/FlowControl/RecursionTest.cs b/Assets/UdonSharp/Tests/TestScripts/FlowControl/RecursionTest.cs index 6a2decea..a1c4892b 100644 --- a/Assets/UdonSharp/Tests/TestScripts/FlowControl/RecursionTest.cs +++ b/Assets/UdonSharp/Tests/TestScripts/FlowControl/RecursionTest.cs @@ -230,6 +230,23 @@ public void CustomEventRecursion() customEventCounter = originalCounter * customEventCounter; } + /// + /// Tests if the temp value from the lhs of the short circuit gets stomped by the recursive call which causes the final expression to evaluate an inaccurate value + /// + [RecursiveMethod] + bool ShortCircuitRecursion(bool a, int count, bool useValue) + { + if (count == 0) + a = false; + + bool result = a == false || !ShortCircuitRecursion(a, count - 1, false); + + if (useValue) + return result; + + return true; + } + [RecursiveMethod] // Just here to test calling out to other types from recursive methods public void ExecuteTests() { @@ -277,6 +294,8 @@ public void ExecuteTests() customEventCounter = 4; CustomEventRecursion(); tester.TestAssertion("SendCustomEvent recursion", customEventCounter == 24); + + tester.TestAssertion("Recursive short circuit operators", ShortCircuitRecursion(true, 2, true) == false); } } }