Skip to content
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

Check of cyclic object initialization v4 #12698

Closed
wants to merge 33 commits into from

Conversation

liufengyun
Copy link
Contributor

Supersedes #12122

This goes back to coarse-grained abstraction for parameters,
which is trade-off between performance and expressiveness.
This helps reduce more than 20 errors:

[error] -- Error: /Users/fliu/Documents/dotty/compiler/src/dotty/tools/dotc/core/StdNames.scala:665:34
[error] 665 |      final val MINUS_STAR  : N = "-*"
[error]     |                                  ^^^^
[error]     |Call method dotty.tools.dotc.core.StdNames.ScalaNames.this.fromString("-*") on a value with an unknown initialization. Calling trace:
[error]     | -> package object printing {	[ package.scala:6 ]
[error]     |    -> val AndTypePrec: Int   = parsing.precedence(tpnme.raw.AMP)	[ package.scala:11 ]
[error]     |     -> object raw {	[ StdNames.scala:649 ]
For traits, its outers will be proxy methods of the class that extends
the trait. As the prefix is stable and is a valid value before any
super constructor calls. Therefore, we may think the outers for traits
are immediately set following the class parameters.

Also, when trying promotion of warm values, we never try warm values
whose fields are not fully filled -- which corresponds to promote
ThisRef with non-initialized fields, and errors will be reported when
the class is checked separately.
@liufengyun
Copy link
Contributor Author

liufengyun commented Jun 7, 2021

The statistics for 3 different designs when compiling Dotty with the checker:

|------------------------------------------------------+----------+----------|
|                                                      | time* (s)| warnings |
|------------------------------------------------------+----------+----------|
| Use type abstraction for outers and parameters       |       60 |       46 |
|------------------------------------------------------+----------+----------|
| Outer-sensitive, type abstraction for parameters     |       65 |       29 |
|------------------------------------------------------+----------+----------|
| Outer and parameter-sensitive (approx with ClassAbs) |    > 568 |        6 |
|------------------------------------------------------+----------+----------|

* total compilation time, with the checker for safe class instantiation disabled

All versions abstract mutable fields or mutable variables by their type.
The latest commit follows the 2nd design.

All of the designs above can catch the bugs found in the issue tracker:

Fix #9176
Fix #11262
Fix scala/bug#9312
Fix scala/bug#9428
Fix scala/bug#9115
Fix scala/bug#9261
Fix scala/bug#5366
Fix scala/bug#9360

The checker does not report a warning when calling a statically resolvable target but TASTy is missing (unlike the checker for class instantiation). It always issues a warning when calling a virtual method that cannot be resolved statically in the analysis (after approximation).

The detailed warnings for the 2nd design when compiling Dotty can be found in this gist.

@liufengyun liufengyun marked this pull request as ready for review June 7, 2021 14:45
This is safe because those symbols are already checked when they are
compiled. This avoids cycles in reading tasty.
/** A reference to a static object */
case class ObjectRef(klass: ClassSymbol) extends Addr

/** An ClassAbs of class */
Copy link
Contributor

Choose a reason for hiding this comment

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

From the comments here, the meanings of TypeAbs and ClassAbs are not clear, and it's not clear what are the differences between TypeAbs, ObjectRef, and ClassAbs. All that's clear is that they are all somehow related to an object.

Also, the abstraction ordering is not documented (although maybe these values are all unrelated by the ordering, except for RefSet in the obvious way).

Copy link
Contributor

Choose a reason for hiding this comment

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

One source of possible confusion is that the word "object" is overloaded to mean many different things:

  • a runtime instance of some class
  • a declaration in the program using the object keyword
  • probably other meanings as well
    It would be useful to precisely define distinct terms for these different meanings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed the documentation and naming are confusing. I try to clarify them below:

  • ClassAbs: abstract a value by its class (outer-sensitive, but not parameter-sensitive, similar to Warm)
  • TypeAbs: abstract a value by its type (primarily used for parameters)
  • ObjectRef: reference to a global object

When ClassAbs gets too complex, we widen to TypeAbs to ensure termination.

@liufengyun
Copy link
Contributor Author

Close for now, we will try new approaches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants