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

Fix extension function imports #1814

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Fix extension function imports #1814

merged 3 commits into from
Jan 29, 2024

Conversation

sebek64
Copy link
Contributor

@sebek64 sebek64 commented Jan 25, 2024

This pull request aims to fix some scenarios where extension function is not properly imported. As described for example here, the import is always needed. This change forces the import to be created, even if it means creating an alias.

I don't know this library so well, so I hope I didn't break some other scenarios. I'm willing to update the pull request by your suggestions.

@Egorand
Copy link
Collaborator

Egorand commented Jan 25, 2024

This pull request aims to fix some scenarios where extension function is not properly imported.

Can you please include unit tests that demonstrate the problem you're trying to solve, and that fail without your fix and pass with your fix?

@sebek64
Copy link
Contributor Author

sebek64 commented Jan 25, 2024

Unit test added - fails if applied without the fixing commits, succeeds now.

@Egorand
Copy link
Collaborator

Egorand commented Jan 26, 2024

Thanks for the test! I'm actually seeing similar behaviour with types - if there's a type with the same name as another type in the same package, we'll use the fully-qualified name for the former. It's not a big deal for types and non-extension members, but definitely breaks compilation for extensions.

I wonder if it's worth special-casing extension members, or should we just generate aliases for all names that clash with names of elements in the same package?.. I'm planning to take a closer look at your PR over the weekend, or next week at the latest.

@sebek64
Copy link
Contributor Author

sebek64 commented Jan 26, 2024

Honestly, I don't care about the particular solution, as long as the generated code would be compilable. I'm leaving the decision up to you, I can adjust the PR if you request in some way, or feel free to implement it your way and reject this one as unnecessary.
It's not time pressing for me, so if the fixed release is available in a month or two, it's fine. As a workaround, I'm moving things around packages to force the imports to be aliased (not to have the extension functions used from the current package).

@Egorand
Copy link
Collaborator

Egorand commented Jan 26, 2024

Note that there are APIs to add alias imports manually, you can use them as well as a workaround.

Copy link
Collaborator

@Egorand Egorand left a comment

Choose a reason for hiding this comment

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

The change looks good, thanks! Left a couple comments, once they're addressed we can merge the PR.

@Egorand Egorand merged commit b28e315 into square:main Jan 29, 2024
4 checks passed
@Egorand
Copy link
Collaborator

Egorand commented Jan 29, 2024

Merged, thank you!

@sebek64 sebek64 deleted the fix-extension-function-imports branch January 29, 2024 11:01
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