Optimise materialised code + allow = ??? and accessor methods
#87
Conversation
…l members from both api and implementation
…ing or being abstract This reverses pull request lihaoyi#58 (lihaoyi#58)
| member <- membersOfBaseAndParents | ||
| if !isAMemberOfAnyRef(member) | ||
| if !member.isSynthetic | ||
| if member.isPublic | ||
| if member.isTerm | ||
| if member.isAbstract |
There was a problem hiding this comment.
Allows a method in the shared API to define a dummy implementation like
def someMethod() = ???
This allow client classes to extend the shared api without themselves having to implement the interface methods or being abstract classes themselves.
Removing the isAbstract check reverses pull request #58. Would omitting the isAbstract check do any harm when using scalapb generated rpc's @nathanleyton?
| } | ||
|
|
||
|
|
||
| class MacroHelp[C <: Context](val c: C) { | ||
| class MacroHelp[C <: blackbox.Context](val c: C) { |
There was a problem hiding this comment.
Replacing deprecated Context with more specific blackbox.Context
| if (member.returnType <:< c.typeOf[Future[_]]) t | ||
| else q"scala.concurrent.Future($t)" | ||
| } | ||
|
|
||
| def getValsOrMeths(curCls: Type): Iterable[Either[(c.Symbol, MethodSymbol), (c.Symbol, MethodSymbol)]] = { | ||
| def getValsOrMeths(curCls: Type): Iterable[Either[(Symbol, MethodSymbol), (Symbol, MethodSymbol)]] = { |
There was a problem hiding this comment.
No need to prefix types with Context
| memTerm = member.asTerm | ||
| if memTerm.isMethod | ||
| } yield { | ||
| member -> memTerm.asMethod | ||
| } | ||
|
|
||
| extractableMembers flatMap { case (member, memTerm) => | ||
| extractableMembers.toList.distinct flatMap { case (member, memTerm) => |
There was a problem hiding this comment.
No need to materialise code from duplicate method names (one from api, one from implementation). This will cut materialised code almost in half.
| """ | ||
| val futurized: Tree = if (params.isEmpty) { | ||
| // Accessor method | ||
| futurize(q"$memSel", meth) |
There was a problem hiding this comment.
Method invocation without parameter list allows for accessor methods.
If a method is defined with empty parenthesis def someMethod() it will be materialised as an accessor method def someMethod which seems to be fine (all tests pass with/without parenthesis).
| (unwrapTree, methodName, args, Nil, Nil) | ||
| case t@q"..${statements: List[ValDef]@unchecked}; $thing.$call(..$args)" | ||
| case q"$unwrapTree.$methodName" => |
There was a problem hiding this comment.
This allows calling the accessor method (without parenthesis).
| case Error.InvalidInput( | ||
| Error.Param.Invalid("x", InvalidData(Arr(Seq()), _)), | ||
| Error.Param.Invalid("ys", InvalidData(Num(234), _)) | ||
| Error.Param.Invalid("x", AbortException("expected number got sequence", _, _, _, _)), |
There was a problem hiding this comment.
Adding the error messages helps understand the problems.
There was a problem hiding this comment.
I hadn't add Exception strings in tests, this is better this way
|
Thanks Marc for this (huge first) PR. Indeed we solved test in the same time :). I tried tests with your PR, they all pass. Do you have any idea on how fix the Case Class test (https://github.com/marcgrue/autowire/blob/master/autowire/shared/src/test/scala/autowire/UpickleTests.scala#L125) ? @lihaoyi, do you have any comment on other items ? |
|
Thanks yourself, Mathieu, for putting time into Lihaoyi's excellent library! I had a look at the has something to do with upickle but I'm not sure :-( |
|
Yes definitely. I tried to implement something like this https://www.lihaoyi.com/upickle/#ManualSealedTraitPicklers, but It didn't work. I am now use Boopickle, so I do not really understand upickle nuances :) ! |
|
I'm happy it worked. Thanks, Mathieu! |
Sorry for the large code base to review! I saw that @mathieuleclaire also just made the tests pass.
This pull request consists of basically 5 parts:
= ???(commit)Allowing a shared interface to define methods with an undefined implementation (
def someMethod() = ???) allows client code to extend such interfaces without having to implement the methods or being abstract itself.Allowing accessor methods solves #36
(embarrassing fact: this is my first pull request - have been working solo on my own projects for years. So please let me know if I'm doing something wrong here)