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

Safe initialization for Scala #7789

Merged
merged 112 commits into from Feb 13, 2020
Merged

Conversation

@liufengyun
Copy link
Contributor

liufengyun commented Dec 17, 2019

This PR implements experimental safe initialization check for Scala, which can be enabled by the compiler option -Ycheck-init.

A Quick Glance

Parent-Child Interaction

Given the following code snippet:

abstract class AbstractFile {
   def name: String
   val extension: String = Path.extension(name)
}

class RemoteFile(url: String) extends AbstractFile {
   val localFile: String = url.hashCode + ".tmp"       // error: usge of `localFile` before it's initialized
   def name: String = localFile
}

The checker will report:

-- Warning: tests/init/neg/AbstractFile.scala:7:4 ------------------------------
7 |	val localFile: String = url.hashCode + ".tmp"  // error
  |	    ^
  |    Access non-initialized field value localFile. Calling trace:
  |     -> val extension: String = name.substring(4)	[ AbstractFile.scala:3 ]
  |      -> def name: String = localFile	[ AbstractFile.scala:8 ]

Inner-Outer Interaction

Given the code below:

object Trees {
  class ValDef { counter += 1 }
  class EmptyValDef extends ValDef
  val theEmptyValDef = new EmptyValDef
  private var counter = 0  // error
}

The checker will report:

-- Warning: tests/init/neg/trees.scala:5:14 ------------------------------------
5 |  private var counter = 0  // error
  |              ^
  |             Access non-initialized field variable counter. Calling trace:
  |              -> val theEmptyValDef = new EmptyValDef	[ trees.scala:4 ]
  |               -> class EmptyValDef extends ValDef	[ trees.scala:3 ]
  |                -> class ValDef { counter += 1 }	[ trees.scala:2 ]

Functions

Given the code below:

abstract class Parent {
  val f: () => String = () => this.message
  def message: String
}
class Child extends Parent {
  val a = f()
  val b = "hello"           // error
  def message: String = b
}

The checker reports:

-- Warning: tests/init/neg/features-high-order.scala:7:6 -----------------------
7 |  val b = "hello"           // error
  |      ^
  |Access non-initialized field value b. Calling trace:
  | -> val a = f()	[ features-high-order.scala:6 ]
  |   -> val f: () => String = () => this.message	[ features-high-order.scala:2 ]
  |    -> def message: String = b	[ features-high-order.scala:8 ]

More details about the design can be found in the document or gist.

This PR is based on a series of joint work with Ondřej Lhoták (@olhotak), Aggelos Biboudis (@biboudis) and Paolo G. Giarrusso (@Blaisorblade).

@liufengyun liufengyun force-pushed the dotty-staging:init-dotty branch from 51e0898 to af9c2bc Dec 17, 2019
@hepin1989

This comment has been minimized.

Copy link

hepin1989 commented Dec 17, 2019

Cool stack trace, how about :

-- Warning: tests/init/neg/features-high-order.scala:7:6 -----------------------
  |  val b = "hello"           // error
  |      ^
  | Access non-initialized field value b. Calling trace:
  |\-> val a = f()	                                [ features-high-order.scala:6 ]
  | \--> val f: () => String = () => this.message	[ features-high-order.scala:2 ]
  |  \---> def message: String = b                 	[ features-high-order.scala:8 ]

would be better to provide a screenshot

@hepin1989

This comment has been minimized.

Copy link

hepin1989 commented Dec 17, 2019

It should be better to provide some helpful information for how to fix it like what unison does.

@liufengyun liufengyun force-pushed the dotty-staging:init-dotty branch from e343407 to 7b2a90b Dec 18, 2019
@LPTK

This comment has been minimized.

Copy link
Contributor

LPTK commented Dec 20, 2019

For modularity, we forbid subtle initialization interaction beyond project boundaries. For example, the following code passes the check when the two classes are defined in the same project:

