Align tasty file lookup with dotty, add test case #10694
Conversation
|
Are you suggesting we last-minute merge this for 2.13.13, or it's fine to leave it for 2.13.14? |
|
I think for the 2.13.13 |
|
minor point, but e.g. regardless, sure, seems reasonable for 2.13.13. we try to avoid merging stuff last second like this, but it's confined to the TASTy reader, and anyway all the other TASTy reader changes were pretty late-breaking and aren't battle-tested yet either, so merging one PR hardly changes our overall risk level (risk of having to rush out a 2.13.14) note that we are hoping to publish 2.13.13 to Maven Central tomorrow (Tuesday) |
|
@SethTisue I've added the change |
I think so too |
| protected def createFileEntry(file: AbstractFile): ClassFileEntryImpl = ClassFileEntryImpl(file) | ||
| protected def isMatchingFile(f: File, siblingExists: String => Boolean): Boolean = | ||
| f.isClass && !(f.getName.endsWith(".class") && siblingExists(f.getName.dropRight(6) + ".tasty")) | ||
| f.isClass && !(f.getName.endsWith(".class") && siblingExists(classNameToTasty(f.getName))) |
There was a problem hiding this comment.
How about testing both cases? I.e., if the classfile is X$.class, drop it if there is either X.tasty or X$.tasty.
If a user defineds class A$, Scala 3 emits A$.class and A$.tasty.
There was a problem hiding this comment.
that seems like a lot of extra lookups to do - Is it really useful being more restrictive than what dotty allows?
I think the assumption being made here is that you shouldn't be doing class Foo$ in source code, of course that isn't enforced.
There was a problem hiding this comment.
Right, the extra lookups are bad.
IIUC, the compiler will anyway find the A$.tasty file and load it. Doing so should re-use an already existing symbol for A$ that could be there from loading the .class file.
it seems this is not strictly necessary to prevent errors, it just aligns with what dotty does and I guess prunes more files