Skip to content

Fix #10131: load enum types more lazily #10135

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 2, 2020
Merged

Fix #10131: load enum types more lazily #10135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d633b4c
Fix #10131: load enum types more lazily
liufengyun Nov 1, 2020
9dc145d
Switch CI from JDK 11 to JDK 14
liufengyun Nov 2, 2020
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
28 changes: 14 additions & 14 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ on:
push:
paths-ignore:
# Do not run everything on changes only in docs
- 'scala3doc/**'
- 'scala3doc/**'
- 'scala3doc-testcases/**'
pull_request:
paths-ignore:
# Do not run everything on changes only in docs
- 'scala3doc/**'
- 'scala3doc/**'
- 'scala3doc-testcases/**'
schedule:
- cron: '0 3 * * *' # Every day at 3 AM
Expand All @@ -20,13 +20,13 @@ env:
jobs:
test:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
if: "!(github.event_name == 'push' &&
startsWith(github.event.ref, 'refs/tags/sbt-dotty-'))"

steps:
- name: Set JDK 11 as default
run: echo "/usr/lib/jvm/java-11-openjdk-amd64/bin" >> $GITHUB_PATH
run: echo "/usr/lib/jvm/java-14-openjdk-amd64/bin" >> $GITHUB_PATH

- name: Checkout cleanup script
uses: actions/checkout@v2
Expand Down Expand Up @@ -65,13 +65,13 @@ jobs:

test_bootstrapped:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
if: "!(github.event_name == 'push' &&
startsWith(github.event.ref, 'refs/tags/sbt-dotty-'))"

steps:
- name: Set JDK 11 as default
run: echo "/usr/lib/jvm/java-11-openjdk-amd64/bin" >> $GITHUB_PATH
run: echo "/usr/lib/jvm/java-14-openjdk-amd64/bin" >> $GITHUB_PATH

- name: Checkout cleanup script
uses: actions/checkout@v2
Expand Down Expand Up @@ -143,7 +143,7 @@ jobs:

community_build:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08

steps:
- name: Checkout cleanup script
Expand Down Expand Up @@ -184,7 +184,7 @@ jobs:

