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

Use imp summon macro to summon type class instance for apply #49

Merged
merged 2 commits into from
Dec 23, 2015

Conversation

mikejcurry
Copy link
Contributor

This PR is to address #48

Happy to tweak it if there is a better way to do this with macros - have never written scala macro code before.

Figured the best way to learn is to write some…

@mpilquist
Copy link
Member

👍 Awesome! Looks good to me.

@non @milessabin Any comments before merging?

@milessabin
Copy link
Member

I think it'll make sense to share implementation in Typeclassic eventually, but in this case I think you'd do better just to make your apply method a macro directly, with a one-liner inferImplicitValue implementation.

@mikejcurry
Copy link
Contributor Author

Cool, I can definitely look at doing something like that if you folks think it is better.

With that said, what's the rationale that means it would be better just to do it directly? The imp macro has tests to verify the bytecode generated is as expected, and it would be a shame to have to duplicate those tests here - or even worse just assume that the same result is achieved, when leveraging the imp library means that the expectation is already verified.

Granted it does not seem like a major duplication but still interested in the rationale, e.g. is it something to do with adding an extra compile time dependency on users?

@mpilquist
Copy link
Member

I'd like to hear more rationale too. I'm typically very dependency adverse, but in this case, given that simulacrum and imp will eventually be combined in typeclassic, I don't see any major downsides.

@milessabin
Copy link
Member

Well, it's really that the local implementation will be less lines of code/sbt than the dependency :-)

@milessabin
Copy link
Member

It's really just,

def apply[$tparam](implicit ev: ${typeClass.name}[${tparam.name}]): ${typeClass.name}[${tparam.name}] =
  macro MacroObj.applyImpl

object MacroObject { def applyImpl[T](implicit c: Context)(t: c.Expr[T]): c.Expr[T] = t }

@mikejcurry
Copy link
Contributor Author

For sure, that seems a small amount of code, and looks like it would directly emulate current imp behaviour.

FWIW, I personally think I prefer the dependency. Amongst other things, if non/imp#2 (comment) is solved to provide a better variant for scala 2.11, and if that happens before typeclassic (not sure when that is happening??) then an imp upgrade should provide the optimized behavior in a relatively straightforward fashion.

BTW - I did spend a few minutes looking at porting the code above into simulacrum. The object that defines the macro needs to be a stable object I think, i.e. the object MacroObject bit outlined, which means generating it inside the other generated classes/objects doesn't seem to work. Even if it was generated at a top level, we'd have to worry about generating duplicates within a given package. So, I think the additional macro object will need to be defined as a stable object somewhere else within simulacrum - but haven't had a chance to see what would be possible on that side yet - of course I could be doing something stupid. :-)

@mpilquist
Copy link
Member

OK let's go with this PR then. @mikejcurry one minor issue -- we need to remove provided from the imp dependency so that the imp macro is on the compile time classpath of a dependent project. I suspected that might be the case but didn't have a chance to test it until now. Once you update that, I'll merge. Thanks!

@mikejcurry
Copy link
Contributor Author

Cool, that should be done now. Good catch on the provided bit! I should have caught it myself

mpilquist added a commit that referenced this pull request Dec 23, 2015
Use imp summon macro to summon type class instance for apply
@mpilquist mpilquist merged commit 4d537dc into typelevel:master Dec 23, 2015
@mpilquist mpilquist mentioned this pull request Jan 5, 2016
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