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

Enable nullable on some modules #1901

Merged
merged 7 commits into from
Feb 3, 2025
Merged

Conversation

slozier
Copy link
Contributor

@slozier slozier commented Feb 2, 2025

Add nullable annotations on some modules.

Other notable changes:

  • Excludes PerformModuleReload from the analyzer so we don't need to pragma disable it.
  • Gets rid of ø from params and ParamDictionary arguments. There are still a couple \u00F8 left over in other arguments. Not sure if they're necessary or not...
  • Remove NotNone from ParamDictionary arguments since it's never null.
  • Add NotNone to all params arguments. Not having this can result in a null instead of the empty array. There seem to be some cases where this isn't necessary but I haven't quite figure out the logic so I'm just putting it everywhere (with the help of an analyzer).
  • Other small bug fixes.

@BCSharp
Copy link
Member

BCSharp commented Feb 2, 2025

Also excludes PerformModuleReload from the analyzer so we don't need to pragma disable it.

Great! I was just about starting to do the same…

@slozier slozier marked this pull request as ready for review February 2, 2025 14:22
Copy link
Member

@BCSharp BCSharp left a comment

Choose a reason for hiding this comment

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

  • Gets rid of ø from params and ParamDictionary arguments. There are still a couple \u00F8 left over in other arguments. Not sure if they're necessary or not...

Strange that there are some \u00F8 in some other arguments. We would probably have to look at those instances case by case (if somebody has the patience).

I would suggest to further standardize the parameter names to args and kwargs, since these are Python names by convention and they do show up in generated signatures by help(...). I see in some places kwArgs (.NET camelCase) or otherArgs (???)

  • Remove NotNone from ParamDictionary arguments since it's never null.

  • Add NotNone to all params arguments. Not having this can result in a null instead of the empty array. There seem to be some cases where this isn't necessary but I haven't quite figure out the logic so I'm just putting it everywhere (with the help of an analyzer).

The odd behaviour is most likely because IronLanguages/dlr#249 is not finished yet — only the prohibition by keyword part is done. The prohibition by position is not, but the goal is that ParamDictionary is prohibited by position as well, while params array is allowed (this is only partly implemented). So the rules you put in the Analyzer are ultimately correct (for IronPython, at least).

The logic for params array is (or is supposed to be) that without [NotNone], when None is passed, then args is null (classic binding by position). With [NotNone], args becomes [null] (an array with 1 element of value null). The reason for supporting binding by position is that it matches C#, and IPY needs to be able to call .NET API like C# can.

I am a bit surprised that ParamDictionary never binds by position (hence is never null), perhaps I implemented more than I remember now. Or perhaps there are still some corner cases when it is possible (like multiple overloads with various number of parameters combined with optionals).

Comment on lines +67 to +68
// PerformModuleReload is special and we don't need NotNone annotations
if (methodSymbol.Name == "PerformModuleReload") return;
Copy link
Member

Choose a reason for hiding this comment

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

I suppose PerformModuleReload is special only if it has SpecialMethodAttribute? Although I haven't seen any non-special ones around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I can tell SpecialMethodAttribute doesn't affect anything. 🤷

@slozier
Copy link
Contributor Author

slozier commented Feb 3, 2025

Strange that there are some \u00F8 in some other arguments. We would probably have to look at those instances case by case (if somebody has the patience).

It's not too bad, count is down to 35 lines after this PR. Some of them seem "legit" like dict.update where I think we'd need something like PositionalOnly to express things properly. Others are more of a head scratcher, but hopefully they're there for a reason...

I would suggest to further standardize the parameter names to args and kwargs, since these are Python names by convention and they do show up in generated signatures by help(...). I see in some places kwArgs (.NET camelCase) or otherArgs (???)

Sure, I can look at that.

I am a bit surprised that ParamDictionary never binds by position (hence is never null), perhaps I implemented more than I remember now. Or perhaps there are still some corner cases when it is possible (like multiple overloads with various number of parameters combined with optionals).

I did double check this with a simple void delete_me([ParamDictionary] IDictionary<string, object> x) which failed with a TypeError: delete_me() takes no arguments (1 given) if called with delete_me(None). Error message is not great, but at least it's throwing as expected...

I think it's when we have both a ParamDictionary and a params array that the NotNone would be unnecessary. The dictionary prevents binding by position (because it's always before params which has to be last) so when calling with None we end up with the [null] array. Anyway, I'm going to leave the analyzer as-is for now since I think it makes the intent clearer.

@slozier slozier merged commit e47fcbe into IronLanguages:main Feb 3, 2025
8 checks passed
@slozier slozier deleted the nullable branch February 3, 2025 21:47
@slozier
Copy link
Contributor Author

slozier commented Feb 3, 2025

I would suggest to further standardize the parameter names to args and kwargs, since these are Python names by convention and they do show up in generated signatures by help(...). I see in some places kwArgs (.NET camelCase) or otherArgs (???)

Sure, I can look at that.

Haha, I just wrote that I would look at it then clicked on merge. 🤦 I've got more analyzer tweaks coming up so will include it in that...

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