[DYN-2588] Enable ArbitraryDimensionArrayImport attribute to function return values, apply this to Dictionary.Values getter #13055

aparajit-pratap · 2022-06-21T22:58:59Z

Purpose

Fixes a case like this that would fail. It now works as expected.

This introduces an API break but I think it's a necessary fix. ~~Alternatively, we can add a new property with a different but similar name to Values and deprecate Values.~~

Declarations

The codebase 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.
This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

@saintentropy

FYIs

@Amoursol what do you think?

Amoursol · 2022-06-22T15:33:31Z

@aparajit-pratap the API break would only be negative if a user was relying on the failure in this case, correct? If so, that should be little to no exposure, and the benefit should outweigh it.

aparajit-pratap · 2022-06-22T16:34:53Z

@aparajit-pratap the API break would only be negative if a user was relying on the failure in this case, correct? If so, that should be little to no exposure, and the benefit should outweigh it.

@Amoursol sorry, I wasn't clear with my question. I think there might be other consequences of the API break such as user code relying on this property and finding it to be broken as well. However, what I wanted from you was, if you were okay with the idea of introducing a new node with a different name than Values that we could expose while deprecating Values if we decided we should avoid the API break at all costs.

Amoursol · 2022-06-22T17:07:08Z

@aparajit-pratap the API break would only be negative if a user was relying on the failure in this case, correct? If so, that should be little to no exposure, and the benefit should outweigh it.

@Amoursol sorry, I wasn't clear with my question. I think there might be other consequences of the API break such as user code relying on this property and finding it to be broken as well. However, what I wanted from you was, if you were okay with the idea of introducing a new node with a different name than Values that we could expose while deprecating Values if we decided we should avoid the API break at all costs.

@aparajit-pratap preference would be to keep "Values" as it correlates with the output port names from the other nodes and may cause confusion in the userbase, but yes, if we deem is necessary to deprecate we can find an alternative name.

sm6srw

LGTM!

aparajit-pratap · 2022-06-23T16:15:33Z

LGTM!

@sm6srw any idea how severe is the API break if a new attribute is added to an API, in this case, the public property? Will user code need to be rebuilt?

sm6srw · 2022-06-23T17:23:31Z

Looks like it is API breaking according to https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net. I assume a recompile is needed? This is not something I have run into.

…ly to Dictionary.Values

aparajit-pratap · 2022-06-24T23:15:35Z

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

-            }
-
-            return false;
-        }


aparajit-pratap · 2022-06-24T23:21:33Z

@Amoursol @sm6srw I have avoided the behaviour change by removing the AllowRankReduction attribute. Instead, I applied the ArbitraryDimensionArrayImport attribute to the return value of Dictionary.Values that fixes the issue as well as does not change the original behaviour, aka, a list of a single value is still returned as a list of a single value.

Amoursol · 2022-06-25T17:47:01Z

@Amoursol @sm6srw I have avoided the behaviour change by removing the AllowRankReduction attribute. Instead, I applied the ArbitraryDimensionArrayImport attribute to the return value of Dictionary.Values that fixes the issue as well as does not change the original behaviour, aka, a list of a single value is still returned as a list of a single value.

Brilliant, thanks @aparajit-pratap! This should safely allow us to make this change.

…dd test

sm6srw

I still try to understand exactly how this work but LGTM!

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

sm6srw · 2022-06-27T14:33:49Z

src/Libraries/DesignScriptBuiltin/Dictionary.cs

