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(#19806): wrong tasty of scala module class reference #19827

Merged

Conversation

i10416
Copy link
Contributor

@i10416 i10416 commented Feb 29, 2024

This commit makes the following diff to TASTy for i17255 files. The TASTy before this commit relied on the compiler (aka all TASTy clients) intrinsically knowing how to resolve Module$ when the definition is actually Module[ModuleClass].

scalac tests/run/i17255/J.java tests/run/i17255/Module.scala -Yprint-tasty -Yjava-tasty
    90:         EMPTYCLAUSE
    91:         TERMREF 17 [Module]
    93:           SHAREDtype 12
    95:         ELIDED
    96:           SHAREDtype 91
    98:         STATIC
    99:       DEFDEF(12) 18 [module]
   102:         EMPTYCLAUSE
-  103:         SELECTtpt 19 [Module$]
+  103:         SELECTtpt 19 [Module[ModuleClass]]
   105:           SHAREDtype 3
   107:         ELIDED
   108:           TYPEREF 17 [Module]
   110:             SHAREDtype 3
   112:         STATIC

fixes #19806

This commit makes the following diff to TASTy for i17255
files. The TASTy before this commit relied on the compiler (aka all TASTy clients)
intrinsically knowing how to resolve Module$ when the definition is actually Module[ModuleClass].

```sh
scalac tests/run/i17255/J.java tests/run/i17255/Module.scala -Yprint-tasty -Yjava-tasty
```

```diff
    90:         EMPTYCLAUSE
    91:         TERMREF 17 [Module]
    93:           SHAREDtype 12
    95:         ELIDED
    96:           SHAREDtype 91
    98:         STATIC
    99:       DEFDEF(12) 18 [module]
   102:         EMPTYCLAUSE
-  103:         SELECTtpt 19 [Module$]
+  103:         SELECTtpt 19 [Module[ModuleClass]]
   105:           SHAREDtype 3
   107:         ELIDED
   108:           TYPEREF 17 [Module]
   110:             SHAREDtype 3
   112:         STATIC
```
@bishabosha bishabosha self-assigned this Feb 29, 2024
@bishabosha bishabosha self-requested a review February 29, 2024 16:53
@i10416
Copy link
Contributor Author

i10416 commented Mar 9, 2024

@bishabosha Could you review this change?

@bishabosha
Copy link
Member

right, I will do that this week

@i10416
Copy link
Contributor Author

i10416 commented Mar 10, 2024

Thank you.

@bishabosha
Copy link
Member

bishabosha commented Mar 11, 2024

turns out we should revert the remapping of class Module$ to object Module, it causes too many downstream problems, I will push the fix

e.g.

   107:         ELIDED
   108:           TYPEREF 17 [Module] // should be TERMREF
   110:             SHAREDtype 3

@nicolasstucki
Copy link
Contributor

@bishabosha is there a way to add a test for this?

@bishabosha
Copy link
Member

@bishabosha is there a way to add a test for this?

im guessing we can make a config option for TastyPrinter to elide absolute paths and use check file

@bishabosha bishabosha force-pushed the fix/19806-wrong-tasty-of-scala-mod-cls-ref branch 3 times, most recently from c81113c to a7d9cbd Compare March 12, 2024 17:30
@bishabosha bishabosha requested a review from sjrd March 12, 2024 17:33
@bishabosha bishabosha assigned sjrd and unassigned bishabosha Mar 12, 2024
@sjrd
Copy link
Member

sjrd commented Mar 13, 2024

Pickling test for tests/pos/i19806/J.tastycheck is still failing, apparently.

@bishabosha
Copy link
Member

im trying to elide the source names

@bishabosha bishabosha force-pushed the fix/19806-wrong-tasty-of-scala-mod-cls-ref branch from a7d9cbd to cfd9324 Compare March 13, 2024 10:43
@bishabosha bishabosha force-pushed the fix/19806-wrong-tasty-of-scala-mod-cls-ref branch from cfd9324 to 090b647 Compare March 13, 2024 10:48
@bishabosha bishabosha force-pushed the fix/19806-wrong-tasty-of-scala-mod-cls-ref branch from 3adcf86 to 923f63f Compare March 13, 2024 11:06
@bishabosha bishabosha force-pushed the fix/19806-wrong-tasty-of-scala-mod-cls-ref branch from 5778f8e to 1d5db00 Compare March 13, 2024 13:35
@bishabosha
Copy link
Member

@sjrd compilation tests passing now

Comment on lines +329 to +330
// handle check file path mismatch on windows
actual1 == expect1 || File.separatorChar == '\\' && actual1.replace('\\', '/') == expect1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still necessary? IIUC you elide paths entirely now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, I could remove that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to do another PR to warn about not adding the -Ytest-pickler-check flag in the presence of .tastycheck so it can be picked up there

@bishabosha bishabosha merged commit 4859415 into scala:main Mar 13, 2024
19 checks passed
@bishabosha
Copy link
Member

thanks again @i10416 for pushing the process along!

@i10416
Copy link
Contributor Author

i10416 commented Mar 13, 2024

@bishabosha My pleasure. Thank you for following up hard part.

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.

Fix Java TASTy of scala module class references
5 participants