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

Keep package structure in `rootTreeOrProvider` #5276

Merged
merged 6 commits into from Nov 29, 2018

Conversation

Projects
None yet
4 participants
@Duhemm
Copy link
Contributor

Duhemm commented Oct 17, 2018

rootTreeOrProvider is a property of ClassSymbol, and contain the
tree that defines this symbol (or a mean to get that tree).

When the tree came from unpickled TASTY files, we were able to see the
nested packages from which the tree comes, along with the imports.
However, when the symbol has been loaded from source, we were filling
the rootTreeOrProvider only with the TypeDef that introduced this
symbol.

This commit moves the assignment of rootTreeOrProvider out of the
typer and into its own phase. Before assigning, we recreate the package
structure so that the information that we get out of
rootTreeOrProvider is the same regardless of whether the symbol has
been loaded from source or unpickled from TASTY.

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Oct 18, 2018

test performance please

@dotty-bot

This comment has been minimized.

Copy link

dotty-bot commented Oct 18, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

@dotty-bot

This comment has been minimized.

Copy link

dotty-bot commented Oct 18, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5276/ to see the changes.

Benchmarks is based on merging with master (a821035)

@nicolasstucki nicolasstucki self-requested a review Oct 18, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Oct 19, 2018

test performance please

@dotty-bot

This comment has been minimized.

Copy link

dotty-bot commented Oct 19, 2018

performance test scheduled: 1 job(s) in queue, 1 running.

@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Oct 19, 2018

Can you clarify what problem this PR solves? What is rootTreeOrPeovider used for?

Is there another way to solve this rather than introducing a complete new tree traversal?

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Oct 19, 2018

That would not be a complete tree traversal, it would be quite shallow. Traverse from the root package until you find the first TypeDef. The implementation is a bit awkward and makes it look like a full traversal.

@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Oct 19, 2018

It looks like a complete tree traversal to me:

    override def traverse(tree: tpd.Tree)(implicit ctx: Context): Unit = {
      if (tree.isInstanceOf[tpd.TypeDef] && tree.symbol.isClass) {
        ...
      }
      traverseChildren(tree)
    }

traverseChildren is called unconditionally. Am I missing something?

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Oct 19, 2018

rootTreeOrProvider is used to get the tree that introduced a given ClassSymbol. This tree can come either from source (for instance, if we have just compiled the file that defined it, or the file that defines it needs recompilation), or it can come from TASTy (we're unpickling).

Given a class symbol, the value of rootTreeOrProvider can come from 2 different paths:

  • When the symbol comes from the source, rootTreeOrProvider used to be populated by the Typer, where it was set to the typed TypeDef that introduced the symbol. We have no information about the packages the TypeDef was nested in, nor about the imports.
  • When the symbol comes from TASTy, rootTreeOrProvider is set to the unpickler by the symbol loader. When the tree is requested, the unpickler will unpickle the trees, which will include the nested package defs and imports.

In the IDE, we always want to see the top level imports, because this information is necessary to correctly do renamings.
With the IDE, we have 3 scenarios to consider:

  1. The file is open in the IDE: We will have the complete tree without needing to look at rootTreeOrProvider, so we'll see the complete structure.
  2. The file is not open in the IDE, and it doesn't need recompilation: We will unpickle the tree and see the complete structure.
  3. The file is not open in the IDE, and it needs recompilation: We will use rootTreeOrProvider, and we won't be able to see top level imports.

This PR is only important in the 3rd scenario. When setting the rootTreeOrProvider, we take the typed compilation unit, keep the nested packages and imports and the class definition. This way, we can store in rootTreeOrProvider an information similar to what we'd if we were unpickling the tree. Doing this is moved out of the typer so that we have access to the typed compilation unit.

Now that we have IDE tests where the files are not "open in the IDE", I'll add tests where that matters.

@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Oct 19, 2018

Also I don't think the benchmark set YretainTrees, so you would not be able to measure the impact of this change

@dotty-bot

This comment has been minimized.

Copy link

dotty-bot commented Oct 19, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/5276/ to see the changes.

Benchmarks is based on merging with master (193e064)

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Oct 19, 2018

Also I don't think the benchmark set YretainTrees, so you would not be able to measure the impact of this change

No, they do not

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 27, 2018

@Duhemm what is the state of this PR

Duhemm added some commits Oct 17, 2018

Keep package structure in `rootTreeOrProvider`
`rootTreeOrProvider` is a property of `ClassSymbol`, and contain the
tree that defines this symbol (or a mean to get that tree).

When the tree came from unpickled TASTY files, we were able to see the
nested packages from which the tree comes, along with the imports.
However, when the symbol has been loaded from source, we were filling
the `rootTreeOrProvider` only with the `TypeDef` that introduced this
symbol.

This commit moves the assignment of `rootTreeOrProvider` out of the
typer and into its own phase. Before assigning, we recreate the package
structure so that the information that we get out of
`rootTreeOrProvider` is the same regardless of whether the symbol has
been loaded from source or unpickled from TASTY.
Shallower traversal in `SetRootTree`
We only need to traverse the tree down to the top level classes, but no
further.

@Duhemm Duhemm force-pushed the dotty-staging:fix/sourceloader-roottreeprovider branch from 3726933 to 6272e8d Nov 28, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Nov 28, 2018

@nicolasstucki I've added a test that illustrates why this patch is needed. Without the addition of SetRootTree, the test fails as expected.

I've addressed your comments:

  • The traversal is now very shallow
  • I've moved from Phase to MacroTransform. I'm not convinced this is an improvement, since this phase doesn't change the tree.

@Duhemm Duhemm assigned nicolasstucki and unassigned Duhemm Nov 28, 2018

@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Nov 28, 2018

I think 6272e8d should be reverted. If it does not transform the tree then it doesn't have to be a macro transform

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 28, 2018

@Duhemm something failed in the tests

@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Nov 28, 2018

I've moved from Phase to MacroTransform. I'm not convinced this is an improvement, since this phase doesn't change the tree.

Yes you are right, it is better as a phase.

@nicolasstucki
Copy link
Contributor

nicolasstucki left a comment

Otherwise looks good

Set incomplete `rootTreeOrProvider` in typer
During typer, set `rootTreeOrProvider` to be the typed class def. This
information will be overwritten in `SetRootTree` to include the whole
package structure, but we may never reach this phase.

@Duhemm Duhemm force-pushed the dotty-staging:fix/sourceloader-roottreeprovider branch from 6272e8d to 136b0ce Nov 28, 2018

@Duhemm

This comment has been minimized.

Copy link
Contributor

Duhemm commented Nov 28, 2018

@nicolasstucki I've fixed the test in 136b0ce, do you mind taking another look?

@Duhemm Duhemm assigned nicolasstucki and unassigned Duhemm Nov 28, 2018

@nicolasstucki nicolasstucki merged commit 75cbbd6 into lampepfl:master Nov 29, 2018

3 checks passed

CLA User signed CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details

@nicolasstucki nicolasstucki deleted the dotty-staging:fix/sourceloader-roottreeprovider branch Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment