Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix over-indexing of lists and restore string indexing #9388

Merged
merged 10 commits into from
Jan 11, 2019
4 changes: 4 additions & 0 deletions src/Engine/ProtoCore/FFI/CLRDLLModule.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1387,6 +1387,10 @@ public FFIMethodAttributes(MethodInfo method, Dictionary<MethodInfo, Attribute[]
{
IsLacingDisabled = true;
}
else if (attr is AllowArrayPromotionAttribute)
{
AllowArrayPromotion = (attr as AllowArrayPromotionAttribute).IsAllowed;
}
}
}

Expand Down
20 changes: 20 additions & 0 deletions src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -361,15 +361,31 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter

dsi.LogWarning(ProtoCore.Runtime.WarningID.InvalidArguments, msg);
}
else if (exc is System.ArgumentOutOfRangeException)
{
dsi.LogWarning(ProtoCore.Runtime.WarningID.IndexOutOfRange, ErrorString(exc));
}
else if (exc is System.ArgumentException)
{
dsi.LogWarning(ProtoCore.Runtime.WarningID.InvalidArguments, ErrorString(exc));
}
else if (exc is System.NullReferenceException)
{
dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(null));
}
else if (exc is System.IndexOutOfRangeException)
{
dsi.LogWarning(ProtoCore.Runtime.WarningID.OverIndexing, ErrorString(exc));
}
else
{
dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(exc));
}
}
else
{
dsi.LogWarning(ProtoCore.Runtime.WarningID.AccessViolation, ErrorString(ex));
}
}
catch (System.Reflection.TargetParameterCountException ex)
{
Expand Down Expand Up @@ -420,7 +436,9 @@ protected object InvokeFunctionPointerNoThrow(ProtoCore.Runtime.Context c, Inter
dsi.LogWarning(ProtoCore.Runtime.WarningID.InvalidArguments, ErrorString(ex.InnerException));
}
else
{
dsi.LogWarning(ProtoCore.Runtime.WarningID.InvalidArguments, ErrorString(ex));
}
}
catch (Exception ex)
{
Expand All @@ -440,9 +458,11 @@ private string ErrorString(System.Exception ex)
return ex.Message;

string msg = (ex == null) ? "" : ex.Message;

if (string.IsNullOrEmpty(msg) || msg.Contains("operation failed"))
return string.Format(Resources.OperationFailType1, ReflectionInfo.DeclaringType.Name, ReflectionInfo.Name);


return string.Format(Resources.OperationFailType2, ReflectionInfo.DeclaringType.Name, ReflectionInfo.Name, msg);
}

