-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix generic signature for type params bounded by primitive #16442
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
Conversation
compiler/src/dotty/tools/dotc/transform/GenericSignatures.scala
Outdated
Show resolved
Hide resolved
054dd3d
to
c7e88c2
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.
@dwijnand Can we now drop the check at https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala#L850-L854 as well?
import core.Types._ | ||
import core.classfile.ClassfileConstants | ||
import SymUtils._ | ||
import TypeUtils._ | ||
import config.Printers.transforms | ||
import reporting.trace |
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.
Imports w.r.t. debugging can be dropped
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'd love to never have to import them, so this is 1 more file that won't have to happen...
|
||
class Foo: | ||
def testNoParam[A <: Int]: A = 1.asInstanceOf[A] | ||
def testSingleParam[A <: Int](a: A): A = 2.asInstanceOf[A] // <A:Ljava/lang/Object;>(I)I |
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.
Shouldn't we drop the generic signature here entirely? In the end it's just int => int
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 it's a matter of taste a this point. From Java, there's no more link between any passed type argument and the term argument, but maybe there is still a type argument you can provide - so that's why I kept it, as well as it being easier to implement it this way. Unless we have a real reason to drop it, I think this is good.
I think I tried earlier and it failed (and I didn't look more into it at the time) but, good shout, because it looks like we can now. |
This reverts commit 5cdb001. Breaks in CI, here's a snippet: [info] Test dotty.tools.dotc.BootstrappedOnlyCompilationTests.posWithCompiler started -- Error: compiler/src/dotty/tools/dotc/transform/CapturedVars.scala:31:20 --------------------------------------------- 31 | private[this] var Captured: Store.Location[util.ReadOnlySet[Symbol]] = _ | ^ |compiler bug: created invalid generic signature for variable Captured in dotty.tools.dotc.transform.CapturedVars |signature: I |if this is reproducible, please report bug at https://github.com/lampepfl/dotty/issues |
c4fe39d
to
be10979
Compare
Fixes #15385