Avoid accidental captures of Context #1974
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Feb 14, 2017
`CheckCaptures` is an experimental phase that will check the captured environment of every Closure to make sure that it doesn't contain any type marked `@checkCaptures`, unless the expected type of the closure is marked `@allowCaptures`. This is used to check that we do not accidentally capture `Context` which can lead to various issues, especially in interactive uses of the compiler since using a `Context` from a previous run is very likely to throw a StaleSymbolError. This phase will not be merged in dotty since it requires a lot of manual annotations to be useful (more than 150), but it has already lead to finding several bugs: - scala#1974 See also https://github.com/TiarkRompf/scala-escape for a much more advanced and safer alternative that could be an inspiration for future work.
smarter
force-pushed
the
fix/ctx-capture
branch
2 times, most recently
from
f59a8f2 to
28dc7e1
Compare
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Feb 14, 2017
`CheckCaptures` is an experimental phase that will check the captured environment of every Closure to make sure that it doesn't contain any type marked `@checkCaptures`, unless the expected type of the closure is marked `@allowCaptures`. This is used to check that we do not accidentally capture `Context` which can lead to various issues, especially in interactive uses of the compiler since using a `Context` from a previous run is very likely to throw a StaleSymbolError. This phase will not be merged in dotty since it requires a lot of manual annotations to be useful (more than 150), but it has already lead to finding several bugs: - scala#1974 See also https://github.com/TiarkRompf/scala-escape for a much more advanced and safer alternative that could be an inspiration for future work.
odersky
reviewed
Feb 15, 2017
| @@ -27,14 +27,14 @@ object ImportInfo { | |||
| * @param isRootImport true if this is one of the implicit imports of scala, java.lang, | |||
| * scala.Predef or dotty.DottyPredef in the start context, false otherwise. | |||
| */ | |||
| class ImportInfo(symf: => Symbol, val selectors: List[untpd.Tree], | |||
| class ImportInfo(symf: Context => Symbol, val selectors: List[untpd.Tree], | |||
| symNameOpt: Option[TermName], val isRootImport: Boolean = false)(implicit ctx: Context) { | |||
There was a problem hiding this comment.
can we delete this ctx parameter?
| private[this] var mySym: Symbol = _ | ||
|
|
||
| override def symbol(implicit ctx: Context): Symbol = { | ||
| if (mySym == null) { |
There was a problem hiding this comment.
This one looks fishy. Should we not recompute the symbol here maewhen its validity period does not fall in the current run? ImportInfo has a similar problem, but there I think one can argue that it does not matter, as ImportInfos are either global (live forever) or are local to one compilation unit. But for annotations I don't think we can make that argument.
The capture context was only used to get its phase so shouldn't cause any problem.
The captured context was passed implicitly to dd.rhs, atGroupEnd is always run with the same runId as the captured context so this should be okay, but it's better to avoid using two contexts in the same expression anyway.
Previously, the following code accidentally used the implicit Context
parameter of `typedApply`:
tryEither {
implicit ctx => typedOpAssign
Previously, `specificProto` was a def even though it is always called, this is because in cece884, the usages of `specificProto` and `genericProto` were swapped. We fix this by only defining the protos where they are used. Incidentally, this mean that the calls to UnapplyFunProto will use the correct Context inside `tryEither`, although in this case this shouldn't matter.
`lazyStats` creates a lazy tree using `readLater`, but the closure inside `readLater` calls `mergeTypeParamsAndAliases` which until this commit implicitly used the Context from the outer method `readTemplate`, this lead to crashes in the Language Server.
getMember needs to take an implicit `Context` parameter, otherwise the
following code:
val result = ctx.atPhaseNotLaterThan(ctx.typerPhase) { implicit ctx =>
getMember(owner, innerName.toTypeName)
}
will not run getMember at the typer phase.
ImportInfo#toString required the ctx parameter,so it was replaced by ImportInfo#toText.
smarter
force-pushed
the
fix/ctx-capture
branch
from
28dc7e1 to
2245ffd
Compare
|
@odersky: The last two commits should address your comments. |
Previously we computed the scope in `findModuleBuddy` using `this.effectiveScope`, this means that we captured `this` which has a self-type of `Context`, replacing it by `ctx.effectiveScope` would be wrong since we are interested in the scope at the time `adjustModuleCompleter` was called, not the scope at the time the completer is called. Therefore, we have to eagerly compute `this.effectiveScope` so that we don't capture the Context but can use the right scope in `findModuleBuddy`. We also move `findModuleBuddy` to a companion object to avoid accidental captures of `this`. This capture lead to crashes in the IDE.
This capture did not cause any problem since we always called TempClassInfo#finalize with the same Context than we captured in `addSuspension`, but it's better to be explicit about these things.
|
I added two more commits to address the remaining context captures issues |
|
LGTM 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These issues where detected using the made-for-the-occasion
CheckCapturesphase available at https://github.com/dotty-staging/dotty/commits/checkClosures (see last two commits for more information). Ideally,CheckCapturesor something like it would be part of the compiler to prevent future accidental captures, but the current implementation requires adding more than 150 annotations to our codebase which is not very practical, see also https://github.com/TiarkRompf/scala-escape for a much more advanced and safer alternative that could be an inspiration for future work.Most of the fixed issues are hypothetical but 1ccee1f caused a
StaleSymbolErrorin the Language Server and 08cd627 apparently fixing an error message.