Conversation
| // The transparent inline here ensures the precise return type | ||
| // propagates to the call site | ||
| transparent inline def selectDynamic(inline name: String): Any = { | ||
| ${ | ||
| HierarchyLookupMacro.selectDynamicImpl[A]( | ||
| '{ this.asInstanceOf[Hierarchy[A]] }, | ||
| 'name | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| transparent inline def applyDynamic(inline name: String)(inline args: Any*): Any = { | ||
| ${ | ||
| HierarchyLookupMacro.applyDynamicImpl[A]( | ||
| '{ this.asInstanceOf[Hierarchy[A]] }, | ||
| 'name, | ||
| 'args | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The split between HierarchyProto and HierarchyIsA doesn't really make sense but the latter has a name that implies something very specified (support for .isA), so I think these methods should go on HierarchyProto (and we should ultimately meld HierarchyIsA into HierarchyProto).
| val clzName = ct.runtimeClass.getCanonicalName | ||
| if (clzName == null) false | ||
| else inBaseClasses(modifyReplString(clzName)) | ||
| } |
There was a problem hiding this comment.
| } | |
| def isA[B](using ct: ClassTag[B]): Boolean = { | |
| val clzName = ct.runtimeClass.getCanonicalName | |
| if (clzName == null) false | |
| else inBaseClasses(modifyReplString(clzName)) |
Don't use implicit in any Scala 3 code as it'll be annoying to switch to using later
Also consider using the syntactic sugar because I think it's nicer in API docs:
| } | |
| def isA[B : ClassTag]: Boolean = { | |
| val clzName = summon[ClassTag[B]].runtimeClass.getCanonicalName | |
| if (clzName == null) false | |
| else inBaseClasses(modifyReplString(clzName)) |
| /** Determine whether underlying proto is of type provided. | ||
| * | ||
| * @note IMPORTANT: this function requires summoning a TypeTag[B], which will fail if B is an inner class. | ||
| * @note IMPORTANT: this function requires summoning a ClassTag[B], which will fail if B is an inner class. |
There was a problem hiding this comment.
Is this statement still true? I asked Opus 4.7 to review and it said ClassTags can be summoned for inner classes, can we add a test for this and delete the note?
There's also a chance that we could capture the ClassTag for A in Scala 3 that might be able to make this whole thing work a lot better and just use <:<. We can save that for a follow on PR though.
There was a problem hiding this comment.
Another alternative is to use the new TypeTest, the following works:
case class Box[A](a: A) {
def isA[B](using tt: TypeTest[A, B]): Boolean = tt.unapply(a).isDefined
}
Box(List(1, 2, 3)).isA[Seq[Int]] // trueThat has same limitation about type parameterized types though. Maybe we should consider izumi-reflect?
| import q.reflect.* | ||
| val argTerms = argsExpr match { | ||
| case Varargs(args) => args.toList.map(_.asTerm) | ||
| case _ => report.errorAndAbort("applyDynamic on an Instance requires explicit varargs") |
There was a problem hiding this comment.
I don't understand this, can you add a comment explaining?
| val fieldSym = typeSym.fieldMember(name) match { | ||
| case s if s != Symbol.noSymbol => s | ||
| case _ => | ||
| typeSym.methodMember(name).find(_.paramSymss.flatten.isEmpty).getOrElse(Symbol.noSymbol) |
There was a problem hiding this comment.
A comment explaining that this looks for a method of the name with no arguments would be helpful.
I'm not completely sure why this is here though--we don't support @public on defs, maybe we hsould but this is not the place to change that. Now I will grant that it could be that the accessors are methods, but if that's the case we need to explain in comments.
| import q.reflect.* | ||
| Lambda( | ||
| owner = Symbol.spliceOwner, | ||
| tpe = MethodType(List("a"))(_ => List(TypeRepr.of[A]), _ => TypeRepr.of[T]), |
There was a problem hiding this comment.
What does this mean? Like what meaning does "a" have in this context?
| firstList.length == argTerms.length && | ||
| firstList.lazyZip(argTerms).forall((p, a) => a.tpe <:< cTpe.memberType(p).widen) | ||
| } | ||
| } |
There was a problem hiding this comment.
What apply method is this looking for? A comment explaining would be helpful.
Also it looks like it only finds the first one and only checks the first argument list. Do we know for sure that that is correct? If so, also needs to be explained in a comment.
| val finalTerm = applySym.paramSymss.zipWithIndex.foldLeft(initial) { case (acc, (_, idx)) => | ||
| val args = | ||
| if (idx == 0) argTerms | ||
| else { | ||
| val paramTpes = acc.tpe.widen match { | ||
| case mt: MethodType => mt.paramTypes | ||
| case _ => Nil | ||
| } | ||
| paramTpes.map { pt => | ||
| summonOrAbort( | ||
| pt, | ||
| explanation => s"could not summon implicit ${pt.show} when calling ${cTpe.show}.apply: $explanation" | ||
| ) | ||
| } |
There was a problem hiding this comment.
This appears to be assuming that only the first parameter list of whatever we're invoking is explicit and the rest are implicit, couldn't we have multiple parameter lists?
There was a problem hiding this comment.
Related to this though, do we really need applyDynamic? When does that come up vs. select dynamic? A case that comes to mind is something like:
@instantiable
class MyModule extends RawModule {
@public val xs = List(1, 2, 3)
}
// ...
val inst = Instantiate(new MyModule)
inst.xs(1) // Needs rewriting to inst._lookup(_.xs(using Lookupable.Aux[List[Int], List[Int]])).apply(1)I can see this calling apply dyanmic so maybe that's what's going on. A comment explaining if that's the case would be good.
But my concern above still applies, what if you have a List[List[Int]]? There could be two applies in a row.
2 participants
Tested locally with (a working subset of) InstanceSpec and DefinitionSpec - will move all tests once other unsupported APIs are ported
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash)and clean up the commit message.Create a merge commit.