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

Return appropriate derived class when method returns self #4622

Merged
merged 11 commits into from
Aug 13, 2018
Merged

Return appropriate derived class when method returns self #4622

merged 11 commits into from
Aug 13, 2018

Conversation

MikhailArkhipov
Copy link

Fixes #4614

Will add test next

MikhailArkhipov added 3 commits August 9, 2018 15:48
return _method.ReturnTypes.GetInstanceType();
// Check if method returns self
var returnType = _method.ReturnTypes.GetInstanceType();
if (args.Length > 0 && returnType.FirstOrDefault() is BuiltinInstanceInfo biif && biif.PythonType == _method.Function?.DeclaringType) {
Copy link
Member

Choose a reason for hiding this comment

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

There's a .Split extension method for returnType here that makes it easier to handle multiple return values properly (split with a predicate, then union and return). Otherwise, I like the approach

@MikhailArkhipov MikhailArkhipov changed the title [WIP]: Return appropriate derived class when method returns self Return appropriate derived class when method returns self Aug 10, 2018
Copy link
Member

@zooba zooba 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. Worth bearing in mind that when we decide to support all parameter mappings through return values for the single-pass analysis that this change will be largely undone. It may also miss some current cases or lose some information if there are multiple return types.

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