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

Remove anomalies and gaps in handling annotations #13348

Merged
merged 7 commits into from Sep 22, 2021

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 20, 2021

Three main fixes, with one commit for each:

  • Fix printing of annotations. Previously arguments were sometimes printed sometimes omitted.
  • Fix TypeMaps over annotated types
  • Fix dependent functions where the dependency goes through an annotation

@odersky
Copy link
Contributor Author

odersky commented Aug 20, 2021

test performance please

@dottybot
Copy link
Member

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

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/13348/ to see the changes.

Benchmarks is based on merging with master (68044a6)

@odersky
Copy link
Contributor Author

odersky commented Sep 2, 2021

test performance please

1 similar comment
@odersky
Copy link
Contributor Author

odersky commented Sep 3, 2021

test performance please

@odersky odersky changed the title Soften anomaly when mapping annotations Remove anomaly when mapping annotations Sep 3, 2021
@odersky odersky changed the title Remove anomaly when mapping annotations Remove anomalies and gaps in handling annotations Sep 3, 2021
@anatoliykmetyuk
Copy link
Contributor

test performance please

@dottybot
Copy link
Member

dottybot commented Sep 3, 2021

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

@dottybot
Copy link
Member

dottybot commented Sep 3, 2021

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/13348/ to see the changes.

Benchmarks is based on merging with master (6a3cd1f)

@odersky
Copy link
Contributor Author

odersky commented Sep 3, 2021

test performance please

@dottybot
Copy link
Member

dottybot commented Sep 3, 2021

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

@dottybot
Copy link
Member

dottybot commented Sep 3, 2021

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/13348/ to see the changes.

Benchmarks is based on merging with master (f7344ab)

@odersky
Copy link
Contributor Author

odersky commented Sep 4, 2021

test performance please

@dottybot
Copy link
Member

dottybot commented Sep 4, 2021

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

@dottybot
Copy link
Member

dottybot commented Sep 4, 2021

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.
@odersky
Copy link
Contributor Author

odersky commented Sep 4, 2021

test performance please

@dottybot
Copy link
Member

dottybot commented Sep 4, 2021

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

@dottybot
Copy link
Member

dottybot commented Sep 4, 2021

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)
Copy link
Member

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

Copy link
Member

@bishabosha bishabosha left a 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
Copy link
Member

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"

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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

@bishabosha bishabosha assigned odersky and unassigned bishabosha Sep 21, 2021
@bishabosha bishabosha self-requested a review September 21, 2021 08:16
@odersky odersky merged commit 1a84caa into scala:master Sep 22, 2021
@odersky odersky deleted the dep-annots branch September 22, 2021 08:43
@Kordyjan Kordyjan added this to the 3.1.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants