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

Fix #4678: Create single Typed tree with multiple type annotations #5714

Merged
merged 2 commits into from Jan 16, 2019

Conversation

Projects
None yet
2 participants
@nicolasstucki
Copy link
Contributor

commented Jan 15, 2019

This is mostly useful for printers as it represents what is written in the source.
I may also slightly improve performance of later phases.

With this change the tree becomes

result of tests/pos/i4678.scala after frontend:
package <empty> {
  class Foo() extends Object() { 
    val x: Int = 1:Int(1) @annot1 @annot2 @annot3 @annot4 @annot5
  }
  ...
}

instead of

result of tests/pos/i4678.scala after frontend:
package <empty> {
  class Foo() extends Object() { 
    val x: Int = 
      1:Int(1) @annot1:Int(1) @annot1 @annot2:Int(1) @annot1 @annot2 @annot3:
        Int(1) @annot1 @annot2 @annot3 @annot4
      :Int(1) @annot1 @annot2 @annot3 @annot4 @annot5
  }
  ...
}
@smarter
Copy link
Member

left a comment

Can we add a decompilation test to ensure the output is as expected ?

Show resolved Hide resolved tests/pos/i4678.scala
Fix #4678: Create single Typed tree with multiple type annotations
This is mostly useful for printers as it represents what is written in the source.
I may also slightly improve performance of later phases.

@nicolasstucki nicolasstucki force-pushed the dotty-staging:fix-#4678 branch from afb2805 to fab9114 Jan 15, 2019

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Added a decompilation test

@smarter

This comment has been minimized.

OK maybe a hover test isn't specific enough, what's important is that go-to-definition on annotX goes to the definition of annotX for all X.

@nicolasstucki

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

I tried the definitions tests and it fails on both versions (before and after these changes). The trees in the annotated type do have the correct positions, nothing is lost with this change. I would fix that in a different PR as it is completely unrelated. Opened #5722 to track this issue.

@smarter smarter merged commit 4ecbb60 into lampepfl:master Jan 16, 2019

2 checks passed

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

@Blaisorblade Blaisorblade deleted the dotty-staging:fix-#4678 branch Jan 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.