-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(#19806): wrong tasty of scala module class reference #19827
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
Conversation
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 Could you review this change? |
|
right, I will do that this week |
|
Thank you. |
|
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 |
|
@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 |
c81113c to
a7d9cbd
Compare
|
Pickling test for |
|
im trying to elide the source names |
a7d9cbd to
cfd9324
Compare
cfd9324 to
090b647
Compare
3adcf86 to
923f63f
Compare
5778f8e to
1d5db00
Compare
|
@sjrd compilation tests passing now |
| // handle check file path mismatch on windows | ||
| actual1 == expect1 || File.separatorChar == '\\' && actual1.replace('\\', '/') == expect1 |
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.
Is this still necessary? IIUC you elide paths entirely now.
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.
true, I could remove that
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 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
|
thanks again @i10416 for pushing the process along! |
|
@bishabosha My pleasure. Thank you for following up hard part. |
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].
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: STATICfixes #19806