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.

Sign up for GitHub

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

Jump to bottom

1768 .NET Attribute Support: Fix #2032

Open

rmadsen-ks wants to merge 1 commit into pythonnet:master from rmadsen-ks:1768-DotNetAttributeSupport

Contributor

rmadsen-ks commented Nov 30, 2022

Added support for specifying .NET attributes using clr_attributes. Classes, properties and methods can specify this value.
Added support for Attribute shorthand without the "Attribute" ending.
Added unit test verifying the behavior

Close #1768

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Ensure you have signed the .NET Foundation CLA
Add yourself to AUTHORS
Updated the CHANGELOG


          1768 .NET Attribute Support: Fix

42ee643

- Added support for specifying .NET attributes using __clr_attributes__. Classes, properties and methods can specify this value.

- Added support for Attribute shorthand without  the "Attribute" ending.

- Added unit test verifying the behavior

lostmsu requested changes

View reviewed changes

src/runtime/Types/ClassDerived.cs

+                          Type attribute = tp[0].As<Type>();
+                          if (typeof(Attribute).IsAssignableFrom(attribute) == false)
+                          {
+                              throw new Exception($"This type cannot be used as an attribute type: {attribute}");

Member

lostmsu Nov 30, 2022

The exception type must be specific.

Also, please add a test for this scenario to ensure it does not lead to a fatal error. You might need to raise a Python exception instead.

src/runtime/Types/ClassDerived.cs

+                          // in python there is no difference. But luckily there is rarely a conflict, so we can treat
+                          // named arguments and named properties or fields the same way.
+                          var tp = new PyTuple(attr);
+                          Type attribute = tp[0].As<Type>();

Member

lostmsu Nov 30, 2022

Same as below, you need to handle the scenario where the object passed here is not actually a Type properly.

src/runtime/Types/ClassDerived.cs

Comment on lines +730 to +731

		var args = tp[1].As<PyObject[]>();
		var dict = new PyDict(tp[2]);

Member

lostmsu Nov 30, 2022

Same.

src/runtime/Types/ClassDerived.cs

Comment on lines +731 to +732

		var dict = new PyDict(tp[2]);
		var dict2 = new Dictionary<string, PyObject>();

Member

lostmsu Nov 30, 2022

Please name variables with descriptive names.

src/runtime/Types/ClassDerived.cs

Comment on lines +739 to +740

		var allConstructors = attribute.GetConstructors();
		foreach (var constructor in allConstructors)

Member

lostmsu Nov 30, 2022

This reimplements our overloading logic. We definitely do not want to do that. Instead the Python side should use some simple mechanism to set the fields. For example, there could be a .with(attributes: dict[str, Any]) -> Self method, that would set values of the fields/properties from the dictionary.

E.g. DllImport("kernel32").with({"CharSet": Auto}) (just an example, a different way to do this could also be acceptable).

Contributor Author

rmadsen-ks Nov 30, 2022 •

edited

I think that might make sense for properties. I just wanted the call to be analogous to how it is invoked in C#, since people will always be finding the C# documentation/ code samples for how those attributes are used. e.g

[AttributeTest("UnnamedArg", NamedArg : "x", NamedProperty="y")]

This would now be used like this:

AttributeTest("UnnamedArg", NamedArg = "x", NamedProperty="y")

But you are suggesting this instead?

AttributeTest("UnnamedArg", NamedArg = "x").with({"NamedProperty": "y"})

Member

lostmsu Nov 30, 2022

@rmadsen-ks correct. The reason is that we don't want to maintain yet another overload resolution handler. That's one of the most complicated parts of C# that we try to mirror.

Contributor Author

rmadsen-ks Nov 30, 2022

Ok, so you are not talking so much about changing these specific lines, but rather changing how the data for that is stored?

I am not sure we can use the other overload resolution handler, because that is for runtime overload resolution, right? this is compile/load time and the data we can put into those attributes is limited.

Member

lostmsu Nov 30, 2022

@rmadsen-ks since this is running in Python, it is runtime.

I believe you should be able to use MethodBinder.Bind which returns a Binding.

Contributor Author

rmadsen-ks Nov 30, 2022 •

edited

Unless I'm completely confused, The selected overload gets compiled to IL by System.Reflection.Emit. That is what the returned CustomAttributeBuilder is for.

Member

lostmsu Nov 30, 2022 •

edited

@rmadsen-ks not sure I understand this comment.

MethodBinder.Bind performs basically what your loop performs: selects the method from the list of overloads, and returns it + the arguments for it as .NET would see them (e.g. an instance of Binding), which you can then use in the CustomAttributeBuilder constructor.

I am talking about replacing the whole loop with an invocation to MethodBinder.Bind

src/runtime/Types/ModuleObject.cs

Comment on lines +139 to +153

+                          var qname2 = qname + "Attribute";
+                          var type2 = AssemblyManager.LookupTypes(qname2).FirstOrDefault(t => t.IsPublic);
+                          if (type2 != null)
+                          {
+                              var str = "def {2}__AttributeBuilder(*args, **kwargs):\n"
+                                        + "   import {1}\n"
+                                        + "   return ({0}, args, kwargs)\n";
+                              str = string.Format(str, qname2, _namespace, name);
+                              PythonEngine.Exec(str);
+                              using var attributeData = PythonEngine.Eval(name + "__AttributeBuilder");
+                              var pinnedAttributeData = attributeData.NewReferenceOrNull();
+                              StoreAttribute(name, pinnedAttributeData.Borrow());
+                              return new NewReference(pinnedAttributeData);
+                          }

Member

lostmsu Nov 30, 2022

This seems overly complicated. Can't you just do what the code above does for type lookup, and just add another StoreAttribute for the alias?

Contributor Author

rmadsen-ks Nov 30, 2022

So for e.g BrowsableAttribute, this generates a function which takes the arguments and stores it as a tuple along with the type name. That way we can reconstruct which arguments are used for creating the attribute when later generating the code.

The reason it needs to be a function is that whenever e.g Browsableis invoked, it needs to generate a new tuple because the arguments are not the same every time.

Member

lostmsu commented Nov 30, 2022

@rmadsen-ks btw generally speaking the code down at that level should not be using PyObject unless strictly necessary. Instead, BorrowedReference and NewReference should be used in conjunction with Runtime.Py... methods.

Contributor Author

rmadsen-ks commented Nov 30, 2022

@rmadsen-ks btw generally speaking the code down at that level should not be using PyObject unless strictly necessary. Instead, BorrowedReference and NewReference should be used in conjunction with Runtime.Py... methods.

Which code do you mean specifically? This code used PyDict before I arrived.. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment