Implement value classes #411
Conversation
|
Failures here seem to be triggered by backend, but not caused by the backend itself: data races, stale symbols, etc. |
… references In scala#411 I implemented value classes and started getting errors in the backend. This commit reproduces similar errors in master with a single change: In master Symbol#isDerivedValueClass always return false because value classes are non-functional, after this commit it still always return false but it calls this.derivesFrom(defn.AnyValClass) first, this is enough to trigger the problem. Here's what's happening with dotc_reporter for example, the first error is: dotty.tools.dotc.core.Types$CyclicReference: cyclic reference involving class FlatHashTable - During Erasure, isDerivedValueClass is called on the class FlatHashTable - derivesFrom forces the computation of the base classes of FlatHashTable - HashUtils is a parent of FlatHashTable, its denotation is forced - HashUtils is then transformed by ExplicitOuter which calls isStatic on it - isStatic calls owner.isStaticOwner, this forces the denotation of the owner of HashUtils, which is the module class FlatHashTable$ - FlatHashTable$ is then transformed by ExtensionMethods which calls linkedClass on it to get the companion class FlatHashTable - isDerivedValueClass is called on the class FlatHashTable - *BOOM*
Using it in isDerivedValueClass fixes the bug showcased in scala#411.
smarter
force-pushed
the
add/value-classes
branch
from
515d41f
to
d83bd3e
Compare
Using it in isDerivedValueClass fixes the bug showcased in scala#411.
smarter
force-pushed
the
add/value-classes
branch
from
d83bd3e
to
2b2fb32
Compare
Using it in isDerivedValueClass fixes the bug showcased in scala#411.
smarter
force-pushed
the
add/value-classes
branch
4 times, most recently
from
a9fb3ba
to
789dc0c
Compare
smarter
force-pushed
the
add/value-classes
branch
5 times, most recently
from
47a446c
to
be4b748
Compare
smarter
changed the title
|
This PR is now complete, @DarkDimius and @odersky : please review! I'd especially like answers to the questions I asked in the |
| @@ -60,6 +60,8 @@ class Compiler { | |||
| new ElimByName, | |||
| new ResolveSuper), | |||
| List(new Erasure), | |||
| List(new ElimErasedValueType), | |||
There was a problem hiding this comment.
Is there a reason you need separate phases for those? Cannot they be blocked together with Mixin?
There was a problem hiding this comment.
Yes, I should probably do that
smarter
force-pushed
the
add/value-classes
branch
from
be4b748
to
650ef9a
Compare
| private def eraseResult(tp: Type)(implicit ctx: Context): Type = tp match { | ||
| case tp: TypeRef => | ||
| val sym = tp.typeSymbol | ||
| if (sym eq defn.UnitClass) sym.typeRef | ||
| else if (sym.isDerivedValueClass) eraseNormalClassRef(tp) | ||
| else if (isConstructor && isDerivedValueClass(sym)) eraseNormalClassRef(tp) |
There was a problem hiding this comment.
why are you testing for constructor here?
There was a problem hiding this comment.
The return type of the constructor of a value class should not be erased: new Meter(4) should have type Meter. The return type of any other method can be safely erased, Do you think this is worth a comment?
There was a problem hiding this comment.
Yes, I would say it is worth a comment, that also tells should it be Meter or EVT(Meter)
There was a problem hiding this comment.
After thinking more about it, I do not see why new Meter(4) is special here. There are numerous ways to instantiate a class(eg use clone on existing one), and from type system point of view new is not different from them. You also say later that you need to leave un-erased TypeTrees for other poly-methods, not only New.
Can you elaborate on this?
There was a problem hiding this comment.
Value classes are unboxed when there is a type mismatch that can be solved by adaptToType: if new Meter(5) has type Meter but the expected type is ErasedValueType(Meter, Int) then adaptToType will unbox new Meter(5). This is the correct behavior and will not happen if the type of new Meter(5) is ErasedValueType(Meter, Int) (which would be wrong because the constructor returns an object of type Meter). clone() is not a member of AnyVal so I don't need to handle it separately, but let me know if you can think of some other case I'd need to handle.
There was a problem hiding this comment.
Ok, resolved. But please include this comment in source code, or a short comment here, and a full one in markdown doc that you were proposing.
|
LGTM otherwise. Good job. |
smarter
force-pushed
the
add/value-classes
branch
from
4888369
to
a3b2643
Compare
|
No, this PR is complete, I just pushed a changed to rename 'origClosure' to 'implClosure'. Feel free to merge it! |
smarter
force-pushed
the
add/value-classes
branch
from
a3b2643
to
a0cfba0
Compare
This commit tries to disentangle the TypeErasure class and the TypeErasure object thereby fixing scala#386. - Remove the `eraseInfo` method in the TypeErasure object, use `transformInfo` instead which takes care of using the correct instance of TypeErasure depending on the symbol to erase. - Remove the unused method `eraseResult` in the TypeErasure class. - In `transformInfo`, use the correct instance of the TypeErasure class when calling `eraseInfo`. - In the `eraseInfo` method of the TypeErasure class, do not call the `erasure` method of the TypeErasure object, instead use the `apply` method of the current instance of TypeErasure.
- isSemi is replaced by semiEraseVCs with a different meaning (but is
still unimplemented):
* If true, value classes are semi-erased to ErasedValueType
(they will be fully erased in ElimErasedValueType which is not
yet present in this commit).
* If false, they are erased like normal classes.
- Fix the documentation of the TypeErasure class which was wrong.
- Remove intermediate functions scalaErasureFn, scalaSigFn, javaSigFn
and semiErasureFn. It's clearer to just use erasureFn directly
instead.
- Add an optional parameter semiEraseVCs to TypeErasure#erasure which
will be used in Erasure#Typer when we need to disable semi-erasure.
This reduces the number of objects created and speeds up subtyping tests Also make ErasedValueType extend ValueType
For a value class V, let U be the underlying type after erasure. We add to the companion object of V two cast methods: def u2evt$(x0: U): ErasedValueType(V, U) def evt2u$(x0: ErasedValueType(V, U)): U The casts are used in Erasure to make it typecheck, they are then removed in ElimErasedValueType (not yet present in this commit). This is different from the implementation of value classes in Scala 2 (see SIP-15) which uses `asInstanceOf` which does not typecheck.
There are three ways to erase a value class: - In most case, it should be semi-erased to an ErasedValueType, which will be fully erased to its underlying type in ElimErasedValueType. This corresponds to semiEraseVCs = true in TypeErasure. - In a few cases, it should be erased like a normal class, so far this seems to be necessary for: * The return type of a constructor * The underlying type of a ThisType * TypeTree nodes inside New nodes * TypeApply nodes * Arrays In these cases, we set semiEraseVCs = false - When calling `sigName` it should be erased to its underlying type. This commit implements all these cases. Note that this breaks most tests because ElimErasedValueType has not been implemented yet, it is part of the next commit.
This phase erases ErasedValueType to their underlying type, in scalac this was done in PostErasure.
This fixes the issues reported in SI-5866 and SI-8097
This method will be needed to implement VCInline.
This corresponds roughly to step 2 of SIP-15 and to the peephole optimizations of step 3. The extractors in TreeExtractors are copied or inspired from src/compiler/scala/tools/nsc/ast/TreeInfo.scala in scalac.
Each test needs to have its own package because pos_all will try to compile the whole valueclasses directory at once.
Each test needs to have its own package because pos_all will try to compile the whole valueclasses directory at once. The remaining tests with "extends AnyVal" in tests/pending/pos are related to separate compilation, except for: - t6482.scala and t7022.scala which were fixed by scala/scala#1468 in scalac and seem to trigger a similar bug in FullParameterization - strip-tvars-for-lubbasetypes.scala which was fixed by scala/scala#1758 in scalac
For a module class V$, the synthesized companion class method looks like: val companion$class: V If V is a value class, after erasure it will look like: val companion$class: ErasedValueType(V, ...) This will break SymDenotation#companionClass which relies on the type of companion$class. The solution is to not semi-erase the type of companion$class.
After erasure, we may have to replace the closure method by a bridge. LambdaMetaFactory handles this automatically for most types, but we have to deal with boxing and unboxing of value classes ourselves.
DarkDimius
force-pushed
the
add/value-classes
branch
from
a0cfba0
to
d012f93
Compare
|
Merging now. I'll add a PR that will include a compilation key to disable lazy vals, and will also define classtags that are needed for arrays. |
|
✨ 💫 🎊 🎉 |
|
Great to have that in! Thanks for the hard work, Guillaume! On Fri, May 1, 2015 at 3:19 PM, Guillaume Martres notifications@github.com
Martin Odersky |
This implements value classes by roughly following SIP-15, see the description of each individual commit for the details.