Skip to content

Fix #11654: create new symbol for stdlib patches #11803

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 4 commits into from
Mar 22, 2021
Merged

Fix #11654: create new symbol for stdlib patches #11803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
62c5b2d
Fix #11654: create new symbol for stdlib patches
liufengyun Mar 18, 2021
d567fe0
format comment
liufengyun Mar 18, 2021
3a7940a
Refactor code
liufengyun Mar 19, 2021
1edd3ca
Fix Todos
liufengyun Mar 19, 2021
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
54 changes: 50 additions & 4 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1111,10 +1111,49 @@ class Definitions {
* is read from a classfile.
*/
def patchStdLibClass(denot: ClassDenotation)(using Context): Unit =

def patch2(denot: ClassDenotation, patchCls: Symbol): Unit =
val scope = denot.info.decls.openForMutations

def recurse(patch: Symbol) = patch.is(Module) && scope.lookup(patch.name).exists

def makeClassSymbol(patch: Symbol, parents: List[Type], selfInfo: TypeOrSymbol) =
newClassSymbol(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use patch.copy(...) here? Would that not be shorter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that info and symbol depend on each other, and Symbol.copy does not support that.

owner = denot.symbol,
name = patch.name.asTypeName,
flags = patch.flags,
// need to rebuild a fresh ClassInfo
infoFn = cls => ClassInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be shorter to use derivedClassInfo here?

Copy link
Contributor Author

@liufengyun liufengyun Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That does not make much difference, as we change all fields here except parents. On the other hand, using the constructor makes the implicit assumptions explicit and clear here.

prefix = denot.symbol.thisType,
cls = cls,
declaredParents = parents, // assume parents in patch don't refer to symbols in the patch
decls = newScope,
selfInfo =
if patch.is(Module)
then TermRef(denot.symbol.thisType, patch.name.sourceModuleName)
else selfInfo // assume patch self type annotation does not refer to symbols in the patch
),
privateWithin = patch.privateWithin,
coord = denot.symbol.coord,
assocFile = denot.symbol.associatedFile
)

def makeNonClassSymbol(patch: Symbol) =
if patch.is(Inline) then
// Inline symbols contain trees in annotations, which is coupled
// with the underlying symbol.
// Changing owner for inline symbols is a simple workaround.
patch.denot = patch.denot.copySymDenotation(owner = denot.symbol)
patch
else
// change `info` which might contain reference to the patch
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a TreeTypeMap to map the info instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean for the inline tree in the annotation? It's quite complex -- we need to create a copy of all local symbols (e.g. for type, term symbols, symbols in the function body, etc.) and rewire them.

For non-inline symbols, we make strong assumptions on symbol.info, which seems to be simpler.

Copy link
Member

Choose a reason for hiding this comment

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

I meant for both, but I don't know if TreeTypeMap works on inline defs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a principled way to do this --- a map needs to make assumptions about from and to. We will not make fewer assumptions and the code will be complex.

patch.copy(
owner = denot.symbol,
info =
if patch.is(Module)
then TypeRef(denot.symbol.thisType, patch.name.moduleClassName)
else patch.info // assume non-object info does not refer to symbols in the patch
)

