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

Actually retract clashing synthetic apply/unapply #5846

Merged
merged 1 commit into from Apr 13, 2017

Conversation

@adriaanm
Copy link
Member

commented Apr 12, 2017

Also make this whole retraction of apply/unapply in case of a
clashing user-defined member conditional on -Xsource:2.12.

It turns out, as explained by lrytz, that the retraction mechanism
was fragile because it relied on the order in which completers are run.
We now cover both the case that:

  • the completer was run, the IS_ERROR flag was set, and the
    symbol was unlinked from its scope before addSynthetics
    in typedStat iterates over the scope (since the symbol is
    already unlinked, the tree is not added, irrespective of its flags).
    For this case, we also remove the symbol from the synthetics in
    its unit (for cleanliness).

  • the completer is triggered during the iteration in addSynthetics,
    which needs the check for the IS_ERROR flag during the iteration.

Before, the completer just unlinked the symbol and set the IS_ERROR flag,
and I assumed the typer dropped a synthetic tree with a symbol with
that flag, because the tree was not shown in -Xprint output.
In reality, the completer just always happened to run before the
addSynthetics loop and unlinked the symbol from its scope in the
test cases I came up with (including the 2.11 community build).

Thankfully, the 2.12 community build caught my mistake, and lrytz provided
a good analysis and review.

Fix scala/bug#10261

@adriaanm adriaanm requested review from lrytz, densh and sjrd Apr 12, 2017

@adriaanm adriaanm added this to the 2.11.11 milestone Apr 12, 2017

@adriaanm adriaanm referenced this pull request Apr 12, 2017
@sjrd

sjrd approved these changes Apr 12, 2017

Copy link
Member

left a comment

Also make this whole retraction of apply/unapply in case of a
clashing user-defined member conditional on -Xsource:2.12.

👍

@adriaanm adriaanm force-pushed the adriaanm:t10261-2.11 branch from 2f86599 to 2b9167d Apr 12, 2017

@adriaanm

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

(Forgot the flags files of course :-))

@adriaanm adriaanm force-pushed the adriaanm:t10261-2.11 branch from 2b9167d to 5647115 Apr 12, 2017

@lrytz
Copy link
Member

left a comment

  • also add [backport] to the commit msg
// (1) If we're already in the loop, set the IS_ERROR flag and trigger the condition
// `sym.initialize.hasFlag(IS_ERROR)` in typedStats::addSynthetics,
// (2) Or, if we are not yet in the addSynthetics loop (and we're not going to emit an error anyway),
// we unlink the symbol from its scope.
sym setFlag IS_ERROR

This comment has been minimized.

Copy link
@lrytz

lrytz Apr 12, 2017

Member
  • add companionContext.unit.synthetics -= sym?
Actually retract clashing synthetic apply/unapply [backport]
Also make this whole retraction of apply/unapply in case of a
clashing user-defined member conditional on `-Xsource:2.12`.

It turns out, as explained by lrytz, that the retraction mechanism
was fragile because it relied on the order in which completers are run.
We now cover both the case that:

  - the completer was run, the `IS_ERROR` flag was set, and the
  symbol was unlinked from its scope before `addSynthetics`
  in `typedStat` iterates over the scope (since the symbol is
  already unlinked, the tree is not added, irrespective of its flags).
  For this case, we also remove the symbol from the synthetics in
  its unit (for cleanliness).

  - the completer is triggered during the iteration in `addSynthetics`,
  which needs the check for the `IS_ERROR` flag during the iteration.

Before, the completer just unlinked the symbol and set the IS_ERROR flag,
and I assumed the typer dropped a synthetic tree with a symbol with
that flag, because the tree was not shown in -Xprint output.
In reality, the completer just always happened to run before the
addSynthetics loop and unlinked the symbol from its scope in the
test cases I came up with (including the 2.11 community build).

Thankfully, the 2.12 community build caught my mistake, and lrytz provided
a good analysis and review.

Fix scala/bug#10261

@adriaanm adriaanm force-pushed the adriaanm:t10261-2.11 branch from 5647115 to 77917e9 Apr 12, 2017

@densh

densh approved these changes Apr 13, 2017

Copy link
Contributor

left a comment

I think it's much safer now under a flag. 👍

@lrytz

lrytz approved these changes Apr 13, 2017

@adriaanm adriaanm merged commit 8a413ba into scala:2.11.x Apr 13, 2017

5 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
integrate-ide [2826] SUCCESS. Took 1 s.
Details
validate-main [3138] SUCCESS. Took 89 min.
Details
validate-publish-core [3241] SUCCESS. Took 5 min.
Details
validate-test [2294] SUCCESS. Took 83 min.
Details
@adriaanm adriaanm referenced this pull request Apr 13, 2017
@naferx naferx referenced this pull request Dec 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.