Skip to content
This repository has been archived by the owner on Jan 22, 2022. It is now read-only.

Commit

Permalink
Fix issue with recursion on binary short circuit operators in some cases
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
MerlinVR committed Feb 26, 2021
1 parent c46d941 commit b560696
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 119 deletions.
2 changes: 1 addition & 1 deletion Assets/UdonSharp/Editor/UdonSharpASTVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
118 changes: 0 additions & 118 deletions Assets/UdonSharp/Tests/IntegrationTestScene.unity
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions Assets/UdonSharp/Tests/TestScripts/FlowControl/RecursionTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,23 @@ public void CustomEventRecursion()
customEventCounter = originalCounter * customEventCounter;
}

/// <summary>
/// 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
/// </summary>
[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()
{
Expand Down Expand Up @@ -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);
}
}
}

0 comments on commit b560696

Please sign in to comment.