DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken #9542

scottmitchell · 2019-03-05T19:42:36Z

Purpose

Jira issue DYN-1476.

This PR addresses a bug where the AllowRankReductionAttribute does not work for Zero Touch properties. The typical use case for the AllowRankReductionAttribute is to reduce the rank of the return value of methods—rather than return a list of a single item, the method will return just the item. This does not currently work for properties because a property's attributes are not automatically applied to that property's getter (which is what actually get's invoked by the Zero Touch node). For example, this does not currently result in rank reduction of the get_SomeProperty return value:

[AllowRankReduction] // This won't have any effect
public List<object> SomeProperty => new List<object> { someObject };

A work around is possible by explicitly declaring the getter with AllowRankReductionAttribute:

[AllowRankReduction]
public List<object> SomeProperty2
{
    [AllowRankReduction] // Adding this one will make it work
    get
    {
         return new List<object> { someObject };
    }
}

There are a couple possible solutions here:

Solution 1: Change nothing, and simply message to users that they must explicitly declare getters if they want to use this attribute. However, this behavior could be confusing to a Zero Touch developer.

Solution 2: Copy the attribute from the property to its getter at runtime—that could happen here. This may be possible, but certainly isn't trivial. Some info on that here: https://stackoverflow.com/questions/14663763/how-to-add-an-attribute-to-a-property-at-runtime

Solution 3: Add another rank reduction check later in the function invoke process, where the property attributes are available—maybe here.

Solution 4 (this PR): Create a way to pass the property's AllowRankReductionAttribute down to the FFIMemberInfo instance created from the getter, without actually adding the attribute to the getter. This seems to be the least invasive way to add this functionality to Zero Touch properties. However, it may not be the cleanest.

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@aparajit-pratap @mjkkirschner

FYIs

@QilongTang

…d debug menu. Deleted debug menu items.

src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs

aparajit-pratap · 2019-03-05T20:42:25Z

@scottmitchell I think this is a clean and simple solution! I would "LGTM" this after some tests :)
Thanks!

src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs

mjkkirschner · 2019-03-05T21:17:19Z

src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs

@@ -64,6 +64,26 @@ protected FFIMemberInfo(MemberInfo info)
            Info = info;
        }

+        internal void CheckForRankReductionAttribute(Dictionary<MethodInfo, Attribute[]> getterAttributes)


name is okay to me, but just considering others - copyRankReductionAttribute?

Would you add some function comments, what you have mentioned in this PR description is good enough

@QilongTang yes, I'll add some comments. @mjkkirschner yeah I like that better. Or maybe "AttachRankReductionAttribute"

mjkkirschner · 2019-03-05T21:17:43Z

it looks good to me, lets add some tests.

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

QilongTang · 2019-03-05T21:44:20Z

LGTM with a few comments and looking forward to tests

aparajit-pratap · 2019-03-06T15:51:57Z

@scottmitchell you can add dummy test classes and methods/properties to FFITarget project. It's a test zero touch library we use in our FFI (Foreign Function Interface) tests.

scottmitchell · 2019-03-06T18:09:11Z

Thanks @aparajit-pratap, I was going to ask about that

aparajit-pratap · 2019-03-07T17:17:11Z

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

@@ -784,7 +784,7 @@ private void RegisterFunctionPointer(string functionName, MemberInfo method, Lis
            else if (CoreUtils.IsGetter(functionName))
            {
                f = new GetterFunctionPointer(Module, functionName, method, retype);
-                (f as GetterFunctionPointer).ReflectionInfo.CheckForRankReductionAttribute(mGetterAttributes);
+                (f as GetterFunctionPointer).ReflectionInfo.CopyRankReductionAttribute(mGetterAttributes);


Sorry, but I think the earlier name was more appropriate considering the context in which it is used. It first checks and then only copies.

aparajit-pratap · 2019-03-07T17:27:07Z

test/Engine/FFITarget/TestObjects.cs

@@ -457,4 +457,35 @@ public void Dispose()
        }
    }

+    public class TestRankReduce : IDisposable


You don't need to make this class an IDisposable.

did you intend to dispose this class?

I only implemented IDisposable because the other test objects in this file did. I didn't really have any intentions for it.

aparajit-pratap · 2019-03-07T17:27:39Z

test/Engine/FFITarget/TestObjects.cs

+        private string RankReduceTestField;
+
+        [AllowRankReduction]
+        public List<string> RankReduceArrowProperty => new List<string> { RankReduceTestField };


How is this syntax different from the RankReducePropertyGetter?

It's just syntactic sugar for the getter. Testing one of these would suffice.

Fair enough. I just carried this over from the test cases I used during development. I removed the arrow property and added a method test (not sure if this exists anywhere else).

aparajit-pratap · 2019-03-07T17:29:22Z

@scottmitchell some minor comments and then LGTM!

mjkkirschner · 2019-03-07T17:29:42Z

why are the builds failing here?

QilongTang · 2019-03-07T17:32:35Z

@mjkkirschner The console shows all clear 18 smoke tests pass and all good, ever see this happens?

mjkkirschner · 2019-03-07T18:05:03Z

all checks clear 👍

scottmitchell · 2019-03-07T18:30:50Z