Expand Down
6 changes: 3 additions & 3 deletions src/Engine/ProtoCore/Lang/BuiltInFunctionEndPoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2514,11 +2514,11 @@ internal static StackValue SortPointers(StackValue svFunction, StackValue svArra
catch (System.Exception e)
{
if (e.InnerException is Exceptions.CompilerInternalException)
runtimeCore.RuntimeStatus.LogWarning(Runtime.WarningID.AurgumentIsNotExpected, Resources.FailedToResolveSortingFunction);
runtimeCore.RuntimeStatus.LogWarning(Runtime.WarningID.ArgumentIsNotExpected, Resources.FailedToResolveSortingFunction);
else if(e.InnerException != null)
runtimeCore.RuntimeStatus.LogWarning(Runtime.WarningID.AurgumentIsNotExpected, e.InnerException.Message);
runtimeCore.RuntimeStatus.LogWarning(Runtime.WarningID.ArgumentIsNotExpected, e.InnerException.Message);
else
runtimeCore.RuntimeStatus.LogWarning(Runtime.WarningID.AurgumentIsNotExpected, e.Message);
runtimeCore.RuntimeStatus.LogWarning(Runtime.WarningID.ArgumentIsNotExpected, e.Message);

return StackValue.Null;
}
Expand Down
11 changes: 11 additions & 0 deletions src/Engine/ProtoCore/Lang/FunctionEndPoint.cs
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,17 @@ public int ComputeTypeDistance(List<StackValue> args, ProtoCore.DSASM.ClassTable

public int GetConversionDistance(List<StackValue> reducedParamSVs, ProtoCore.DSASM.ClassTable classTable, bool allowArrayPromotion, RuntimeCore runtimeCore)
{
// If the replication strategy allows array promotion, first check for the case
// where it could be disabled using the [AllowArrayPromotion(false)] attribute
// and if so set it from the attribute.
if (allowArrayPromotion)
{
var ma = procedureNode.MethodAttribute;
if (ma != null)
{
allowArrayPromotion = ma.AllowArrayPromotion;
}
}
int dist = ComputeTypeDistance(reducedParamSVs, classTable, runtimeCore, allowArrayPromotion);
if (dist >= 0 && dist != (int)ProcedureDistance.MaxDistance) //Is it possible to convert to this type?
{
Expand Down
1 change: 1 addition & 0 deletions src/Engine/ProtoCore/Parser/AssociativeAST.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1663,6 +1663,7 @@ public IEnumerable<string> ReturnKeys
public string ObsoleteMessage { get; protected set; }
public bool IsObsolete { get { return !string.IsNullOrEmpty(ObsoleteMessage); } }
public bool IsLacingDisabled { get; protected set; }
public bool AllowArrayPromotion { get; protected set; } = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aparajit-pratap should this also be added to the imperative AST methodAttributes class - does that exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking this class doesn't have anything to do with Associative AST's and should not be in this file but that's a consideration for another day.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆


/// <summary>
/// Returns/Sets description for the method.
Expand Down
4 changes: 2 additions & 2 deletions src/Engine/ProtoCore/RuntimeStatus.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public enum WarningID
Default,
AccessViolation,
AmbiguousMethodDispatch,
AurgumentIsNotExpected,
ArgumentIsNotExpected,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aparajit-pratap I don't think we can change these now, they are public. 😢 maybe just add a todo?

CallingConstructorOnInstance,
ConversionNotPossible,
DereferencingNonPointer,
Expand All @@ -29,7 +29,7 @@ public enum WarningID
CyclicDependency,
MethodResolutionFailure,
OverIndexing,
TypeConvertionCauseInfoLoss,
TypeConversionCauseInfoLoss,
TypeMismatch,
ReplicationWarning,
InvalidIndexing,
Expand Down
12 changes: 12 additions & 0 deletions src/Libraries/DesignScriptBuiltin/Builtin.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public static object ValueAtIndex(Dictionary dictionary, string key)
return dictionary.ValueAtKey(key);
}

[AllowArrayPromotion(false)]
public static object ValueAtIndex(IList list, int index)
{
while (index < 0)
Expand All @@ -29,6 +30,17 @@ public static object ValueAtIndex(IList list, int index)
}
return list[index];
}

public static object ValueAtIndex(string stringList, int index)
{
while (index < 0)
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
{
var count = stringList.Length;
if (count == 0) break;
index += count;
}
return stringList[index];
}
}
}
}
22 changes: 22 additions & 0 deletions src/NodeServices/Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -342,4 +342,26 @@ public class KeepReferenceThisAttribute : Attribute
public class IsLacingDisabledAttribute : Attribute
{
}

/// <summary>
/// This attribute is applied to a function that has one or more parameters
/// as lists. It can be used to control arguments to the function
/// from being promoted to arrays or arrays of higher dimension when the VM tries
/// to do method resolution and match argument(s) to the function parameter(s).
/// </summary>
[AttributeUsage(AttributeTargets.Method)]
public class AllowArrayPromotionAttribute : Attribute
{
public bool IsAllowed { get; private set; }

public AllowArrayPromotionAttribute()
{
IsAllowed = true;
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
}

public AllowArrayPromotionAttribute(bool isAllowed)
{
IsAllowed = isAllowed;
}
}
}
2 changes: 1 addition & 1 deletion test/DynamoCoreTests/CodeBlockNodeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1558,7 +1558,7 @@ public void TestImperativePropertyAccessing()
AssertPreviewValue("39c65660-8575-43bc-8af7-f24225a6bd5b", 21);
}

[Test, Category("Failure")]
[Test]
[Ignore("Test Loops Forever. Danger.")]
public void TestImperativeLanguageBlock()
{
Expand Down
12 changes: 9 additions & 3 deletions test/DynamoCoreTests/DSCoreDataTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,16 @@ public void StringifyJSON()
NodeModel testNode = workspace.NodeFromWorkspace(testNodeGuid);

// Expected parsed types
List<string> expectedOutputs = Enumerable.Repeat(typeof(System.String).FullName, 8).ToList();

var expectedString = typeof(System.String).FullName;
var expectedChar = typeof(System.Char).FullName;

// Verify node output types match expected output
AssertPreviewValue(testNode.GUID.ToString(), expectedOutputs);
AssertPreviewValue(testNode.GUID.ToString(), new[]
{
expectedString, expectedString, expectedChar, expectedString, expectedString, expectedString,
expectedString, expectedString

});
}

