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

Accommodate user-defined mirrors #219

Merged
merged 2 commits into from
May 5, 2024
Merged

Conversation

joroKr21
Copy link
Member

Fixes #218

@joroKr21 joroKr21 added the enhancement New feature or request label Apr 26, 2024
@joroKr21 joroKr21 self-assigned this Apr 26, 2024
@joroKr21 joroKr21 force-pushed the ud-mirrors branch 4 times, most recently from f461a2d to be4bc43 Compare April 27, 2024 19:20
@joroKr21
Copy link
Member Author

@tschuchortdev I tried this out in Kittens and unfortunately it degrades compiler performance significantly (rootJVM/Test/compile went from ~30s to ~60s). So I think we will have to put it behind an import after all.

@tschuchortdev
Copy link

Oh, that's too bad, but I guess it can't be avoided. Have you thought about putting an @implicitNotFound on Generic parameters to direct the user towards the import? I've just experimented with it a little bit and it seems that the custom error message is simply prepended to the compiler's message "failed to synthesize Generic because Foo is not a case class", meaning the error messages will not be worse in other cases if we add our own hint.

@joroKr21
Copy link
Member Author

Yes, this would make sense. Although I'm still waiting for a backport of @implicitNotFound for type aliases: scala/scala3#20275

Comment on lines +399 to +403
def assertImportSuggested(errors: List[testing.Error]): Unit =
assertEquals(1, errors.size)
val message = errors.head.message.trim
assert(message.startsWith("No given instance of type"))
assert(message.endsWith("Generic.fromMirror"))
Copy link
Member Author

Choose a reason for hiding this comment

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

@tschuchortdev the default message already suggests the import so I think it's adequate

Choose a reason for hiding this comment

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

Alright. In my experience those suggestions are rarely productive, so I tend to ignore them.

@tschuchortdev
Copy link

Looks good to me now.

@joroKr21 joroKr21 merged commit a203ad0 into typelevel:main May 5, 2024
6 checks passed
@joroKr21 joroKr21 deleted the ud-mirrors branch May 5, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic instances for user-defined Mirror instances
2 participants