@aparajit-pratap @mjkkirschner @QilongTang Thanks for your alls comments. I think I've addressed everything—Let me know if I haven't. Otherwise, I'll go ahead and merge once the checks pass.

mjkkirschner · 2019-03-07T18:59:54Z

@scottmitchell please send a cherry pick if this is labeled 2.2

DynamoDS#9542) * update * Brought truncated search results and full query search out from behind debug menu. Deleted debug menu items. * Added CheckForRankReductionAttribute() method to FFIMemberInfo * Cleaned up CheckForRankReductionAttribute * Added AllowRankReductionAttribute tests * Comments. Change method name. * Update tests, method name * Corrected test names * Fixed annoying diff

…rties (#9542) (#9549) * DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken (#9542) * update * Brought truncated search results and full query search out from behind debug menu. Deleted debug menu items. * Added CheckForRankReductionAttribute() method to FFIMemberInfo * Cleaned up CheckForRankReductionAttribute * Added AllowRankReductionAttribute tests * Comments. Change method name. * Update tests, method name * Corrected test names * Fixed annoying diff * DYN-1476 Follow Up: Remove Rank Reduction from Dictionary.Values (#9550) * update * Brought truncated search results and full query search out from behind debug menu. Deleted debug menu items. * Removed AllowRankReduction from Dictionary.Values. Added TODO

scottmitchell added 9 commits November 26, 2018 15:22

update

13a71a7

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

83563aa

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

0e6bac9

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

f5cd9b8

Brought truncated search results and full query search out from behin…

3d2a126

…d debug menu. Deleted debug menu items.

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

2434a2a

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

552a678

Added CheckForRankReductionAttribute() method to FFIMemberInfo

8340042

pull master

425e933

scottmitchell requested review from aparajit-pratap and mjkkirschner March 5, 2019 19:42

aparajit-pratap reviewed Mar 5, 2019

View reviewed changes

src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Mar 5, 2019

View reviewed changes

src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs Outdated Show resolved Hide resolved

aparajit-pratap reviewed Mar 5, 2019

View reviewed changes

src/Engine/ProtoCore/FFI/CLRFFIFunctionPointer.cs Outdated Show resolved Hide resolved

Cleaned up CheckForRankReductionAttribute

c29e668

mjkkirschner reviewed Mar 5, 2019

View reviewed changes

QilongTang reviewed Mar 5, 2019

View reviewed changes

src/Engine/ProtoCore/FFI/CLRDLLModule.cs Show resolved Hide resolved

scottmitchell added 2 commits March 7, 2019 09:47

Added AllowRankReductionAttribute tests

e20028a

Comments. Change method name.

263d925

aparajit-pratap reviewed Mar 7, 2019

View reviewed changes

scottmitchell added 3 commits March 7, 2019 13:07

Update tests, method name

c08b5c8

Corrected test names

e537687

Fixed annoying diff

818c8ed

QilongTang added the LGTM Looks good to me label Mar 7, 2019

QilongTang changed the title ~~[WIP] [PTAL] DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken~~ DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken Mar 7, 2019

scottmitchell merged commit 0dbc0cc into DynamoDS:master Mar 7, 2019

scottmitchell mentioned this pull request Mar 7, 2019

Cherry Pick DYN-1476: AllowRankReductionAttribute on Zero Touch Properties (#9542) #9549

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken #9542

DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken #9542

scottmitchell commented Mar 5, 2019

aparajit-pratap commented Mar 5, 2019

mjkkirschner Mar 5, 2019

QilongTang Mar 5, 2019

scottmitchell Mar 6, 2019

mjkkirschner commented Mar 5, 2019 •

edited

Loading

QilongTang commented Mar 5, 2019

aparajit-pratap commented Mar 6, 2019

scottmitchell commented Mar 6, 2019

aparajit-pratap Mar 7, 2019 •

edited

Loading

aparajit-pratap Mar 7, 2019

mjkkirschner Mar 7, 2019

scottmitchell Mar 7, 2019

aparajit-pratap Mar 7, 2019

aparajit-pratap Mar 7, 2019

scottmitchell Mar 7, 2019

aparajit-pratap commented Mar 7, 2019

mjkkirschner commented Mar 7, 2019

QilongTang commented Mar 7, 2019

mjkkirschner commented Mar 7, 2019

scottmitchell commented Mar 7, 2019

mjkkirschner commented Mar 7, 2019

DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken #9542

DYN-1476: AllowRankReductionAttribute on Zero Touch seems to be broken #9542

Conversation

scottmitchell commented Mar 5, 2019

Purpose

Declarations

Reviewers

FYIs

aparajit-pratap commented Mar 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Mar 5, 2019 • edited Loading

QilongTang commented Mar 5, 2019

aparajit-pratap commented Mar 6, 2019

scottmitchell commented Mar 6, 2019

aparajit-pratap Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap commented Mar 7, 2019

mjkkirschner commented Mar 7, 2019

QilongTang commented Mar 7, 2019

mjkkirschner commented Mar 7, 2019

scottmitchell commented Mar 7, 2019

mjkkirschner commented Mar 7, 2019

mjkkirschner commented Mar 5, 2019 •

edited

Loading

aparajit-pratap Mar 7, 2019 •

edited

Loading