Skip to content

Fix #8799: Use initial denotation for signatures #8813

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1892,11 +1892,11 @@ object Types {
/** The signature of the last known denotation, or if there is none, the
* signature of the symbol
*/
protected def computeSignature(implicit ctx: Context): Signature = {
val lastd = lastDenotation
if (lastd != null) lastd.signature
else symbol.asSeenFrom(prefix).signature
}
protected def computeSignature(implicit ctx: Context): Signature =
lastDenotation match
case null => symbol.asSeenFrom(prefix).signature
case sd: SingleDenotation => sd.initial.signature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps Denotation#signature itself should do the work of calling initial? That way we're sure to never use the wrong signature

Copy link
Member

@smarter smarter Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case in point: the code just below for currentSignature calls lastd.signature and lastd might not be the initial denotation. Similarly, symbol.asSeenFrom(prefix).signature above will also be computed at the current phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as signatures are defined in denotations, I think they should apply to the owner denotation. If we wanted to always go for initial, then signatures should be defined on symbols.

Agree that currentSignature needs to be fixed as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to always go for initial, then signatures should be defined on symbols.

That might make sense, I can see how different prefixes can lead to different signatures for denotations, but I don't think that should ever be needed to disambiguate between overloads ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I guess that would probably interfere with how we deal with merging denotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it's better to leave them on denotations.

case d => d.signature

/** The signature of the current denotation if it is known without forcing.
* Otherwise the signature of the current symbol if it is known without forcing.
Expand Down
31 changes: 31 additions & 0 deletions compiler/test/dotty/tools/SignaturesTests.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package dotty.tools

import vulpix.TestConfiguration

import org.junit.Test

import dotc.ast.Trees._
import dotc.core.Decorators._
import dotc.core.Contexts._
import dotc.core.Types._
import dotc.core.Denotations._

import java.io.File
import java.nio.file._

class SignaturesTest:
@Test def signatureCaching: Unit =
inCompilerContext(TestConfiguration.basicClasspath, "case class Foo(value: Unit)") {
val defn = ctx.definitions
val leftCls = ctx.requiredClass("Foo")
val ref = leftCls.requiredMethod("value").termRef

def checkSig()(using Context): Unit =
val denot = ref.denot match
case sd: SingleDenotation => sd.initial
case d => d
assert(ref.signature == denot.signature, i"Wrong cached signature at phase ${ctx.phase} for $ref.\nActual denotation signature: ${denot.signature}\nCached ref signature: ${ref.signature}")

checkSig()
checkSig()(using ctx.withPhase(ctx.erasurePhase.next))
}
18 changes: 15 additions & 3 deletions compiler/test/dotty/tools/compilerSupport.scala
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package dotty.tools

import javax.tools._
import java.io.File
import java.io._
import java.nio.file._
import java.net.URI
import scala.jdk.CollectionConverters._
Expand All @@ -10,13 +10,25 @@ import core._
import core.Contexts._
import dotc.core.Comments.{ContextDoc, ContextDocstrings}

// TODO: refactor, copy-pasted from Run#compileFromStrings
private def sourceFile(source: String, isJava: Boolean): util.SourceFile = {
val uuid = java.util.UUID.randomUUID().toString
val ext = if (isJava) ".java" else ".scala"
val virtualFile = new io.VirtualFile(s"compileFromString-$uuid.$ext")
val writer = new BufferedWriter(new OutputStreamWriter(virtualFile.output, "UTF-8")) // buffering is still advised by javadoc
writer.write(source)
writer.close()
new util.SourceFile(virtualFile, scala.io.Codec.UTF8)
}

/** Initialize a compiler context with the given classpath and
* pass it to `op`.
*/
def inCompilerContext[T](classpath: String)(op: Context ?=> T): T =
def inCompilerContext[T](classpath: String, scalaSources: String*)(op: Context ?=> T): T =
val compiler = Compiler()
val run = compiler.newRun(initCtx(classpath))
run.compileUnits(Nil) // Initialize phases
run.compileUnits(scalaSources.toList.map(s =>
CompilationUnit(sourceFile(s, isJava = false))(using run.runContext)))
op(using run.runContext)

private def initCtx(classpath: String): Context =
Expand Down