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

Extension copy #262

Merged
merged 14 commits into from
Dec 17, 2024
Merged

Extension copy #262

merged 14 commits into from
Dec 17, 2024

Conversation

OndrejSpanel
Copy link
Contributor

@OndrejSpanel OndrejSpanel commented Dec 1, 2024

Started work on #234 by creating target test.

At the moment the test is failing with:

Unsupported path element. Path must have shape: .field1.field2.each. field3.(...), got: (($1: Vec) => x(_$1))

I am not sure how far I will get, I never tried to access extensions from macros, but hopefully it should be possible.

@OndrejSpanel
Copy link
Contributor Author

I am familiarizing myself with the toPath method. It seems to me the extension should be processed in the branch case Apply(deep, idents). Actually no other code from tests goes through this path.

@OndrejSpanel
Copy link
Contributor Author

First test passed: extension is found in a companion object of a plain class. This needed some adjustments in how is copy called in caseClassCopy, but nothing drastic (extension methods need to receive one more argument list). I will continue with different extension locations and then hopefully proceed to opaque types.

@OndrejSpanel
Copy link
Contributor Author

More progress: I can use extensions to read source values as well.

There are few things I was struggling with. Most of them are related to the fact the functions are operating on symbols (Symbol), not types (TypeRepr), therefore finding suitable function overloads is sometimes a bit more messy than I would like.

The last step is to support opaque types. In first attempts I get assertions in the compiler when using them, hopefully I will be able to work through this somehow.

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented Dec 4, 2024

I have hit a kind of obstacle: scala/scala3#22143

I am able to access the fields using extension methods, but if the underlying type already has fields with corresponding names, Symbol.fieldMember finds them and I assume they exist and can be used, which results in a bad resulting tree, with a.x instead of Vec.x(a), which later fails when compiling.

The whole Symbol / TypeRepr reflection is still something I do not understand much, therefore it is quite possible the behaviour I see is not a bug. No matter what, hopefully there will be some workaround, some way to find the a.x is not accessible.

@OndrejSpanel OndrejSpanel force-pushed the extension-copy branch 3 times, most recently from d839147 to 1cdfa2b Compare December 4, 2024 17:18
@OndrejSpanel
Copy link
Contributor Author

I think it should be done now.

@OndrejSpanel OndrejSpanel marked this pull request as ready for review December 4, 2024 17:20
@OndrejSpanel OndrejSpanel force-pushed the extension-copy branch 2 times, most recently from 63f5866 to c08a747 Compare December 4, 2024 23:55
@adamw
Copy link
Member

adamw commented Dec 6, 2024

@KacperFKorban maybe you could take a look as well?

@KacperFKorban
Copy link
Collaborator

@adamw Sure, I'll review it it on Monday.

@KacperFKorban KacperFKorban self-assigned this Dec 7, 2024
@KacperFKorban KacperFKorban self-requested a review December 7, 2024 11:19
Copy link
Collaborator

@KacperFKorban KacperFKorban left a comment

Choose a reason for hiding this comment

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

I figured that it would be faster to just push some changes directly to the branch, hope you don't mind @OndrejSpanel
Looks good overall!

@@ -351,6 +427,7 @@ object QuicklensMacros {
def mapToCopy(owner: Symbol, mod: Expr[A => A], objTerm: Term, pathTree: PathTree): Term = pathTree match {
case PathTree.Empty =>
val apply = termMethodByNameUnsafe(mod.asTerm, "apply")
// TODO: calling extension may be necessary here
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still relevant? AFAIK this is just the modification function being called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@OndrejSpanel This comment might've slipped through. Do you remember the reasoning behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was just a comment made during exploration and it should be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, Thanks!

@OndrejSpanel
Copy link
Contributor Author

hope you don't mind

Sure, better than requesting me to do them. I will have some questions regarding some changes.

@adamw adamw merged commit 663e750 into softwaremill:master Dec 17, 2024
2 checks passed
@adamw
Copy link
Member

adamw commented Dec 17, 2024

Great, thanks! :)

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.

3 participants