Fix #413: Handle inner classes based on content, not names. #419
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjrd
force-pushed
the
fix-weird-toplevel-scala2-names
branch
from
a4f9628
to
c818a7f
Compare
sjrd
changed the title
adpi2
approved these changes
Dec 11, 2023
| true | ||
| else if doLoadClassFile(classData, loadedFiles) then true | ||
| else | ||
| // Oops, maybe we will need this one later; removing it from loadedFiles |
There was a problem hiding this comment.
Maybe you should explain in which situation we would need it again
|
|
||
| if result == ClassKind.Java && innerClassesStream != null then | ||
| if containsSelfInnerClassDecl(structure.binaryName, innerClassesStream.nn) then ClassKind.JavaInnerOrArtifact | ||
| else ClassKind.Java |
There was a problem hiding this comment.
Should we expect this else to happen?
There was a problem hiding this comment.
That is definitely expected. Top-level classes have InnerClasses data for their inner classes, and even for any other inner class of other classes that they happen to refer to.
Previously, we assumed that if there is a file `Foo` and a file `Foo$Bar`, that the latter must be an inner class (or companion) of `Foo`, and that it must therefore be ignored. As scalacenter#413 shows, and as the new `JavaDefined$Evil` test shows, this is not always valid. The only true test of a class being top-level can only be found in its content, either through the `Scala` attribute or the `InnerClasses` attribute. We completely rewrite `Loaders`. We now systematically try to load every file that is requested, except when we *already* know that it is a Java inner class. This means that we will read the contents of more files than before, but still each file at most once in the common case. A file *may* be read multiple times if it is a Java non-top-level class that gets requested by its name as if it were a top-level class. The loading will fail but we have to keep the file in case it is loaded later on as an inner class. When reading *all* the declarations of a package, we always try outer classes before potential inner classes so that no double reading happens.
sjrd
force-pushed
the
fix-weird-toplevel-scala2-names
branch
from
c818a7f
to
648de54
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we assumed that if there is a file
Fooand a fileFoo$Bar, that the latter must be an inner class (or companion) ofFoo, and that it must therefore be ignored.As #413 shows, and as the new
JavaDefined$Eviltest shows, this is not always valid.The only true test of a class being top-level can only be found in its content, either through the
Scalaattribute or theInnerClassesattribute.We completely rewrite
Loaders. We now systematically try to load every file that is requested, except when we already know that it is a Java inner class. This means that we will read the contents of more files than before, but still each file at most once in the common case.A file may be read multiple times if it is a Java non-top-level class that gets requested by its name as if it were a top-level class. The loading will fail but we have to keep the file in case it is loaded later on as an inner class.
When reading all the declarations of a package, we always try outer classes before potential inner classes so that no double reading happens.
The following
TypeSuitetest passes with this PR with the"org.scalaz" %% "scalaz-core" % "7.3.8"dependency:Moreover, I re-tested locally the local test from PR #416.