SI-8941 Idempotent presentation compilation of implicit classes #4079
Conversation
This file contains 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
retronym
force-pushed
the
ticket/8941
branch
3 times, most recently
from
3417f3a
to
2564752
Compare
| val result = enclClass.info decl name suchThat (x => x.isMethod && x.isSynthetic) | ||
| assert(result != NoSymbol, "not found: "+name+" in "+enclClass+" "+enclClass.info.decls) | ||
| val result = enclClass.info decl name filter (x => x.isMethod && x.isSynthetic) | ||
| if (result == NoSymbol && result.isOverloaded) |
There was a problem hiding this comment.
should it be || here instead of &&?
|
very nice work, great commit message! |
When we name an implicit class, `enterImplicitWrapper` is called, which enters the symbol for the factory method into the owning scope. The tree defining this factory method is stowed into `unit.synthetics`, from whence it will be retrieved and incorporated into the enclosing tree during typechecking (`addDerivedTrees`). The entry in `unit.synthetics` is removed at that point. However, in the presentation compiler, we can typecheck a unit more than once in a single run. For example, if, as happens in the enclosed test, a call to ask for a type at a given position interrupts type checking of the entire unit, we can get into a situation whereby the first type checking invocation has consumed the entry from `unit.synthetics`, and the second will crash when it can't find an entry. Similar problems have been solved in the past in `enterExistingSym` in the presentation compiler. This method is called when the namer encounters a tree that already has a symbol attached. See 0b78a01 / 148736c. This commit takes a two pronged approach. First, `enterExistingSym` is extended to handle implicit classes. Any previous factory method in the owning scope is removed, and `enterImplicitWrapper` is called to place a new tree for the factory into `unit.synthetics` and to enter its symbol into the owning scope. Second, the assertions that could be tripped in `addDerivedTrees` and in `ImplicitClassWrapper#derivedSym` have been converted to positioned errors. The first change is sufficient to fix this bug, but the second is also enough to make the enclosed test pass, and has been retained as an extra layer of defence.
|
I've just pushed another commit that tests this, and other forms of idempotency, in a single threaded manner. Incidentally, this reminds me of SI-5265. Idempotency can even be needed in the batch compiler, to make typechecking re-entract: https://github.com/retronym/scala/compare/ticket/5265 |
A retrospective test case which covers typechecking idempotency which was introduced in 0b78a01 / 148736c. It also tests the implicit class handling, which was fixed in the previous commit. It is difficult to test this using existing presentation compiler testing infrastructure, as one can't control at which point during the first typechecking the subesquent work item will be noticed. Instead, I've created a test with a custom subclass of `interactive.Global` that allows precise, deterministic control of when this happens. It overrides `signalDone`, which is called after each tree is typechecked, and watches for a defintion with a well known name. At that point, it triggers a targetted typecheck of the tree marked with a special comment. It is likely that this approach can be generalized to a reusable base class down the track. In particular, I expect that some of the nasty interactive ScalaDoc bugs could use this single-threaded approach to testing the presentation compiler.
|
LGTM! |
retronym
added a commit
that referenced
this pull request
Nov 2, 2014
SI-8941 Idempotent presentation compilation of implicit classes
retronym
added a commit
to retronym/scala
that referenced
this pull request
Jul 8, 2017
Fixes regression in that the reporter bisected to scala#4079. I haven't reverted that change, but rather noticed that the path through namers that enters trees that already have symbols assigned was not extending the context chain for imports.
retronym
added a commit
to retronym/scala
that referenced
this pull request
Jul 8, 2017
Fixes regression in that the reporter bisected to scala#4079. I haven't reverted that change, but rather noticed that the path through namers that enters trees that already have symbols assigned was not extending the context chain for imports.
retronym
added a commit
to retronym/scala
that referenced
this pull request
Jul 8, 2017
Fixes regression in that the reporter bisected to scala#4079. I haven't reverted that change, but rather noticed that the path through namers that enters trees that already have symbols assigned was not extending the context chain for imports.
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.
When we name an implicit class,
enterImplicitWrapperis called,which enters the symbol for the factory method into the
owning scope. The tree defining this factory method is stowed into
unit.synthetics, from whence it will be retrieved and incorporatedinto the enclosing tree during typechecking (
addDerivedTrees).The entry in
unit.syntheticsis removed at that point.However, in the presentation compiler, we can typecheck a unit
more than once in a single run. For example, if, as happens
in the enclosed test, a call to ask for a type at a given
position interrupts type checking of the entire unit, we
can get into a situation whereby the first type checking
invocation has consumed the entry from
unit.synthetics,and the second will crash when it can't find an entry.
Similar problems have been solved in the past in
enterExistingSymin the presentation compiler. This methodis called when the namer encounters a tree that already has
a symbol attached. See 0b78a01 / 148736c.
This commit takes a two pronged approach.
First,
enterExistingSymis extended to handle implicit classes.Any previous factory method in the owning scope is removed, and
enterImplicitWrapperis called to place a new tree for the factoryinto
unit.syntheticsand to enter its symbol into the owning scope.Second, the assertions that could be tripped in
addDerivedTreesand in
ImplicitClassWrapper#derivedSymhave been converted topositioned errors.
The first change is sufficient to fix this bug, but the second
is also enough to make the enclosed test pass, and has been retained
as an extra layer of defence.
Review by @lrytz / @dragos