class Base {
  private val map: mutable.Map[Int, String] = mutable.Map.empty
  def enter(k: Int, v: String) = map(k) = v
}
class Child extends Base {
  enter(1, "one")
  enter(2, "two")
}

This seems quite restrictive. There are libraries that use anonymous classes as a scope injection mechanism. I imagine something like the following be rejected, if the extended type is from another project:

foo(new {
  enter(1, "one")
  enter(2, "two")
})
@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Dec 20, 2019

foo(new {
  enter(1, "one")
  enter(2, "two")
})

The code above per se is not an issue, the checker can statically know that no fields in the child class may be reached --- as there is no overriding & local field access. It may relax the rule in such cases.

@liufengyun liufengyun force-pushed the dotty-staging:init-dotty branch 2 times, most recently from a6578f8 to 185ec56 Jan 13, 2020
Copy link
Contributor

anatoliykmetyuk left a comment

Apart from a few opportunities for DRY-ing the code, looks very good to me. The code is easy to follow and the architecture is clear, nice work!

@liufengyun liufengyun force-pushed the dotty-staging:init-dotty branch from 7403b66 to d90f14e Jan 27, 2020
@dwijnand

This comment has been minimized.

Copy link
Contributor

dwijnand commented Jan 27, 2020

Is there a tracking issue interested parties could subscribe to for this feature?

@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Jan 27, 2020

@dwijnand

This comment has been minimized.

Copy link
Contributor

dwijnand commented Jan 27, 2020

Thanks. I was hoping a GitHub issue, as issues have state (open/closed) and also one can subscribe to solely the closing of the issue. Discourse threads, much like Google Docs, have no such known state. (Just sharing thoughts, so you can consider them.)

@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Jan 27, 2020

@dwijnand Thanks for the detailed explanation: it's possible to subscribe to this PR, and get notified when it's merged (on the right-hand side of the page).

@liufengyun liufengyun force-pushed the dotty-staging:init-dotty branch from a09993d to 22d853d Jan 28, 2020
@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Jan 28, 2020

@anatoliykmetyuk : I have run the check on the whole test set (and community projects), all crashes are fixed. Could you have a look at the latest changes?

@liufengyun liufengyun force-pushed the dotty-staging:init-dotty branch from 56205ad to 33b3d25 Jan 28, 2020
@liufengyun liufengyun requested a review from anatoliykmetyuk Feb 10, 2020
liufengyun added 21 commits Jan 17, 2020
ErasedFunctionX does not have constructors, thus no need to follow.

dotc -d out -Yerased-terms -Ycheck-init tests/run-custom-args/erased/erased-15.scala
dotc -d out -Ycheck-init  tests/run/vc-equals.scala
dotc -d out -Ycheck-init tests/pos/aliasNew.scala
The method name is not an invariant:

dotc -d out -Ycheck-init tests/pos/i4350.scala
dotc -d out -Ycheck-init tests/pos/SI-4012-b.scala
dotc -d out -Ycheck-init tests/run/polymorphic-functions.scala
dotc -d out -Ycheck-init tests/pos/annot.scala
It should return a tuple of effects and potentials due to length limit.
Previously, the effects are checked but the errors are thrown away.
@liufengyun liufengyun force-pushed the dotty-staging:init-dotty branch from 97d4b22 to 1694e4a Feb 13, 2020
Copy link
Contributor

anatoliykmetyuk left a comment

LGTM, nice work @liufengyun 🎉

@anatoliykmetyuk anatoliykmetyuk merged commit 228b61e into lampepfl:master Feb 13, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@anatoliykmetyuk anatoliykmetyuk deleted the dotty-staging:init-dotty branch Feb 13, 2020
@liufengyun

This comment has been minimized.

Copy link
Contributor Author

liufengyun commented Feb 13, 2020

@anatoliykmetyuk Thanks a lot for your valuable feedback in the review.

@anatoliykmetyuk anatoliykmetyuk added this to the 0.23.0-RC1 milestone Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.