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
SI-10260 better error messages for raw Java types #5847
Conversation
". To implement a raw type, use %s[_]".format(pa) | ||
if (underlying.isJavaDefined && pa.typeArgs.isEmpty && abstractSym.typeParams.nonEmpty) { | ||
prepareRawTypeForMessage(abstractSym).fold( | ||
". To implement a raw type, use %s[_]".format(pa) |
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.
I think this case should actually never happen. Am I right? Should that be an error instead of a default then? If yes, which?
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.
Yep, I think you can implement this message as s". To implement this raw type, use ${rawToExistential(pa)}"
with a slight variation on a helper method it uses (patch below).
--- a/src/reflect/scala/reflect/internal/Types.scala
+++ b/src/reflect/scala/reflect/internal/Types.scala
@@ -3877,7 +3877,7 @@ trait Types
def typeParamsToExistentials(clazz: Symbol, tparams: List[Symbol]): List[Symbol] = {
val eparams = mapWithIndex(tparams)((tparam, i) =>
- clazz.newExistential(newTypeName("?"+i), clazz.pos) setInfo tparam.info.bounds)
+ clazz.newExistential(tparam.name.toTypeName, clazz.pos) setInfo tparam.info.bounds)
eparams map (_ substInfo (tparams, eparams))
}
Would you be interested in pursuing this change and see if we can make this work for the whole compiler, or alternatively make a second rawToExistentialPreservingNames
?
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.
Yes I will look into this in a week or so, when I have access to a real computer. By 'make this work for the whole compiler' do you mean resolving all subsequent test failures?
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.
Yes, it's worth a shot, I think, but it could also be too hairy. If you could experiment with this a bit and report back, we can decide whether to duplicate the rawToExistential transform or not.
9eb8631
to
86d21bd
Compare
Review by @adriaanm ? |
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.
Thanks for looking into this! Sorry about the delay.
I agree the current message is a bit confusing. I think we can implement this more directly, as suggested below. Also, instead of "better error message", could you sharpen the title? ("Display raw type as existential" or something like that)
". To implement a raw type, use %s[_]".format(pa) | ||
if (underlying.isJavaDefined && pa.typeArgs.isEmpty && abstractSym.typeParams.nonEmpty) { | ||
prepareRawTypeForMessage(abstractSym).fold( | ||
". To implement a raw type, use %s[_]".format(pa) |
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.
Yep, I think you can implement this message as s". To implement this raw type, use ${rawToExistential(pa)}"
with a slight variation on a helper method it uses (patch below).
--- a/src/reflect/scala/reflect/internal/Types.scala
+++ b/src/reflect/scala/reflect/internal/Types.scala
@@ -3877,7 +3877,7 @@ trait Types
def typeParamsToExistentials(clazz: Symbol, tparams: List[Symbol]): List[Symbol] = {
val eparams = mapWithIndex(tparams)((tparam, i) =>
- clazz.newExistential(newTypeName("?"+i), clazz.pos) setInfo tparam.info.bounds)
+ clazz.newExistential(tparam.name.toTypeName, clazz.pos) setInfo tparam.info.bounds)
eparams map (_ substInfo (tparams, eparams))
}
Would you be interested in pursuing this change and see if we can make this work for the whole compiler, or alternatively make a second rawToExistentialPreservingNames
?
@adriaanm As far as I can see only the following tests fail:
To my untrained eyes they all look like |
Cool! Could you submit the updated check files so we can have a look (more easily -- sorry, a bit swamped)? @retronym, what do you think? |
Before, when implementing a raw type in a method override in the wrong way, the error message always suggested a simple wildcard type. This commit changes that error message to always suggest an existential type that the user can copy paste into his code. To that end `typeParamsToExistentials` is changed to preserve the names of the type parameters instead of changing them to "?0".."?N". fixes scala/bug#10260
No prob. |
@@ -1,4 +1,4 @@ | |||
Test_2.scala:9: error: value exxx is not a member of ?0 | |||
Test_2.scala:9: error: value exxx is not a member of T |
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.
would be interesting to investigate this one -- should indicate it's an existential skolem
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.
Approving because I think it's a step in the right direction. I think it could still be refined (will the user get confused because the existentially quantified names are now the same as the universally quantified, i.e., the class's own type params?). The whole cooking Java raw types thing is a big hack, though,... We really shouldn't use modifyInfo to destroy the original info in cookJavaRawInfo, but I don't see any other way either (except for considering this conversion every time).
Thanks for your patience. |
PS: de groeten daar in Heverlee ;-) |
Thanks ;) maar ik ben een week geleden vertrokken in Heverlee :D |
Attempt at fixing scala/bug#10260. It's not very pretty, but since compilation will fail anyway I assume a lot is permitted. You can now basically copy&paste the type that the error message suggests.