-            public IEnumerable<object> Values => D.Values;
+            public IEnumerable<object> Values
+            {
+                [return: ArbitraryDimensionArrayImport]


I have not seen this before. Can you give me some hints about how this is different for the previous solution?

ArbitraryDimensionArrayImport and AllowRankReduction attributes are slightly different in behaviour. The first one can be used for function parameters and return values and allows arbitrary ranked arrays and mixed rank arrays from being passed and returned to and from functions. The latter attribute can only be applied to return value of a function. It returns a single value in the case that a function returning a list returns a list of a single value (reduces the rank of a return value).

The thing that both attributes are similar in is that both of them convert the value to arbitrary rank, if it is an array (and if it's an array containing more than one value in the case of AllowRankReduction).

In this case, we don't want to change the behaviour of Dictionary.Values as far as possible, so we don't want to use AllowRankReduction but use ArbitraryArrayDimensionImport attribute instead so we can support returning arbitrarily ranked arrays.

ArbitraryArrayDimensionImport in the past even though it could be applied to function return values and function parameters, we didn't add support to make it handle return values. So, in this PR, I've added that support as well as applied it to Dictionary.Values.

Sounds good. And this approach will not break the API?

No idea but from my testing, it doesn't appear to break the API. I tested it with another assembly that referenced Dictionary.Values and I didn't need to rebuild it. However, @mjkkirschner would know better as he's better with this kind of stuff.

I think this is fine, can you name the syntax though? so I can research further- my googling is coming up short - [return: ArbitraryDimensionArrayImport]

oh, I see, it's an attribute only of the return value, not the method.

mjkkirschner · 2022-06-27T15:59:55Z

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

+                var returnTypeAtts = minfo.ReturnTypeCustomAttributes;
+                if (returnTypeAtts != null)
+                {
+                    attributes.AddRange(returnTypeAtts.GetCustomAttributes(false).Cast<Attribute>());


so this seems to be compressing a bit of information - we seem to take the return type attribute and then assign it to a list of attributes for the entire node (method) - whats the purpose of making it an attribute of the return value if we just add it to the same list of attributes on the FFIMethodAttributes ?

I guess we could make it an attribute on the entire method as we do for AllowRankReduction. Currently, the ArbitraryDimensionArrayImport attribute's usage is for parameters and return values, and if we were to change, it, it would need to be applied to methods and properties as we do for AllowRankReduction. Not sure if that is a bigger change and/or API break. I believe with the return qualifier it also makes it clearer that it's applied to the return value and not to something else on the method.

mjkkirschner · 2022-06-27T16:01:21Z

@aparajit-pratap it looks good to me, I'm a bit confused by the purpose of using the return qualifier when assigning the attribute since it appears we just lump it together with other attributes, but I don't think it hurts anything unless we might want to support that same attribute on the whole method in some cases where it does something else?

mjkkirschner · 2022-06-27T17:04:30Z

@aparajit-pratap I've started: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/6392/ as the last test run on this branch had 3 failures.

add allowrankreduction attr to Dictionary.Values

490b8c6

aparajit-pratap requested a review from saintentropy June 21, 2022 22:59

aparajit-pratap requested a review from sm6srw June 22, 2022 19:31

sm6srw approved these changes Jun 23, 2022

View reviewed changes

aparajit-pratap added 3 commits June 24, 2022 11:07

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

e3b609b

update tests

34fece2

enable arbitrarydimensionarrayimport attribute for return values, app…

1743221

…ly to Dictionary.Values

aparajit-pratap commented Jun 24, 2022

View reviewed changes

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

}

return false;

}

Copy link

Contributor Author

aparajit-pratap Jun 24, 2022

Choose a reason for hiding this comment

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

unused

revert changed tests as there's no change in behaviour

eaccdaf

aparajit-pratap requested a review from sm6srw June 24, 2022 23:18

aparajit-pratap changed the title ~~Re-add allowrankreduction attribute to Dictionary.Values~~ Enable ArbitraryDimensionArrayImport attribute to function on return values, apply this to Dictionary.Values getter Jun 24, 2022

aparajit-pratap changed the title ~~Enable ArbitraryDimensionArrayImport attribute to function on return values, apply this to Dictionary.Values getter~~ [DYN-2588] Enable ArbitraryDimensionArrayImport attribute to return values, apply this to Dictionary.Values getter Jun 25, 2022

aparajit-pratap changed the title ~~[DYN-2588] Enable ArbitraryDimensionArrayImport attribute to return values, apply this to Dictionary.Values getter~~ [DYN-2588] Enable ArbitraryDimensionArrayImport attribute to function return values, apply this to Dictionary.Values getter Jun 25, 2022

add unit test

75ef95a

support return value arbitrary dimension attribute for all methods, a…

ef6ebb2

…dd test

sm6srw approved these changes Jun 27, 2022

View reviewed changes

aparajit-pratap requested a review from sm6srw June 27, 2022 15:15

mjkkirschner reviewed Jun 27, 2022

View reviewed changes

mjkkirschner approved these changes Jun 27, 2022

View reviewed changes

aparajit-pratap merged commit 2f4a270 into DynamoDS:master Jun 27, 2022

aparajit-pratap deleted the fixDictValues branch June 27, 2022 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DYN-2588] Enable ArbitraryDimensionArrayImport attribute to function return values, apply this to Dictionary.Values getter #13055

[DYN-2588] Enable ArbitraryDimensionArrayImport attribute to function return values, apply this to Dictionary.Values getter #13055

aparajit-pratap commented Jun 21, 2022 •

edited

Loading

Amoursol commented Jun 22, 2022

aparajit-pratap commented Jun 22, 2022 •

edited

Loading

Amoursol commented Jun 22, 2022

sm6srw left a comment

aparajit-pratap commented Jun 23, 2022

sm6srw commented Jun 23, 2022

aparajit-pratap Jun 24, 2022

aparajit-pratap commented Jun 24, 2022 •

edited

Loading

Amoursol commented Jun 25, 2022

sm6srw left a comment

sm6srw Jun 27, 2022

aparajit-pratap Jun 27, 2022

aparajit-pratap Jun 27, 2022

sm6srw Jun 27, 2022

aparajit-pratap Jun 27, 2022

mjkkirschner Jun 27, 2022 •

edited

Loading

mjkkirschner Jun 27, 2022

mjkkirschner Jun 27, 2022

aparajit-pratap Jun 27, 2022 •

edited

Loading

mjkkirschner commented Jun 27, 2022

mjkkirschner commented Jun 27, 2022

[DYN-2588] Enable ArbitraryDimensionArrayImport attribute to function return values, apply this to Dictionary.Values getter #13055

[DYN-2588] Enable ArbitraryDimensionArrayImport attribute to function return values, apply this to Dictionary.Values getter #13055

Conversation

aparajit-pratap commented Jun 21, 2022 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

Amoursol commented Jun 22, 2022

aparajit-pratap commented Jun 22, 2022 • edited Loading

Amoursol commented Jun 22, 2022

sm6srw left a comment

Choose a reason for hiding this comment

aparajit-pratap commented Jun 23, 2022

sm6srw commented Jun 23, 2022

Choose a reason for hiding this comment

aparajit-pratap commented Jun 24, 2022 • edited Loading

Amoursol commented Jun 25, 2022

sm6srw left a 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

mjkkirschner Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

mjkkirschner commented Jun 27, 2022

mjkkirschner commented Jun 27, 2022

aparajit-pratap commented Jun 21, 2022 •

edited

Loading

aparajit-pratap commented Jun 22, 2022 •

edited

Loading

aparajit-pratap commented Jun 24, 2022 •

edited

Loading

mjkkirschner Jun 27, 2022 •

edited

Loading

aparajit-pratap Jun 27, 2022 •

edited

Loading