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

reproducible builds: undeterministic additional source path references in tasty #21154

Closed
raboof opened this issue Jul 10, 2024 · 7 comments · Fixed by #21214
Closed

reproducible builds: undeterministic additional source path references in tasty #21154

raboof opened this issue Jul 10, 2024 · 7 comments · Fixed by #21214
Assignees
Labels
area:tasty-format issues relating to TASTy as a portable standard itype:bug
Milestone

Comments

@raboof
Copy link
Contributor

raboof commented Jul 10, 2024

We encountered a scala 3 binary reproducibility problem in Pekko (apache/pekko-http#566, apache/pekko-connectors#739): on a clean compile, FcmOption.tasty seems to have references to FcmJsonSupport, but it does not seem to be deterministic what @Child annotation position id those references point to.

When compiling FcmOption in isolation, the references to FcmJsonSupport do not appear in the .tasty file at all, which begs the question whether they should be created at all.

Compiler version

3.3.3

(3.4.2 also generated the suspicious references, but we haven't tested yet whether they are deterministic there)

Minimized code

I have a fairly minimized project showing the surprising references to FcmJsonSupport at https://codeberg.org/raboof/scala3-reproduce-21154

Output

  source paths:
     0: 19 [FcmOption.scala]
    69: 27 [FcmJsonSupport.scala]

Expectation

Maybe no reference to FcmJsonSupport at all, or at least deterministic ones.

It seems like, when (e.g.) FcmOptions is encountered in FcmJsonSupport, the Child annotation linking FcmOptions and FcmOption is already created, but still with the Context of FcmJsonSupport - though neither FcmOptions nor FcmOption are in that file. It seems at some point the context should be updated, though I'm not familiar enough with the code to make a good suggestion where exactly :)

@bishabosha
Copy link
Member

bishabosha commented Jul 10, 2024

before investigating, I know that the children operation that forces any @Child annotations to be resolved is computed on demand - it could be possible that this code path isn't always reached before pickling

Edit: seems a red herring

@raboof
Copy link
Contributor Author

raboof commented Jul 10, 2024

FWIW all 3 @Child annotations are present even when compiling FcmOption in isolation, it seems it's only the position/source references that are different

@bishabosha
Copy link
Member

indeed here is the diff when following the steps in the reproducer

diff --git a/before.tastyout b/after.tastyout
index 65f90bc..7e49781 100644
--- a/before.tastyout
+++ b/after.tastyout
@@ -4,9 +4,9 @@ target/scala-3.3.3/classes/FcmOption.tasty
 Header:
   version: 28.3.0
   tooling: Scala 3.3.3
-     UUID: 00db1032-8125-36cd-004a-18b3ccf8a98d
+     UUID: 00c12d31-02a5-e9cd-0053-5f122bde978d
 
-Names (259 bytes, starting from 35):
+Names (237 bytes, starting from 35):
      0: ASTs
      1: <empty>
      2: FcmOption
@@ -34,10 +34,9 @@ Names (259 bytes, starting from 35):
     24: ApnsFcmOptions
     25: FcmOptions
     26: Positions
-    27: FcmJsonSupport.scala
-    28: Comments
+    27: Comments
 
-Trees (105 bytes, starting from 296):
+Trees (105 bytes, starting from 274):
      0: PACKAGE(103)
      2:   TERMREFpkg 1 [<empty>]
      4:   TYPEDEF(99) 2 [FcmOption]
@@ -94,7 +93,7 @@ Trees (105 bytes, starting from 296):
    103:             SHAREDtype 62
    105:
 
-Positions (97 bytes, starting from 403):
+Positions (89 bytes, starting from 381):
   lines: 51
   line sizes:
      2, 69, 72, 2, 48, 2, 78, 3, 0, 2, 70, 3, 0, 22, 0, 3, 20, 91, 3, 64
@@ -112,17 +111,13 @@ Positions (97 bytes, starting from 403):
     40: 364 .. 364
     56: 377 .. 377
     60: 377 .. 377
-    69: 377 .. 377
     77: 377 .. 377
     81: 377 .. 377
-    89: 377 .. 377
     97: 377 .. 377
    101: 377 .. 377
 
   source paths:
      0: 19 [FcmOption.scala]
-    69: 27 [FcmJsonSupport.scala]
-    89: 27 [FcmJsonSupport.scala]
 
 
 --------------------------------------------------------------------------------

@raboof
Copy link
Contributor Author

raboof commented Jul 10, 2024

 Header:
   version: 28.3.0
   tooling: Scala 3.3.3
-     UUID: 00db1032-8125-36cd-004a-18b3ccf8a98d
+     UUID: 00c12d31-02a5-e9cd-0053-5f122bde978d
 
-Names (259 bytes, starting from 35):
+Names (237 bytes, starting from 35):

somewhat offtopic, but out of curiosity: how do you get this representation? scalac -print-tasty doesn't show the header for me.

@bishabosha
Copy link
Member

bishabosha commented Jul 10, 2024

scalac -print-tasty doesn't show the header for me.

it does since 3.4

@bishabosha
Copy link
Member

I have been doing some investigation - so the source file of the context when constructing two of the child annotations is FcmJsonSupport.scala, despite them being located in FcmOption.scala, not clear why so far

@Gedochao Gedochao added area:tasty-format issues relating to TASTy as a portable standard and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Jul 10, 2024
@bishabosha
Copy link
Member

So I have a fix coming - the trace seems to be that the compiler is typing FcmJsonSupport.scala, then when it comes across a value, it needs to type ahead the signature - and then at that point it starts completing e.g. FcmOptions, at which point that needs to register as a child of its owner - and currently that is done in the context of the caller, and not the context of where the class originated from - so it completes in FcmJsonSupport.scala. The non determinism is that when you delete the class on the classpath, then the origin of the completer changes, so then it is correct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:tasty-format issues relating to TASTy as a portable standard itype:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants