Skip to content

Handle generic parameter types as return types of builtins in DocBuilder#1046

Merged
BCSharp merged 5 commits intoIronLanguages:masterfrom
BCSharp:generic_param
Nov 22, 2020
Merged

Handle generic parameter types as return types of builtins in DocBuilder#1046
BCSharp merged 5 commits intoIronLanguages:masterfrom
BCSharp:generic_param

Conversation

@BCSharp
Copy link
Copy Markdown
Member

@BCSharp BCSharp commented Nov 21, 2020

Fixes:

>>> import System
>>> help(System.Reflection.CustomAttributeExtensions.GetCustomAttribute)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\_sitebuiltins.py", line 103, in __call__
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\pydoc.py", line 1843, in __call__
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\pydoc.py", line 1876, in help
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\pydoc.py", line 1630, in doc
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\pydoc.py", line 1623, in render_doc
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\pydoc.py", line 373, in document
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\pydoc.py", line 1364, in docroutine
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\pydoc.py", line 90, in getdoc
  File "C:\Code\ironlang\ironpython3\Src\StdLib\Lib\inspect.py", line 477, in getdoc
SystemError: Method must be called on a Type for which Type.IsGenericParameter is false.

The documentation on PythonType.GetPythonType(Type type) says it may return null if no type is available. So this is what it does now. However, this code path (chiefly though DynamicHelpers.GetPythonTypeFromType) is used in a lot of places that do not check for null. From nullability annotation point of view, it would be good to keep the return type of GetPythonTypeFromType non-nullable, but how to handle errors? ArgumentException?

@BCSharp BCSharp marked this pull request as draft November 21, 2020 05:04
@BCSharp BCSharp marked this pull request as ready for review November 21, 2020 06:56
@slozier
Copy link
Copy Markdown
Contributor

slozier commented Nov 21, 2020

Why do we need to prevent GetPythonType from returning a type when Type.IsGenericParameter is true? As far as I can tell it works in .NET Core and only fails on .NET Framework due to an interface for COM support. Or am I missing something?

Edit: Ok, the failure is not related to the COM type itself, but it still seems like we could guard against calling Type.GetInterfaceMap on an IsGenericParameter type instead of preventing the PythonType creation? For example if (_underlyingSystemType.IsGenericParameter) return; in PythonType.AddSystemInterfaces.

@BCSharp
Copy link
Copy Markdown
Member Author

BCSharp commented Nov 21, 2020

we could guard against calling Type.GetInterfaceMap on an IsGenericParameter type

Actually, this is what I had implemented first, but then got thinking that perhaps it would be a bad idea to have such types floating around in the Python space. After all, they are not "real" types, should not be instantiated, each one is unique, despite having the same name/CLR type, may not belong to any namespace (when "declared" by a method), etc.

But I am not quite sure about that, so if you think it would be OK, I can put that support back in.

@slozier
Copy link
Copy Markdown
Contributor

slozier commented Nov 21, 2020

I haven't tried to get my hands on a PythonType of a generic parameter type to see what you could do with it. I expect not much? Not sure what the disadvantage of having such types would be.

It seems like having PythonType.GetPythonType always return a PythonType will lead to a cleaner codebase than returning null (and by consequence having DynamicHelpers.GetPythonTypeFromType throw or return null)? We wouldn't have to deal with potential null checks or try/catch when trying to get a PythonType.

Note that while the comment said it may return null it was also annotated with /*!*/ to say that it does not return null. 😄

Copy link
Copy Markdown
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

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

Looks good to me.

type = type.GetElementType();
}

if (type.IsGenericParameter) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we still need this check? I guess it's fine since prevents the PythonType from being created.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it is a fast track now, still useful. Generic parameter types have trivial names.

@BCSharp BCSharp merged commit 7840403 into IronLanguages:master Nov 22, 2020
@BCSharp BCSharp deleted the generic_param branch November 22, 2020 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants