Pattern matcher: extractors become name-based. #2835
Conversation
|
There's still probably plenty to quibble with, but this at least presents a basis for discussion. |
|
I am assuming the failure in pr-scala-distpack is not on my conscience. |
|
I'm a fan of the refactoring in d37e629, but could you please make a rough summary of behavior changes in that commit. I went through the codegen bit, but the behavior signal in match translation and typers is hard to separate from the refactoring "noise". Based on initial skimming, I expect a swift path to LGTM. |
|
I will jump to the conclusion that the failures in pr-scala-integrate-partest aren't my doing either. |
|
That last push accounts for 7214. I'm not going to bother with any please-rebuilds because I assume it will just fail again. |
It's a source of constant peril that sym.tpe on NoSymbol is fine (it's NoType) but tpe memberType sym on NoSymbol throws a NSDNHO. The last thing we should be doing is discouraging people from using memberType in favor of sym.tpe, the latter being almost always the wrong thing.
Motivated by pattern matcher work, also useful elsewhere.
The new method is the same as sameMethodAndFieldSignatures, but ignores generic signatures. This allows for testing methods which receive the same descriptor but differing generic signatures. In particular, this happens with value classes, which get a generic signature where a method written in terms of the underlying values does not.
We should do a lot more of this - it's ridiculously difficult and error prone to generate code of this kind involving implicits, type inference, etc. where the same goal is trivially accomplished by generating a method call and letting the typer work out the details.
Because who doesn't want a little positioning in their life.
All parents of an intersection type must be checkable for the type to be checkable.
This exploits the infrastructure developed for checking the checkability of type patterns to improve pattern type inference, which suffered from a similar deficit. There was a hack for SI-2486, the best I could manage at the time I wrote it; that is replaced with the principled approach.
This makes it a lot less error prone and redundant to find the part you need when unwrapping an UnApply tree.
An extractor is no longer required to return Option[T], and
can instead return anything which directly contains methods
with these signatures:
def isEmpty: Boolean
def get: T
If the type of get contains methods with the names of
product selectors (_1, _2, etc.) then the type and arity
of the extraction is inferred from the type of get. If
it does not contain _1, then it is a single value
extractor analogous like Option[T].
This has significant benefits and opens new territory:
- an AnyVal based Option-like class can be used which
leverages null as None, and no allocations are necessary
- for primitive types the benefit is squared (see below)
- the performance difference between case classes and
extractors should now be largely eliminated
- this in turn allows us to recapture great swaths of
memory which are currently squandered (e.g. every
TypeRef has fields for pre and args, even though these
are more than half the time NoPrefix and Nil)
Here is a primitive example:
final class OptInt(val x: Int) extends AnyVal {
def get: Int = x
def isEmpty = x == Int.MinValue // or whatever is appropriate
}
// This boxes TWICE: Int => Integer => Some(Integer)
def unapply(x: Int): Option[Int]
// This boxes NONCE
def unapply(x: Int): OptInt
As a multi-value example, after I contribute some methods to TypeRef:
def isEmpty = false
def get = this
def _1 = pre
def _2 = sym
def _3 = args
Then it's extractor becomes
def unapply(x: TypeRef) = x
Which, it need hardly be said, involves no allocations.
That was a lot harder than it looks. The dueling test cases for various corners make it sensitive to the slightest change in the logic. And of course it never helps that correctness depends on ad hoc in-place overloading resolution, which is now performed in a method called inPlaceAdHocOverloadingResolution so it's a little more obvious.
|
I went through the whole diff. Excellent work. Good to merge on Monday, as far as I'm concerned. Your refactoring is a great improvement, but, for my understanding, is there anything in MatchTranslation that is crucial for the new translation? Here are my Cliff's notes for understanding the new implementation of the new behavior:
Anything I missed? |
| def resultType = tpe.finalResultType | ||
| def method = unapplyMember(tpe) | ||
| def paramType = firstParamType(unapplyType) | ||
| def rawGet = if (isBool) UnitTpe else resultOfMatchingMethod(resultType, "get")() |
|
Gone are the days of oppression. Unapply macros are finally making it into trunk! As brought up by Denys yesterday, this pull request will finally allow us to turn unapplications into macros without resorting to insane hacks like manual assembly of UnApply nodes. Oh yeah! |
|
Could you submit a follow up pr with a few test cases for unapply macros? Would be great to nail them down. IntelliJ will also need to be updated with the relaxed rules for extractors. SIP worthy? /cc @Alefas |
|
@retronym Yes, that's what I'm doing right now. |
|
@retronym and all: https://github.com/paulp/scala/pull/8 establishes a pattern that can be used to implement extractor macros, explains how to implement it and provides examples in tests. |
|
@paulp Btw when playing with extractor macros, I noticed a gotcha: Is this intended? |
|
@xeno-by this has been bugging me and I'm not sure what to do about it. I was going to bring it up today. It's only an issue with Product1 (or something else which defines _1 and not _2 for whatever reason) but in such cases I don't know what the right thing is to do. I'm surprised it prints oops, I thought the logic would have it printing 2, but I'm not sure 2 isn't a problem also. |
|
@adriaanm I got stuck in some other stuff yesterday, I will elaborate/comment today. |
|
It seems the IDE validation fails with a compilation error: (the Jenkins job looks blue, though): |
|
We had to make the jenkins job always succeed because the build flow plugin's ignore combinator is broken. |
|
@dragos thanks for pointing it out, that looks like a error which needs addressing here. |
|
@paulp for the record, the overloaded method is defined in a trait hierarchy in PreferenceDescriptor.scala |
|
I'm sure I'll be able to fix it. This is one of those brutal things where some arbitrary overloading resolution is done in-place on the tree (directly modifying the symbol and type in the midst of typing) so of course no matter how one touches it something somewhere will break. |
|
@adriaanm I'm going through the most recent.. Do you have a tip on how to find only failures? I'm clicking my way through them, but it's very time consuming. |
|
See my comment in #2848 ; there was definitely a bug on my end which was shown in scalariform, but I don't know to what extent that will explain the current crop of failures. |
|
Anywhere you see this: That's my bug. |
|
@dragos: The script does the grepping for you, should be extensible to find other failures. To clarify: the script I attached in that thread. I've gisted it: https://gist.github.com/adriaanm/6258066 |
|
Superseded by #2848. |
|
@dragos I've reproduced the overload issue, hopefully that will be the hard part. I'm sort of impressed we have no test which exercises this. It doesn't even have to be overloaded. |
An extractor is no longer required to return Option[T], and
can instead return anything which directly contains methods
with these signatures:
If the type of get contains methods with the names of
product selectors (_1, _2, etc.) then the type and arity
of the extraction is inferred from the type of get. If
it does not contain _1, then it is a single value
extractor analogous like Option[T].
This has significant benefits and opens new territory:
leverages null as None, and no allocations are necessary
extractors should now be largely eliminated
memory which are currently squandered (e.g. every
TypeRef has fields for pre and args, even though these
are more than half the time NoPrefix and Nil)
Here is a primitive example:
As a multi-value example, after I contribute some methods to TypeRef:
Then its extractor becomes
Which, it need hardly be said, involves no allocations.