Encode static-final constant fields as Singletons#2085
Conversation
…pi changes, they change.
|
Hi @stuhood, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
|
Can one of the admins verify this patch? |
|
@stuhood on it now, sorry for delay. |
There was a problem hiding this comment.
I think maybe the TODO shoudl be about reading as much of the APi as possible from a classfile parser rather than reflection. Reflection locks us into a specific JDK we have to be running on, where as parsing frees that.
|
This LGTM. My comments are all very minor, but usability related. Can't wait to see more of these :) |
|
Skimming on BART, I'm wondering whether the compiler's ConstantType is available here? When type checking, a final val without an expected type is typed as a ConstantType that tracks its value, denoting it can be constant folded. In general, it's not safe to do this for a SingletonType. I'll have a deeper look at the latest when M2 is out. |
|
@adriaanm We may need to add a new subclass to denote ConstantType. you're right that may be better as an option in the future. Shouldn't be a difficult change to make. Let use know after M2 :) |
|
Cleanups look great!, just waiting on @adriaanm if he has any concerns over how we've encoded the types of the constant pool |
There was a problem hiding this comment.
I think it would be useful to elaborate a bit on why these parts (class name, field name, constant value) are included in the singleton type's path. The thought process of a future incremental compiler hacker would have to go something like this. If the class name changes, but the constant does not, should we recompile? (I assume we don't.) So, does it suffice to include the value? But wait! If you change the constant's value, and not the class name, the change won't be detected, so I should include the reference to the field to encode the dependency.
How about the type of the field (not captured here), though? Does it matter whether I go from int to long, as, erm, long as the value doesn't change?
There was a problem hiding this comment.
Regarding my earlier comment, I think it's okay not to add ConstantType to the API, but I would suggest including a marker (:snail:, to evoke its constant nature?) in the singleton type's path to make sure we can tell the difference if we ever need to.
There was a problem hiding this comment.
That's a lot of questions, none of which I know the answer to. "baby's first sbt/scala patch", and so forth.
What would you suggest?
There was a problem hiding this comment.
Sorry, I should've used a more specific format for that feedback.
I think your encoding makes sense, but it does not capture the type of the field, which is unlikely not to also result in a change of value when it changes, but it could (and thus changes to the type of the field would go undetected).
I'd also like to see some docs for the motivation of choosing these elements for the path. I think my rhetorical questions could serve as a basis for that.
There was a problem hiding this comment.
I think your encoding makes sense, but it does not capture the type of the field, which is unlikely not to also result in a change of value when it changes, but it could (and thus changes to the type of the field would go undetected).
Aha! Absolutely right. Will fix.
There was a problem hiding this comment.
I've added a test for the type change, and expanded the explanation around the choice of members in the Singleton type. Thanks!
|
This is great stuff, thanks! I added a suggestion or two. |
|
Thanks @adriaanm, all good points. |
|
This needs notes as well btw. |
|
LGTM |
…-fields-as-singletons Encode static-final constant fields as Singletons
|
Thank you! I love the template approach to testing compilation. Hypnotoad approves. |
…nal-fields-as-singletons Encode static-final constant fields as Singletons
... so that when their values change, the APIs change and downstream consumers are invalidated. See #1967
There are 3 TODOs in the code currently:
JavacForUnitTestsclass, and move the new test to the appropriate package