[Test]
Expand Down
4 changes: 2 additions & 2 deletions test/DynamoCoreTests/MultiOutputNodeTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ class MultiOutputNodeTest : DynamoModelTestBase
{
protected override void GetLibrariesToPreload(List<string> libraries)
{
libraries.Add("DesignScriptBuiltin.dll");
libraries.Add("FFITarget.dll");
base.GetLibrariesToPreload(libraries);
}

[Test, Category("Failure")]
[Test]
public void TestSingleOutputNode()
{
// TODO pratapa: Test goes into infinite loop after Dictionary changes
RunModel(@"core\multiout\singleoutput.dyn");
AssertPreviewValue("060e57e1-b889-4b94-a440-8adb0067ae79", null);

Expand Down
2 changes: 1 addition & 1 deletion test/Engine/ProtoTest/Associative/BuiltinMethodsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ public void BIM16_Insert()
s = Insert(a,e,5);
u = p[1];
v = q[1][0];
w = r[1][0];
w = r[1];
x = s[5][0][0];
";
ExecutionMirror mirror = thisTest.RunScriptSource(code);
Expand Down
39 changes: 2 additions & 37 deletions test/Engine/ProtoTest/Associative/MicroFeatureTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -653,11 +653,9 @@ def foo()
thisTest.Verify("y", 2);
}

[Test, Category("Failure")]
[Test]
public void TestArrayOverIndexing01()
{
// TODO pratapa: Zero sub-indexing of array now works due to array promotion
// after introducing Builtin.Get.ValueAtIndex for indexing operator
string code = @"
[Imperative]
{
Expand All @@ -668,7 +666,7 @@ public void TestArrayOverIndexing01()
}
";
thisTest.RunScriptSource(code);
TestFrameWork.VerifyRuntimeWarning(ProtoCore.Runtime.WarningID.OverIndexing);
TestFrameWork.VerifyRuntimeWarning(ProtoCore.Runtime.WarningID.MethodResolutionFailure);
}

[Test]
Expand Down Expand Up @@ -1265,39 +1263,6 @@ public void TestDictionaryRegressMAGN619()
thisTest.Verify("r", 5);
}

[Test, Category("Failure")]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test doesn't make any sense.

public void TestDictionary25()
{
// TODO pratapa: Crash typing this code in CBN post Dictionary changes
string code = @"
a = [];
x = ""key"";
a[x] = 42;

y = ""key"";
a[y] = 24;

z = ""key"";
a[z] = 12;

r1 = a[x];
r2 = a[y];
r3 = a[z];
r4 = a[""key""];

a[""key""] = 1;
r5 = a[""key""];
r6 = a[x];
r7 = a[y];
r8 = a[z];
";
thisTest.RunScriptSource(code);
thisTest.Verify("r5", 1);
thisTest.Verify("r6", 1);
thisTest.Verify("r7", 1);
thisTest.Verify("r8", 1);
}

[Test]
public void TestArrayCopyAssignment01()
{
Expand Down
15 changes: 5 additions & 10 deletions test/Engine/ProtoTest/Imperative/MicroFeatureTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ public void ForLoop04()
@"x=[Imperative]
{
x = 0;
for (val in 10)
for (val in [10])
{
x = x + val;
}
Expand All @@ -444,7 +444,7 @@ public void ForLoop05()
{
y = 0;
b = 11;
for (val in b)
for (val in [b])
{
y = y + val;
}
Expand Down Expand Up @@ -1268,22 +1268,18 @@ constructor B(_m : int, _n : int)
thisTest.Verify("i", new[] {2, 3, 3, -3});
}

[Test, Category("Failure")]
[Test]
public void TestArrayOverIndexing01()
{
// TODO pratapa: Zero sub-indexing of array now works due to array promotion
// after introducing Builtin.Get.ValueAtIndex for indexing operator
string code = @"
[Imperative]
{
arr1 = [true, false];
arr2 = [1, 2, 3];
arr3 = [false, true];
t = arr2[1][0];
}
";
thisTest.RunScriptSource(code);
TestFrameWork.VerifyRuntimeWarning(ProtoCore.Runtime.WarningID.OverIndexing);
TestFrameWork.VerifyRuntimeWarning(ProtoCore.Runtime.WarningID.MethodResolutionFailure);
}

[Test]
Expand Down Expand Up @@ -1460,10 +1456,9 @@ public void TestStringTypeConversion()
thisTest.Verify("i", new object[] {true, true, Convert.ToInt64('h') + 1});
}

[Test, Category("Failure")]
[Test]
public void TestStringForloop()
{
// TODO pratapa: Regression: Strings can no longer be indexed into
string code =
@"
r = [Imperative]
Expand Down
3 changes: 1 addition & 2 deletions test/Engine/ProtoTest/TD/Associative/Update.cs
Original file line number Diff line number Diff line change
Expand Up @@ -228,11 +228,10 @@ public void T006_Update_In_Class()
}

[Test]
[Category("DSDefinedClass_Ported"), Category("Failure")]
[Category("DSDefinedClass_Ported")]
[Category("SmokeTest")]
public void T008_Update_Of_Variables()
{
// TODO pratapa: Regression after introduction of Get.ValueAtIndex method for array indexing
string code = @"
a = 1;
b = a + 1;
Expand Down
Loading