if patchCls.exists then
val patches = patchCls.info.decls.filter(patch =>
!patch.isConstructor && !patch.isOneOf(PrivateOrSynthetic))
Expand All @@ -1124,9 +1163,16 @@ class Definitions {
for patch <- patches do
patch.ensureCompleted()
if !recurse(patch) then
patch.denot = patch.denot.copySymDenotation(owner = denot.symbol)
scope.enter(patch)
else if patch.isClass then
val sym =
patch.info match
case ClassInfo(_, _, parents, _, selfInfo) =>
makeClassSymbol(patch, parents, selfInfo)
case _ =>
makeNonClassSymbol(patch)
end match
sym.annotations = patch.annotations
scope.enter(sym)
if patch.isClass then
patch2(scope.lookup(patch.name).asClass, patch)

def patchWith(patchCls: Symbol) =
Expand Down
5 changes: 3 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/TypeTestsCasts.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,9 @@ object TypeTestsCasts {
withMode(Mode.GadtConstraintInference) {
// Why not widen type arguments here? Given the following program
//
// trait Tree[-T] class Ident[-T] extends Tree[T] def foo1(tree:
// Tree[Int]) = tree.isInstanceOf[Ident[Int]]
// trait Tree[-T] class Ident[-T] extends Tree[T]
//
// def foo1(tree: Tree[Int]) = tree.isInstanceOf[Ident[Int]]
//
// In checking whether the test tree.isInstanceOf[Ident[Int]]
// is realizable, we want to constrain Ident[X] <: Tree[Int],
Expand Down
6 changes: 2 additions & 4 deletions compiler/test/dotty/tools/dotc/BootstrappedOnlyCompilationTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,13 @@ class BootstrappedOnlyCompilationTests {
// lower level of concurrency as to not kill their running VMs

@Test def picklingWithCompiler: Unit = {
val jvmBackendFilter = FileFilter.exclude(List("BTypes.scala", "Primitives.scala")) // TODO
val runtimeFilter = FileFilter.exclude(List("Tuple.scala", "stdLibPatches")) // TODO
implicit val testGroup: TestGroup = TestGroup("testPicklingWithCompiler")
aggregateTests(
compileDir("compiler/src/dotty/tools", picklingWithCompilerOptions, recursive = false),
compileDir("compiler/src/dotty/tools/dotc", picklingWithCompilerOptions, recursive = false),
compileDir("library/src/scala/runtime/function", picklingWithCompilerOptions),
compileFilesInDir("library/src/scala/runtime", picklingWithCompilerOptions, runtimeFilter),
compileFilesInDir("compiler/src/dotty/tools/backend/jvm", picklingWithCompilerOptions, jvmBackendFilter),
compileFilesInDir("library/src/scala/runtime", picklingWithCompilerOptions),
compileFilesInDir("compiler/src/dotty/tools/backend/jvm", picklingWithCompilerOptions),
compileDir("compiler/src/dotty/tools/dotc/ast", picklingWithCompilerOptions),
compileDir("compiler/src/dotty/tools/dotc/core", picklingWithCompilerOptions, recursive = false),
compileDir("compiler/src/dotty/tools/dotc/config", picklingWithCompilerOptions),
Expand Down
15 changes: 15 additions & 0 deletions library/src/scala/runtime/stdLibPatches/language.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package scala.runtime.stdLibPatches

import scala.annotation.compileTimeOnly

/** Scala 3 additions and replacements to the `scala.language` object.
*/
object language:
Expand Down Expand Up @@ -29,18 +31,21 @@ object language:
*
* @see [[https://dotty.epfl.ch/docs/reference/other-new-features/named-typeargs]]
*/
@compileTimeOnly("`namedTypeArguments` can only be used at compile time in import statements")
object namedTypeArguments

/** Experimental support for generic number literals.
*
* @see [[https://dotty.epfl.ch/docs/reference/changed-features/numeric-literals]]
*/
@compileTimeOnly("`genericNumberLiterals` can only be used at compile time in import statements")
object genericNumberLiterals

/** Experimental support for `erased` modifier
*
* @see [[https://dotty.epfl.ch/docs/reference/experimental/erased-defs]]
*/
@compileTimeOnly("`erasedDefinitions` can only be used at compile time in import statements")
object erasedDefinitions

end experimental
Expand All @@ -49,13 +54,15 @@ object language:
* Features in this object are slated for removal. New code should not use them and
* old code should migrate away from them.
*/
@compileTimeOnly("`deprecated` can only be used at compile time in import statements")
object deprecated:

/** Symbol literals have been deprecated since 2.13. Since Scala 3.0 they
* are no longer an official part of Scala. For compatibility with legacy software,
* symbol literals are still supported with a language import, but new software
* should not use them.
*/
@compileTimeOnly("`symbolLiterals` can only be used at compile time in import statements")
object symbolLiterals
end deprecated

Expand All @@ -69,6 +76,7 @@ object language:
* '''Why allow it?''' Not allowing auto-tupling is difficult to reconcile with
* operators accepting tuples.
*/
@compileTimeOnly("`noAutoTupling` can only be used at compile time in import statements")
object noAutoTupling

/** Where imported, loose equality using eqAny is disabled.
Expand All @@ -78,6 +86,7 @@ object language:
*
* @see [[https://dotty.epfl.ch/docs/reference/contextual/multiversal-equality]]
*/
@compileTimeOnly("`strictEquality` can only be used at compile time in import statements")
object strictEquality

/** Where imported, ad hoc extensions of non-open classes in other
Expand All @@ -96,29 +105,35 @@ object language:
* such extensions should be limited in scope and clearly documented.
* That's why the language import is required for them.
*/
@compileTimeOnly("`adhocExtensions` can only be used at compile time in import statements")
object adhocExtensions

/** Unsafe Nulls fot Explicit Nulls
* Inside the "unsafe" scope, `Null` is considered as a subtype of all reference types.
*
* @see [[http://dotty.epfl.ch/docs/reference/other-new-features/explicit-nulls.html]]
*/
@compileTimeOnly("`unsafeNulls` can only be used at compile time in import statements")
object unsafeNulls

@compileTimeOnly("`future` can only be used at compile time in import statements")
object future

@compileTimeOnly("`future-migration` can only be used at compile time in import statements")
object `future-migration`

/** Set source version to 3.0-migration.
*
* @see [[https://scalacenter.github.io/scala-3-migration-guide/docs/scala-3-migration-mode]]
*/
@compileTimeOnly("`3.0-migration` can only be used at compile time in import statements")
object `3.0-migration`

/** Set source version to 3.0.
*
* @see [[https://scalacenter.github.io/scala-3-migration-guide/docs/scala-3-migration-mode]]
*/
@compileTimeOnly("`3.0` can only be used at compile time in import statements")
object `3.0`

/* This can be added when we go to 3.1
Expand Down