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

Align tasty file lookup with dotty, add test case #10694

Merged
merged 2 commits into from Feb 19, 2024

Conversation

bishabosha
Copy link
Member

it seems this is not strictly necessary to prevent errors, it just aligns with what dotty does and I guess prunes more files

@scala-jenkins scala-jenkins added this to the 2.13.14 milestone Feb 19, 2024
@bishabosha bishabosha marked this pull request as ready for review February 19, 2024 14:36
@SethTisue
Copy link
Member

Are you suggesting we last-minute merge this for 2.13.13, or it's fine to leave it for 2.13.14?

@bishabosha
Copy link
Member Author

I think for the 2.13.13

@SethTisue SethTisue modified the milestones: 2.13.14, 2.13.13 Feb 19, 2024
@SethTisue
Copy link
Member

SethTisue commented Feb 19, 2024

minor point, but e.g. .stripSuffix(".class") could be a bit less error-prone, and show intent more clearly, than .dropRight(6)

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)

@bishabosha
Copy link
Member Author

@SethTisue I've added the change

@lrytz
Copy link
Member

lrytz commented Feb 19, 2024

I think for the 2.13.13

I think so too

@@ -334,7 +334,7 @@ case class DirectoryClassPath(dir: File) extends JFileDirectoryLookup[ClassFileE

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)))
Copy link
Member

@lrytz lrytz Feb 19, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@SethTisue SethTisue merged commit ec6078f into 2.13.x Feb 19, 2024
5 checks passed
@SethTisue SethTisue deleted the tasty/test-classpath-standalone-obj branch February 19, 2024 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants