Applied types, unoptimized #3061
Conversation
There was a problem hiding this comment.
Hello, and thank you for opening this PR! 🎉
All contributors have signed the CLA, thank you! ❤️
Commit Messages
We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).
Please stick to these guidelines for commit messages:
- Separate subject from body with a blank line
- When fixing an issue, start your commit message with
Fix #<ISSUE-NBR>:- Limit the subject line to 72 characters
- Capitalize the subject line
- Do not end the subject line with a period
- Use the imperative mood in the subject line ("Added" instead of "Add")
- Wrap the body at 80 characters
- Use the body to explain what and why vs. how
adapted from https://chris.beams.io/posts/git-commit
Have an awesome day! ☀️
|
test performance please |
|
performance test scheduled: 1 jobs in total. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/3061 to see the changes. Benchmarks for this PR is based on the merge of this PR (d92e3e6) with master |
| yield new HasProblemBounds(mbr) | ||
|
|
||
| def baseTypeProblems(base: Type) = base match { | ||
| case AndType(base1, base2) => |
There was a problem hiding this comment.
Complication: Realizablility used to just check that all type members have good bounds. Now it has to look at basetypes as well.
| def apply(tp: Type) = mapOver { | ||
| tp match { | ||
| case tp: RefinedType if param occursIn tp.refinedInfo => tp.parent | ||
| case tp @ AppliedType(tycon, args) => | ||
| tp.derivedAppliedType(tycon, |
There was a problem hiding this comment.
I think this case was missing before, we'd need one for HKApply anyway.
| case _: TypeBounds => | ||
| if (fromBelow) addLess(bound, param) else addLess(param, bound) | ||
| case tp => | ||
| if (fromBelow) isSubType(bound, tp) else isSubType(tp, bound) |
There was a problem hiding this comment.
Added case prompted by a failure. But it looks like we'd need it anyway, and we just failed to have a test that exercises this pass before.
| @@ -335,7 +334,7 @@ object Contexts { | |||
| * from constructor parameters to class parameter accessors. | |||
| */ | |||
| def superCallContext: Context = { | |||
| val locals = newScopeWith(owner.asClass.paramAccessors: _*) | |||
| val locals = newScopeWith(owner.typeParams ++ owner.asClass.paramAccessors: _*) | |||
There was a problem hiding this comment.
Type parameters are no longer accessors, need to be mentioned explicitly.
| "uniqueTypeAliases" -> uniqueTypeAliases) | ||
| "uniqueAppliedTypes" -> uniqueAppliedTypes, | ||
| "uniqueWithFixedSyms" -> uniqueWithFixedSyms, | ||
| "uniqueNamedTypes" -> uniqueNamedTypes) |
There was a problem hiding this comment.
Optimize for different set of common types
| /** A binding for a type parameter of a base class or trait. | ||
| */ | ||
| final val BaseTypeArg = typeFlag(25, "<basetypearg>") | ||
|
|
There was a problem hiding this comment.
Simplification: No more BaseTypeArgs
| @@ -38,6 +38,15 @@ abstract class Periods extends DotClass { self: Context => | |||
| } | |||
| Period(runId, first, nxTrans) | |||
| } | |||
|
|
|||
| /** Are all base types in the current period guaranteed to be the same as in period `p`? */ | |||
| def hasSameBaseTypesAs(p: Period) = { | |||
There was a problem hiding this comment.
Refactored out because we test this now in several places
| @@ -84,19 +84,12 @@ object Config { | |||
| /** If this flag is set, take the fast path when comparing same-named type-aliases and types */ | |||
| final val fastPathForRefinedSubtype = true | |||
|
|
|||
| /** If this flag is set, `TypeOps.normalizeToClassRefs` will insert forwarders | |||
| * for type parameters of base classes. This is an optimization, which avoids | |||
| * long alias chains. We should not rely on the optimization, though. So changing | |||
There was a problem hiding this comment.
Simplification: No paramForwarding or suspensions needed
| @@ -1382,6 +1379,14 @@ object SymDenotations { | |||
| NoSymbol | |||
| } | |||
|
|
|||
| /** The explicitly given self type (self types of modules are assumed to be | |||
| * explcitly given here). | |||
There was a problem hiding this comment.
Refactoring: givenSelfType moved from ClassInfo to ClassDenotation
| case tp: AndOrType => inCache(tp.tp1) && inCache(tp.tp2) | ||
| //case tp: TypeProxy => inCache(tp.underlying) // disabled, can re-enable insyead of last two lines for performance testing | ||
| //case tp: AndOrType => inCache(tp.tp1) && inCache(tp.tp2) | ||
| case tp: TypeProxy => isCachable(tp.underlying, btrCache) |
There was a problem hiding this comment.
It seems we were needlessly pessimistic before and therefore cached less than we could have.
| else subcls.denot match { | ||
| case cdenot: ClassDenotation => | ||
| if (cdenot.baseClassSet contains symbol) foldGlb(NoType, tp.parents) | ||
| case tp @ TypeRef(prefix, _) => |
There was a problem hiding this comment.
Complication: Compute applied base types instead of simple TypeRefs.
| case tp: TypeBounds => throw new AssertionError("no TypeBounds allowed") | ||
| case _ => tp | ||
| } | ||
|
|
||
| /** If `tp` is a TypeBounds instance return its lower bound else return `tp` */ |
There was a problem hiding this comment.
Refactoring: Moved from TypeApplications to Types (as loBound, hiBound)
| case _ => None | ||
| } | ||
| } | ||
|
|
||
| /** Extractor for type application T[U_1, ..., U_n]. This is the refined type | ||
| * |
There was a problem hiding this comment.
Simplification: AppliedTypes are case classes, no extractor needed.
| case _ => | ||
| appliedTo(args) | ||
| } | ||
|
|
||
| /** Turn this type, which is used as an argument for | ||
| * type parameter `tparam`, into a TypeBounds RHS | ||
| */ |
There was a problem hiding this comment.
Simplification: Everything is now handled by baseType, no additional methods needed.
|
|
||
| /** Argument types where existential types in arguments are approximated by their upper bound */ | ||
| def argTypesHi(implicit ctx: Context) = argInfos mapConserve boundsToHi | ||
|
|
There was a problem hiding this comment.
Simplification: withoutArgs is replaced by typeConstructor
| compareLower(tycon2.info.bounds, tyconIsTypeRef = true) | ||
| isMatchingApply(tp1) || { | ||
| tycon2.info match { | ||
| case info2: TypeBounds => |
There was a problem hiding this comment.
Complication: Need to compare baseclasses of lhs against full rhs if it is an AppliedType
| val tparam = tparams.head | ||
| val v = tparam.paramVariance | ||
|
|
||
| def compareCaptured(arg1: Type, arg2: Type): Boolean = arg1 match { |
There was a problem hiding this comment.
Complication: Need to do capture conversion when comparing an applied type with wildcard arguments.
| isSubArg(arg1, arg2) || { | ||
| // last effort: try to adapt variances of higher-kinded types if this is sound. | ||
| // TODO: Move this to eta-expansion? | ||
| val adapted2 = arg2.adaptHkVariances(tparam.paramInfo) |
There was a problem hiding this comment.
Complication: Need to adapt higher-kinded variances when comparing applied types as well as refined types.
| @@ -1203,6 +1244,56 @@ class TypeComparer(initctx: Context) extends DotClass with ConstraintHandling { | |||
| final def lub(tps: List[Type]): Type = | |||
| ((defn.NothingType: Type) /: tps)(lub(_,_, canConstrain = false)) | |||
|
|
|||
| def lubArgs(args1: List[Type], args2: List[Type], tparams: List[TypeParamInfo], canConstrain: Boolean = false): List[Type] = | |||
There was a problem hiding this comment.
Complications: Need added case for lub and glb of applied types, which mimic the simplifications done for refined types.
| // Given two refinements `T1 { X = S1 }` and `T2 { X = S2 }` rewrite to | ||
| // `T1 & T2 { X B }` where `B` is the conjunction of the bounds of `X` in `T1` and `T2`. | ||
| // | ||
| // However, if `homogenizeArgs` is set, and both aliases `X = Si` are |
There was a problem hiding this comment.
Refactoring: Moved to glbArgs
| tp.derivedRefinedType(simplify(tp.parent, theMap), tp.refinedName, simplify(tp.refinedInfo, theMap)) | ||
| case tp: TypeAlias => | ||
| tp.derivedTypeAlias(simplify(tp.alias, theMap)) | ||
| case AndType(l, r) => | ||
| case AndType(l, r) if !ctx.mode.is(Mode.Type) => |
There was a problem hiding this comment.
Improvement: Don't simplify & and | types given as types
| mergeRefinedOrApplied(parent1, parent2), name1, rinfo1 | rinfo2) | ||
| case _ => fail | ||
| } | ||
| case tp1 @ AppliedType(tycon1, args1) => |
There was a problem hiding this comment.
Complication: need to duplicate merge logic between RefinedTypes and AppliedTypes
| } | ||
| */ | ||
|
|
||
| private def enterArgBinding(formal: Symbol, info: Type, cls: ClassSymbol, decls: Scope) = { |
There was a problem hiding this comment.
Simplifications: No more enterArgBinding, normalizeToClassRefs and related operations
| @@ -154,14 +149,6 @@ object Types { | |||
| false | |||
| } | |||
|
|
|||
| def isInfixType(implicit ctx: Context): Boolean = this match { | |||
| case TypeApplications.AppliedType(tycon, args) => | |||
There was a problem hiding this comment.
Refactoring: Moved back to RefinedPrinter
| case tp @ AppliedType(tycon, args) if tycon.typeSymbol.isClass => | ||
| tycon.parents.map(_.subst(tycon.typeSymbol.typeParams, args)) // @!!! cache? | ||
| case tp: TypeRef => | ||
| if (tp.info.isInstanceOf[TempClassInfo]) { |
There was a problem hiding this comment.
Complication: Handle missing parents due to incomplete denotations here. (But it's better than to do it via suspensions as was done before).
|
|
||
| /** The argument corresponding to class type parameter `tparam` as seen from | ||
| * prefix `pre`. | ||
| */ |
There was a problem hiding this comment.
Complication: Need specific logic to substitute an actual argument for a formal parameter. This is somewhat more involved than the previous matchParams, which was removed.
- explain why checkRealizable better - add test case for bad bounds in basetypes - drop dead code - documentation fixes - fixes to lubArgs/glbArgs
- eliminate redundant guard - eliminate isClassParam method
Ensure that hkResult and typeParams are in sync, except that typeParams work also for classes.
This allows us to re-instantiate existentials.scala.
odersky
force-pushed
the
test-applied-type
branch
from
1890815 to
aa9709b
Compare
There's a scenario where a NamedType is created with an initial symbol but no denotation yet. If the denotation of that NamedType is first computed in a subsequent run, the initially given symbol might be stale, so the symbol should be recomputed from the denotation.
odersky
force-pushed
the
test-applied-type
branch
from
aa9709b to
d2b7080
Compare
|
Yes, I'll do a final pass today
|
There was a problem hiding this comment.
LGTM, let's get this in! :) But I'd like to see the MajorVersion in TastyFormat.scala bumped. This is good practice and should also avoid weird error messages when updating a dotty git clone and compiling the bootstrapped compiler (if the bootstrapped compiler has been compiled before, incremental compilation will try to read the old tasty sections and get confused).
I'll do that as part of my-follow-on work on NamedTypes. |
|
test performance please The compilation failure of https://github.com/liufengyun/scalaPB on the bench server originates from this PR. |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
performance test failed: [check /data/workspace/bench/logs/pull-3061-09-25-15.42.out for more information] |
Types in union need to be fully applied, the previous code was wrong but happened to work before scala/scala3#3061. See scala/scala3#3161.
This PR covers AppliedTypes before any attempt at optimizations. Recent experience has shown that attempts as optimizations often have unintuitive results, so we should test them very carefully and measure each of them individually.