test_sbt:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
if: (
github.event_name == 'push' &&
startsWith(github.event.ref, 'refs/tags/')
Expand Down Expand Up @@ -227,7 +227,7 @@ jobs:

test_java8:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
if: "(
github.event_name == 'push' &&
startsWith(github.event.ref, 'refs/tags/') &&
Expand Down Expand Up @@ -274,7 +274,7 @@ jobs:

publish_nightly:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
needs: [test, test_bootstrapped, community_build, test_sbt, test_java8]
if: github.event_name == 'schedule'
env:
Expand Down Expand Up @@ -321,7 +321,7 @@ jobs:

nightly_documentation:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
needs: [publish_nightly]
if: github.event_name == 'schedule'
env:
Expand Down Expand Up @@ -375,7 +375,7 @@ jobs:

publish_release:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
needs: [test, test_bootstrapped, community_build, test_sbt, test_java8]
if: github.event_name == 'push' &&
startsWith(github.event.ref, 'refs/tags/') &&
Expand Down Expand Up @@ -470,7 +470,7 @@ jobs:

release_documentation:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
needs: [publish_release]
if: github.event_name == 'push' &&
startsWith(github.event.ref, 'refs/tags/') &&
Expand Down Expand Up @@ -527,7 +527,7 @@ jobs:

publish_sbt_release:
runs-on: [self-hosted, Linux]
container: lampepfl/dotty:2020-04-24
container: lampepfl/dotty:2020-09-08
needs: [community_build, test_sbt]
if: github.event_name == 'push' &&
startsWith(github.event.ref, 'refs/tags/sbt-dotty-')
Expand Down
47 changes: 30 additions & 17 deletions compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,16 +518,24 @@ class ClassfileParser(
}
// sigToType

def parseAnnotArg(skip: Boolean = false)(using ctx: Context, in: DataReader): Option[untpd.Tree] = {
class EnumTag(sig: String, name: NameOrString) {
def toTree(using ctx: Context): untpd.Tree = {
val enumClassTp = sigToType(sig)
val enumModuleClass = enumClassTp.classSymbol.companionModule
val tmref = TermRef(enumModuleClass.termRef, name.name)
untpd.TypedSplice(ref(tmref))
}
}

def parseAnnotArg(skip: Boolean = false)(using ctx: Context, in: DataReader): Option[untpd.Tree | EnumTag] = {

// If we encounter an empty array literal, we need the type of the corresponding
// parameter to properly type it, but that would require forcing the annotation
// early. To avoid this we store annotation arguments as untyped trees
import untpd._

// ... but constants need to actually be typed with a ConstantType, so we
// can't rely on type inference, and type them early.
def lit(c: Constant): Tree = TypedSplice(ast.tpd.Literal(c))
def lit(c: Constant): untpd.Tree = untpd.TypedSplice(tpd.Literal(c))

val tag = in.nextByte.toChar
val index = in.nextChar
Expand All @@ -545,34 +553,36 @@ class ClassfileParser(
case CLASS_TAG =>
if (skip) None else Some(lit(Constant(pool.getType(index))))
case ENUM_TAG =>
val enumClassTp = pool.getType(index)
val sig = pool.getExternalName(index).value
val enumCaseName = pool.getName(in.nextChar)
if (skip)
None
else {
val enumModuleClass = enumClassTp.classSymbol.companionModule
Some(Select(ref(enumModuleClass), enumCaseName.name))
}
if (skip) None else Some(EnumTag(sig, enumCaseName))
case ARRAY_TAG =>
val arr = new ArrayBuffer[Tree]()
val arr = new ArrayBuffer[untpd.Tree]()
var hasError = false
for (i <- 0 until index)
parseAnnotArg(skip) match {
case Some(c) => arr += c
case Some(c: untpd.Tree) => arr += c
case Some(tag: EnumTag) => arr += tag.toTree
Copy link
Member

Choose a reason for hiding this comment

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

The toTree here means we're still forcing things early in some cases

Copy link
Member

Choose a reason for hiding this comment

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

I guess if this works OK with JDK 14 we can merge as is but we should revisit it.

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, this is a recursive call, I see no other ways to deal with it elegantly.

Copy link
Member

Choose a reason for hiding this comment

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

We us this call to build up elems which is used in Some(untpd.JavaSeqLiteral(elems, TypeTree())), if we want to delay things properly we'd have to propagate the delay to this construction too: replace EnumTag by Context => untpd.Tree again, and construct a new Context => untpd.Tree when any element in the Array is itself a Context => untpd.Tree

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, if we go back to Context => untpd.Tree, things will work, but it will undo the micro-optimizations. I'd say being lazy enough is a good tradeoff. If a problem emerges, we can be lazier (the lazy problem might be in other places as well).

case None => hasError = true
}
if (hasError) None
else if (skip) None
else {
val elems = arr.toList
Some(untpd.JavaSeqLiteral(elems, TypeTree()))
Some(untpd.JavaSeqLiteral(elems, untpd.TypeTree()))
}
case ANNOTATION_TAG =>
parseAnnotation(index, skip).map(_.untpdTree)
}
}

class ClassfileAnnotation(annotType: Type, args: List[untpd.Tree]) extends LazyAnnotation {
class ClassfileAnnotation(annotType: Type, lazyArgs: List[(NameOrString, untpd.Tree | EnumTag)]) extends LazyAnnotation {
private def args(using Context): List[untpd.Tree] =
lazyArgs.map {
case (name, tree: untpd.Tree) => untpd.NamedArg(name.name, tree).withSpan(NoSpan)
case (name, tag: EnumTag) => untpd.NamedArg(name.name, tag.toTree).withSpan(NoSpan)
}

protected var mySym: Symbol | (Context ?=> Symbol) =
(using ctx: Context) => annotType.classSymbol

Expand All @@ -598,13 +608,16 @@ class ClassfileParser(
case _ =>

val nargs = in.nextChar
val argbuf = new ListBuffer[untpd.Tree]
val argbuf = new ListBuffer[(NameOrString, untpd.Tree | EnumTag)]
var hasError = false
for (i <- 0 until nargs) {
val name = pool.getName(in.nextChar)
parseAnnotArg(skip) match {
case Some(arg) => argbuf += untpd.NamedArg(name.name, arg)
case None => hasError = !skip
case Some(arg) =>
argbuf += name -> arg

case None =>
hasError = !skip
}
}
if (hasError || skip) None
Expand Down