Allow inter-parameter dependencies #2079
Conversation
|
#2078 merged, you should be able to rebase. |
| @@ -233,6 +233,12 @@ object ProtoTypes { | |||
| typer.adapt(targ, formal, arg) | |||
| } | |||
|
|
|||
| /** The type of the argument `arg`. | |||
| * @pre `arg` ahs been typed before | |||
| addArg(typedArg(arg, formal), formal) | ||
| if (methodType.isParamDependent) | ||
| _.substParam(MethodParam(methodType, n), typeOfArg(arg)) |
There was a problem hiding this comment.
Possible performance concern: The resulting type transformer is applied below using formals1.mapconserve(substParam), so we do O(|formals|^2) substitutions.
It might be worth collecting just the substitution pairs, by returning Option[(MethodParam, Type)] instead, and whenever we finalize an argument i, we apply all of them using formal_i.substParam(listOfMts, listOfTps). This would require adding a substituter that can handle multiple ParamTypes at once (cf. Type.substParam).
Moreover, when adding arg#i we could avoid mapping over the first i parameters in formals1, since only parameters j > i can depend on i.
There was a problem hiding this comment.
I am not convinced it will be a problem. First, how many methods will be paramDependent? Then, how long is the typical parameter list? If it is 2, then the code given is the best (just one substitution). If it is 3 or 4, not much worse. Also, note that single parameter substitutions are faster than multiple parameter ones which require a linear search anyway. So, in summary, let's wait until the profiler indicates this is a hotspot.
| @@ -278,7 +278,8 @@ class TreeUnpickler(reader: TastyReader, tastyName: TastyName.Table, posUnpickle | |||
| result | |||
| case METHODtype => | |||
| val (names, paramReader) = readNamesSkipParams | |||
| val result = MethodType(names.map(_.toTermName), paramReader.readParamTypes[Type](end))( | |||
| val result = MethodType(names.map(_.toTermName))( | |||
| mt => paramReader.readParamTypes[Type](end), // !!! | |||
There was a problem hiding this comment.
That "!!!" is slightly alarming :-)
I suppose you are referring to the fact that we now rely on the constructor calling the two closures in the right order, which is somewhat risky indeed. Why not read them explicitly and construct a constant function?
| /** Check that method parameter types do not reference their own parameter | ||
| * or later parameters in the same parameter section. | ||
| */ | ||
| def checkNoForwardDependencies(vparams: List[ValDef])(implicit ctx: Context): Unit = vparams match { |
There was a problem hiding this comment.
I'm sure there's a good reason, but it seems that here we are duplicating the checking provided by Config.checkMethodTypes. Why do we need both?
There was a problem hiding this comment.
One is a source check, the other an internal assertion that results in an exception. Not all method types are created directly from source, so the assertion is useful.
| def f(x: C, y: x.T): x.T = y // ok | ||
|
|
||
| val c = new C { type T = String } | ||
| val c2 = c |
There was a problem hiding this comment.
I think we only added val c2 to see what type is inferred, so we can now remove it, no? A similar thing goes for the val d below, I think.
There was a problem hiding this comment.
Yes, but it's a test case. Does not really matter either way.
|
(I unfortunately put one of my comments in the outdated version of TreeUnpickler.scala, so the comment is collapsed by default. I think the point still stands, please have a look.) |
To allow for dependencies between method type parameters, construct MethodTypes from a closure that maps the currently constructed MethodType to its parameter types.
Also: check validity of method types, so that no forward references occur.
Take parameter dependencies into account when typechecking arguments.
The dropped method takes direct parameter types but a result type expression. Since parameter types are now in general dependent as well, that method is mostly redundant.
All PolyTypes get variances passed, so isTypeLambda is always true and the deleted assert is never triggered.
Allow a later parameter's type to refer to an earlier parameter in the same parameter section. Previously, we allowed only references to parameters in preceding sections. This is simpler, but it
can force unnatural currying. The problem becomes urgent only if we want to add dependent types more broadly. It feels natural to write
and being forced to curry this as
def fun(n: Int)(v: Vec(n))is annoying.The changes in this PR have given me an idea for a much more sweeping change. We might be able to unify MethodTypes and PolyTypes, which would improve regularity of the language quite significantly and which would open up lots of new possibilities. But one thing after another.
Review by @gsps?