-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework reflect Constant API #10753
Rework reflect Constant API #10753
Conversation
5038206
to
67bfb75
Compare
Add types for specific constants as in the tasty format specification. This also aligns the API design with the rest of reflection by useing `TypeTest`s. ```diff - Constant.Int(x) + IntConstant(x) - Constant.Long(x) + LongConstant(x) ... ```
67bfb75
to
13b9679
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems we have too many concepts here. Can we just keep one concept Constant
as it's in the compiler?
class Constant(...) {
def isByteRange: Boolean
def isShortRange: Boolean
def isCharRange: Boolean
def isIntRange: Boolean
def isLongRange: Boolean
def isFloatRange: Boolean
def isNumeric: Boolean
def isNonUnitAnyVal: Boolean
def isAnyVal: Boolean
def booleanValue: Boolean
def byteValue: Byte
def shortValue: Short
def charValue: Char
def intValue: Int
def longValue: Long
def floatValue: Float
def doubleValue: Double
}
That API does not allow you to know what the type of the constant is. It also does not provide concrete constructors. |
Those are all implementation details that are not really in TASTy |
This is the only part of the abstraction we can relly on being stable https://github.com/lampepfl/dotty/blob/master/tasty/src/dotty/tools/tasty/TastyFormat.scala#L134-L146 |
In this PR we do not increase the number of concepts, we just move them to follow the same API design as |
It's indirectly decided by the language spec, which is more stable than TASTY. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I believe there is room to have simpler and cleaner APIs without introducing so many concepts. But as the usability aspect is subjective, and the constant APIs are not commonly used in meta-programming, thus I think the changes are fine.
I think this is a Much uglier API then before. Compare this: term match
case Literal(StringConstant(v: String)) => Some(v)
case Literal(IntConstant(v: Int)) => Some(v)
case Literal(LongConstant(v: Long)) => Some(v)
case Literal(BooleanConstant(v: Boolean)) => Some(v)
case Literal(FloatConstant(v: Float)) => Some(v)
case Literal(DoubleConstant(v: Double)) => Some(v)
case Literal(ByteConstant(v: Byte)) => Some(v)
case _ => None To this: term match
case Literal(Constant.String(v: String)) => Some(v)
case Literal(Constant.Int(v: Int)) => Some(v)
case Literal(Constant.Long(v: Long)) => Some(v)
case Literal(Constant.Boolean(v: Boolean)) => Some(v)
case Literal(Constant.Float(v: Float)) => Some(v)
case Literal(Constant.Double(v: Double)) => Some(v)
case Literal(Constant.Byte(v: Byte)) => Some(v)
case _ => None This StringConstant, IntConstant, etc... reminds me of Java's Hadoop's IntWritable, LongWritable, DoubleWritable. That was a nasty way of doing things I was hoping I would not need to work with again. Can we at least have Constant.String, Constant.Int etc... available as an unapply? |
Actually you just need term match
case Literal(StringConstant(v)) => Some(v)
case Literal(IntConstant(v)) => Some(v)
case Literal(LongConstant(v)) => Some(v)
case Literal(BooleanConstant(v)) => Some(v)
case Literal(FloatConstant(v)) => Some(v)
case Literal(DoubleConstant(v)) => Some(v)
case Literal(ByteConstant(v)) => Some(v)
case _ => None |
That makes sense. Thanks! |
I also liked it but it made more sense to change it for the sake of consistency with the rest of the API. This will help with doc navigation which is already non-trivial. |
Could I somehow wire this up myself? I recall in Scala 2 trying to do |
Add types for specific constants as in the tasty format specification. This also aligns the API design with the rest of reflection by useing
TypeTest
s (this inconsistency was noticed in #10738 (comment)).