-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove anomalies and gaps in handling annotations #13348
Conversation
test performance please |
performance test scheduled: 1 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13348/ to see the changes. Benchmarks is based on merging with master (68044a6) |
test performance please |
1 similar comment
test performance please |
test performance please |
performance test scheduled: 1 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13348/ to see the changes. Benchmarks is based on merging with master (6a3cd1f) |
test performance please |
performance test scheduled: 1 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13348/ to see the changes. Benchmarks is based on merging with master (f7344ab) |
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13348/ to see the changes. Benchmarks is based on merging with master (a82a1a6) |
Always print annotation arguments if there are some, except for Body annotations
A TypeMap previously mapped the annotation only if it returned a different result for the parent type. There is no good reason for this behavior. We now map always, but allow the mapping operation to be defined in the annotation. The change uncovers a bug illustrated in annotDepMethType.scala, which is fixed in the next commit.
Fix problems handling function types that are dependent through their result annotations.
test performance please |
performance test scheduled: 1 job(s) in queue, 0 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13348/ to see the changes. Benchmarks is based on merging with master (a82a1a6) |
@@ -607,7 +607,7 @@ class RefinedPrinter(_ctx: Context) extends PlainPrinter(_ctx) { | |||
case tree: Template => | |||
toTextTemplate(tree) | |||
case Annotated(arg, annot) => | |||
toTextLocal(arg) ~~ annotText(annot) | |||
toTextLocal(arg) ~~ toText(annot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we are now printing annot
as a tree, and not a tree interpreted as an annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printing output is broken before typer now
class Foo() extends Annotation | ||
class Bar(s: String) extends Annotation | ||
|
||
def x: Int @nowarn @main @Foo @Bar("hello") = "abc" // error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with these changes -Xprint:parser
will only show <none>
where the class name should be
def x: Int @<none> @<none> @<none> @<none>("hello") = "abc"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 3.1.0-RC1 we still show the correct class names with -Xprint:parser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Is there an easy way to write a printing test to guard against regressions for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PrintingTest
can be modified to add cases that print at parser instead:
compiler/test/dotty/tools/dotc/printing/PrintingTest.scala
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@odersky I have added tests to PrintingTest
in latest commit
Three main fixes, with one commit for each: