Some fixes for AnnotatedTypes mapping #19957

mbovel · 2024-03-15T23:40:10Z

Fixes #5789, ~~fixes~~ #17939, fixes #18064 and ~~fixes~~ #19846.

Description of 3e1667c:

Annotation.mapWith maps an Annotation with a type map tm. As an optimization, this function first checks if tm would result in any change (by traversing the annotation’s argument trees with a TreeAccumulator) before applying tm to the whole annotation tree. This optimization had two problems: 1. it didn’t include type parameters, and 2. it used frozen_=:= to compare types, which didn’t work as expected with NoType. This commit fixes these issues.

Additionally, positions of trees that appear only inside AnnotatedType were not pickled. This commit also fixes this.

mbovel · 2024-03-16T00:55:02Z

✅ Fixed: this was due to an interaction between PrintingTest and BashExitCodeTests. The problem is that running PrintingTest generates Test.class in the Dotty root directory, and then running scalac -script -d <tmp_directory> test.scala from the Dotty root directory—where <tmp_directory>/test.scala contains @main def Test = ()—picks up the main method from <dotty>/Test.class instead of the one compiled from <tmp_directory>/test.scala. I fixed the PrintingTest side (76492df) so that printing tests don't pollute Dotty root directory. In the future, we should probably also fix BashExitCodeTests to not use classes from the Dotty root directory.

2 test failures in dotty.tools.scripting.BashExitCodeTests:

Error:  Test dotty.tools.scripting.BashExitCodeTests.runPos failed: java.lang.AssertionError: expected 0 but got 1
Error:  stderr:
Error:    No main methods detected for [/tmp/exit-code-tests16570023260888957621/BashExitCodeTests18250568141216098210.sc] expected:<0> but was:<1>, took 2.715 sec
Error:      at dotty.tools.scripting.BashExitCodeTests.verifyExit(BashExitCodeTests.scala:29)
Error:      at dotty.tools.scripting.BashExitCodeTests.scala$$anonfun$1(BashExitCodeTests.scala:32)
Error:      at dotty.tools.scripting.BashExitCodeTests.runPos(BashExitCodeTests.scala:46)
bashCmd: /usr/bin/bash -c "/__w/scala3/scala3/dist/target/pack/bin/scala /tmp/exit-code-tests12984782338892998733/BashExitCodeTests6292908361233855493.sc"
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
Error:      at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Error:      at java.lang.reflect.Method.invoke(Method.java:568)
Error:      ...
Error:  Test dotty.tools.scripting.BashExitCodeTests.scPos failed: java.lang.AssertionError: expected 0 but got 1
Error:  stderr:
Error:    dotty.tools.scripting.StringDriverException: No main methods detected for [/tmp/exit-code-tests17273406077719898552/BashExitCodeTests9163024565476305325.sc]
Error:    	at dotty.tools.scripting.StringDriverException$.apply(StringDriver.scala:52)
Error:    	at dotty.tools.scripting.Util$.detectMainClassAndMethod(Util.scala:54)
Error:    	at dotty.tools.scripting.ScriptingDriver.compileAndRun(ScriptingDriver.scala:28)
Error:    	at dotty.tools.scripting.Main$.process(Main.scala:45)
Error:    	at dotty.tools.scripting.Main$.main(Main.scala:49)
Error:    	at dotty.tools.MainGenericCompiler$.run$1(MainGenericCompiler.scala:177)
Error:    	at dotty.tools.MainGenericCompiler$.main(MainGenericCompiler.scala:186)
Error:    	at dotty.tools.MainGenericCompiler.main(MainGenericCompiler.scala) expected:<0> but was:<1>, took 2.776 sec
Error:      at dotty.tools.scripting.BashExitCodeTests.verifyExit(BashExitCodeTests.scala:29)
Error:      at dotty.tools.scripting.BashExitCodeTests.scalacRaw$$anonfun$1(BashExitCodeTests.scala:33)
Error:      at dotty.tools.scripting.BashExitCodeTests.scPos(BashExitCodeTests.scala:50)
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
Error:      at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
Error:      at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
bashCmd: /usr/bin/bash -c "/__w/scala3/scala3/dist/target/pack/bin/scalac -d /tmp/exit-code-tests15800954831614427502 -script /tmp/exit-code-tests15800954831614427502/BashExitCodeTests9673740865682828781.sc"
Error:      at java.lang.reflect.Method.invoke(Method.java:568)
Error:      ...

Relevant tests:

  @Test def runNeg = scala(f("@main def Test = prin", ".sc"))(1)
  @Test def runRun = scala(f("@main def Test = ???", ".sc"))(1)
  @Test def runPos = scala(f("@main def Test = ()", ".sc"))(0) // Failing

  @Test def scNeg = scalac("-script", f("@main def Test = prin", ".sc"))(1)
  @Test def scRun = scalac("-script", f("@main def Test = ???", ".sc"))(1)
  @Test def scPos = scalac("-script", f("@main def Test = ()", ".sc"))(0) // Failing

Run with:

scala3-compiler-bootstrapped / testOnly dotty.tools.scripting.BashExitCodeTests

tests/pos-custom-args/captures/i20272b.scala

mbovel · 2024-05-06T12:53:32Z

test performance please

dottybot · 2024-05-06T12:54:00Z

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

dottybot · 2024-05-06T15:25:49Z

Performance test finished successfully:

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

Benchmarks is based on merging with main (6d29951)

odersky · 2024-05-06T15:28:46Z

test performance please

odersky · 2024-05-06T15:29:05Z

(let's do it again, to see whether this is a fluke or a trend)

dottybot · 2024-05-06T15:30:21Z

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

dottybot · 2024-05-06T18:43:47Z

Performance test finished successfully:

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

Benchmarks is based on merging with main (6af2bcf)

mbovel · 2024-05-07T12:47:38Z

`Annotation.mapWith` micro benchmark

Setup

The benchmark compares applying a TypeMap on the types of the following values:

   val v1: Int @FlagAnnot = 42

   val v2: Int @StringAnnot("hello") = 42

   val v3: Int @LambdaAnnot(it => it == 42) = 42

   val v4: Int @LambdaAnnot(it => {
     def g(x: Int, y: Int) = x - y + 5
     g(it, 7) * 2 == 80
   }) = 42

It runs the type map with two functions:

id:which returns the same types,
mapInts: that maps every Int to type SpecialInt <: Int.

I ran it before (a6f4514) and after (a6689bc) the Annotation.mapWith changes.

Results

hash	jvm	benchmark	time median	time min	time max	time error minus	time error plus
`a6f4514`	openjdk	applyTypeMap(id,v1)	15,857,622	14,224,785	17,418,021	1,632,837	1,560,399
`a6689bc`	openjdk	applyTypeMap(id,v1)	16,294,074	14,841,272	17,925,590	1,452,802	1,631,516
`a6f4514`	openjdk	applyTypeMap(id,v2)	6,970,539	6,429,362	7,380,593	541,177	410,053
`a6689bc`	openjdk	applyTypeMap(id,v2)	7,553,913	7,221,825	7,848,735	332,087	294,822
`a6f4514`	openjdk	applyTypeMap(id,v3)	951,639	908,826	1,083,239	42,813	131,599
`a6689bc`	openjdk	applyTypeMap(id,v3)	1,080,714	1,024,838	1,120,194	55,875	39,480
`a6f4514`	openjdk	applyTypeMap(id,v4)	449,014	380,642	491,410	68,372	42,396
`a6689bc`	openjdk	applyTypeMap(id,v4)	480,909	361,007	493,837	119,902	12,928
`a6f4514`	openjdk	applyTypeMap(mapInts,v1)	1,361,795	1,162,717	1,433,297	199,078	71,502
`a6689bc`	openjdk	applyTypeMap(mapInts,v1)	1,395,522	1,333,792	1,473,311	61,730	77,790
`a6f4514`	openjdk	applyTypeMap(mapInts,v2)	881,566	831,334	931,736	50,232	50,171
`a6689bc`	openjdk	applyTypeMap(mapInts,v2)	908,371	867,399	958,703	40,972	50,332
`a6f4514`	openjdk	applyTypeMap(mapInts,v3)	151,217	140,365	157,185	10,852	5,968
`a6689bc`	openjdk	applyTypeMap(mapInts,v3)	34,095	19,082	35,680	15,014	1,585
`a6f4514`	openjdk	applyTypeMap(mapInts,v4)	47,729	43,171	49,587	4,558	1,858
`a6689bc`	openjdk	applyTypeMap(mapInts,v4)	14,768	9,174	15,650	5,595	882

I also ran with GraalVM; the results are similar to OpenJDK.

Conclusions

As expected, runtime when are types don't change are unaffected, as we don't enter the TreeTypeMap in these cases.

When we do need to enter the TreeTypeMap (in the cases applyTypeMap(mapInts,v3) and applyTypeMap(mapInts,v4)), the runtime in the fixed version is significantly slower (7x and 3x slower). This is expected as applying a TreeTypeMap is significantly slower than applying a TypeMap or TypeAccumulator.

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

compiler/src/dotty/tools/dotc/core/Annotations.scala

odersky

Overall LGTM I only had two questions.

mbovel · 2024-05-22T15:16:02Z

I addressed both questions (in 2 separate commits to ease review, but I can squash them afterward).

odersky

I still have several questions about the logic. Why the duplication? Why single out PostTyper? And, why does transforming an annotation require its symbols to be copied separately? Isn't that logic already handled in TreeTypeMap?

odersky · 2024-06-18T19:00:59Z

compiler/src/dotty/tools/dotc/core/Annotations.scala

        val diff = findDiff(NoType, args)
        if tm.isRange(diff) then EmptyAnnotation
-        else if diff.exists then derivedAnnotation(tm.mapOver(tree))
+        else if diff.exists then
+          // If the annotation has been transformed, we need to make sure that the


Why does transforming an annotation duplicate symbols? Don't we forget the old copy and use only the transformed one?

The problem is that we have an Annotated tree whose type is an AnnotatedType, the Annotated tree has an annot field which is a tree and the AnnotatedType has .annot.tree. Initially, these trees are equal, but in PostTyper we call transformAnnot on both separately, so if PostTyper does some transformation, we end up with two non-referentially-equal trees that define the same symbol and crash in pickler (when PostTyper#transformAnnot does nothing for a specific annotation, then there is only one shared tree and pickler is happy).

I think we could fix this locally in pickler by not pickling the annot field of the Annotated tree and instead reconstructing it from the type on unpickling, but more generally for type trees, it seems that Annotated#annot should just be a forward call to .tpe.tree.annot

Actually I spoke too soon, it seems that in practice the Annotated trees are kept in sync with their AnnotatedType. Things start going wrong when that AnnotatedType is used to type another tree (for example in

scala3/compiler/src/dotty/tools/dotc/ast/Desugar.scala

Line 153 in 99f431e

tpd.TypeTree(relocate(sym.info))

but likely also just through type inference). Since there's no easy way to know if the same AnnotatedType is shared by multiple trees, this PR assumes this is always the case and defensively performs symbol copies.

odersky · 2024-06-18T19:02:41Z

compiler/src/dotty/tools/dotc/transform/PostTyper.scala

+      try
+        val res = transform(annot)
+        if res ne annot then
+          // If the annotation has been transformed, we need to make sure that the


Why do we need to handle this here as well? I thought annotation transforms already took care of this themselves?

As we discussed orally at the time I believe, PostTyper doesn't use TypeMap, it instead calls transform with the annotation manually, hence the need to also copy it manually.

scala3/compiler/src/dotty/tools/dotc/transform/PostTyper.scala

Lines 157 to 166 in 5e83606

private def transformAnnot(annot: Tree)(using Context): Tree = {

val saved = inJavaAnnot

inJavaAnnot = annot.symbol.is(JavaDefined)

if (inJavaAnnot) checkValidJavaAnnotation(annot)

try transform(annot)

finally inJavaAnnot = saved

}

private def transformAnnot(annot: Annotation)(using Context): Annotation =

annot.derivedAnnotation(transformAnnot(annot.tree))

smarter · 2024-08-22T17:16:33Z

The following still fails with this PR:

class dummy(b: Boolean) extends annotation.StaticAnnotation with annotation.RefiningAnnotation

object Test:
  def foo(elem: Int, bla: Int @dummy(elem == 0)) = bla

-- Error: try/annn2.scala:9:37 -------------------------------------------------
9 |  def foo(elem: Int, bla: Int @dummy(elem == 0)) = bla
  |                                     ^^^^^^^^^
  |          undefined: elem.== # -1: TermRef(TermParamRef(elem),==) at typer

The initial typing of the arguments work, but when we want to create the MethodType using fromSymbols we perform a substitution. We correctly substitute TermRef(NoPrefix, elem) by a TermParamRef representing the first parameter of the MethodType, the TypeMap goes back up one level, and creates a new elem.== which can be done without forcing the underlying type of elem, but when we go back up one more level and create a new elem.==(0), the TypeAssigner for Apply forces the computation of the underlying type of elem.==, which in turn requires computing the underlying type of elem, where we get NoType because we're in the middle of computing paramInfos:

scala3/compiler/src/dotty/tools/dotc/core/Types.scala

Lines 4697 to 4700 in d490d13

    
           override def underlying(using Context): Type = { 
        
             // TODO: update paramInfos's type to nullable 
        
             val infos: List[Type] | Null = binder.paramInfos 
        
             if (infos == null) NoType // this can happen if the referenced generic type is not initialized yet

scala3/compiler/src/dotty/tools/dotc/core/Types.scala

Line 4091 in d490d13

val paramInfos: List[Type] = paramInfosExp(this: @unchecked)

To avoid this issue it seems we'd need paramInfos to be filled progressively as we compute each parameter, so the second parameter can at least safely refer to the first one.

mbovel · 2024-08-29T09:27:57Z

I rebased on top of main and reverted the logic that always copies symbols in annotations. As it stands, this PR would fix #5789 and #18064, but not #17939 and #19846. Could we merge this already and tackle other problems—including duplicate symbols and what @smarter described above—in separate PRs?

mbovel · 2024-09-12T15:21:21Z

I squashed two commits together to minimize the changes. No code changes.

`Annotation.mapWith` maps an `Annotation` with a type map `tm`. As an optimization, this function first checks if `tm` would result in any change (by traversing the annotation’s argument trees with a `TreeAccumulator`) before applying `tm` to the whole annotation tree. This optimization had two problems: 1. it didn’t include type parameters, and 2. it used `frozen_=:=` to compare types, which didn’t work as expected with `NoType`. This commit fixes these issues. Additionally, positions of trees that appear only inside `AnnotatedType` were not pickled. This commit also fixes this.

mbovel requested a review from odersky March 15, 2024 23:40

mbovel requested a review from Linyxus March 27, 2024 17:06

mbovel force-pushed the mb/19846 branch from b92fa3e to 3bd7df2 Compare April 16, 2024 16:59

This comment was marked as outdated.

Sign in to view

mbovel changed the title ~~Fix mapping of annotations containing defs~~ Fix mapping and pickling of non-trivial annotations Apr 17, 2024

mbovel force-pushed the mb/19846 branch 2 times, most recently from 878cc3e to 131f2e3 Compare April 18, 2024 12:26

mbovel mentioned this pull request May 1, 2024

Cleanup retains annotations in all inferred type trees #20305

Open

mbovel force-pushed the mb/19846 branch from 131f2e3 to ee4edb1 Compare May 1, 2024 16:26

mbovel removed request for odersky and Linyxus May 2, 2024 08:12

mbovel force-pushed the mb/19846 branch 2 times, most recently from 95a664f to 1cf88db Compare May 2, 2024 10:26

Linyxus reviewed May 5, 2024

View reviewed changes

tests/pos-custom-args/captures/i20272b.scala Outdated Show resolved Hide resolved

mbovel force-pushed the mb/19846 branch 2 times, most recently from 4a0cff8 to 58e5f74 Compare May 6, 2024 12:48

mbovel force-pushed the mb/19846 branch 2 times, most recently from 0d1183d to 76492df Compare May 7, 2024 12:18

mbovel requested a review from odersky May 7, 2024 13:23

mbovel assigned odersky May 7, 2024

odersky requested changes May 19, 2024

View reviewed changes

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala Show resolved Hide resolved

compiler/src/dotty/tools/dotc/core/Annotations.scala Outdated Show resolved Hide resolved

compiler/src/dotty/tools/dotc/core/Annotations.scala Outdated Show resolved Hide resolved

odersky assigned mbovel and unassigned odersky May 19, 2024

odersky reviewed May 19, 2024

View reviewed changes

mbovel force-pushed the mb/19846 branch from 3740c2d to 12ad085 Compare May 22, 2024 14:27

mbovel requested a review from odersky May 22, 2024 15:16

mbovel assigned odersky and unassigned mbovel Jun 11, 2024

odersky reviewed Jun 18, 2024

View reviewed changes

odersky assigned mbovel and unassigned odersky Jun 18, 2024

Use uninitialized instead of _ in lazyval benchmarks

85b1c0e

mbovel force-pushed the mb/19846 branch from 12ad085 to 2419176 Compare August 29, 2024 08:56

mbovel changed the title ~~Fix mapping and pickling of non-trivial annotations~~ Some fixes for AnnotatedTypes mapping Aug 29, 2024

smarter approved these changes Aug 29, 2024

View reviewed changes

mbovel requested a review from odersky August 29, 2024 14:30

mbovel force-pushed the mb/19846 branch from 2419176 to 08e8561 Compare September 12, 2024 15:20

mbovel force-pushed the mb/19846 branch from 08e8561 to 3e1667c Compare September 12, 2024 15:22

mbovel added 3 commits September 13, 2024 08:51

Add AnnotationsMappingBenchmark

90d99c3

Run printing tests in a separate directory

56fe7cf

mbovel force-pushed the mb/19846 branch from 3e1667c to ac76938 Compare September 13, 2024 06:52

mbovel merged commit c9b9aea into scala:main Sep 13, 2024
28 checks passed

mbovel deleted the mb/19846 branch September 13, 2024 09:34

mbovel mentioned this pull request Sep 15, 2024

Undefined reference when typing dependent annotation #21595

Closed

WojciechMazur added this to the 3.6.0 milestone Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some fixes for AnnotatedTypes mapping #19957

Some fixes for AnnotatedTypes mapping #19957

mbovel commented Mar 15, 2024 •

edited

Loading

mbovel commented Mar 16, 2024 •

edited

Loading

This comment was marked as outdated.

mbovel commented May 6, 2024

dottybot commented May 6, 2024

dottybot commented May 6, 2024

odersky commented May 6, 2024

odersky commented May 6, 2024

dottybot commented May 6, 2024

dottybot commented May 6, 2024

mbovel commented May 7, 2024 •

edited

Loading

odersky left a comment

mbovel commented May 22, 2024

odersky left a comment

odersky Jun 18, 2024

smarter Aug 15, 2024 •

edited

Loading

smarter Aug 18, 2024

odersky Jun 18, 2024

mbovel Aug 29, 2024

smarter commented Aug 22, 2024

mbovel commented Aug 29, 2024

mbovel commented Sep 12, 2024

	private def transformAnnot(annot: Tree)(using Context): Tree = {
	val saved = inJavaAnnot
	inJavaAnnot = annot.symbol.is(JavaDefined)
	if (inJavaAnnot) checkValidJavaAnnotation(annot)
	try transform(annot)
	finally inJavaAnnot = saved
	}

	private def transformAnnot(annot: Annotation)(using Context): Annotation =
	annot.derivedAnnotation(transformAnnot(annot.tree))

Some fixes for AnnotatedTypes mapping #19957

Some fixes for AnnotatedTypes mapping #19957

Conversation

mbovel commented Mar 15, 2024 • edited Loading

mbovel commented Mar 16, 2024 • edited Loading

This comment was marked as outdated.

mbovel commented May 6, 2024

dottybot commented May 6, 2024

dottybot commented May 6, 2024

odersky commented May 6, 2024

odersky commented May 6, 2024

dottybot commented May 6, 2024

dottybot commented May 6, 2024

mbovel commented May 7, 2024 • edited Loading

Annotation.mapWith micro benchmark

Setup

Results

Conclusions

odersky left a comment

Choose a reason for hiding this comment

mbovel commented May 22, 2024

odersky left a comment

Choose a reason for hiding this comment

odersky Jun 18, 2024

Choose a reason for hiding this comment

smarter Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

smarter Aug 18, 2024

Choose a reason for hiding this comment

odersky Jun 18, 2024

Choose a reason for hiding this comment

mbovel Aug 29, 2024

Choose a reason for hiding this comment

smarter commented Aug 22, 2024

mbovel commented Aug 29, 2024

mbovel commented Sep 12, 2024

mbovel commented Mar 15, 2024 •

edited

Loading

mbovel commented Mar 16, 2024 •

edited

Loading

mbovel commented May 7, 2024 •

edited

Loading

`Annotation.mapWith` micro benchmark

smarter Aug 15, 2024 •

edited

Loading