-
-
Notifications
You must be signed in to change notification settings - Fork 686
Enhance Java first party dependency inference to support unqualified, same-package references #22817
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
Open
hopper-signifyd
wants to merge
12
commits into
pantsbuild:main
Choose a base branch
from
hopper-signifyd:jvm-infer-first-party-same-package
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Enhance Java first party dependency inference to support unqualified, same-package references #22817
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
c476687
Infer first party deps within the same package even when imports are …
hopper-signifyd 48ce425
improvements
hopper-signifyd ddb7748
remove logger call
hopper-signifyd 6b4fa79
simplify
hopper-signifyd 864717a
revert changes
hopper-signifyd a9055ff
fix
hopper-signifyd 6d85900
remove log
hopper-signifyd e6f6b4e
Fix issue with two-step non-obvious deps
hopper-signifyd 60fa6b1
Merge branch 'main' into jvm-infer-first-party-same-package
hopper-signifyd 7c49e98
take a simpler approach
hopper-signifyd 3e72657
remove exports field
hopper-signifyd 938d860
cleanup redundant fielddeclaration usage
hopper-signifyd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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 don't know the JVM backend too well, but I guess this is necessary because we only put the direct deps of A on the compiler classpath? So we have to treat C as a direct dep.
But shouldn't we really be putting all transitive deps on the compiler classpath? After all, couldn't we contrive a more complex situation where C has a reference to some D etc?
And if we did so then presumably this problem wouldn't occur, because we do detect that B depends on C and A depends on B, and therefore C is in the transitive closure?
Or are there reasons not to put all transitive dep classes classpath?
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.
That's a good point. In my experience, all transitive deps should be on the classpath when compiling Java sources, so I'm not sure why we're not doing it here. I agree that's a better approach. Let me revisit this.
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'm assuming the reason for not just putting all transitive dep classes on the classpath was potentially performance related? (although I didn't run a performance test with and without this change) Putting all transitive dep classes on the classpath is good from a correctness standpoint and obviates the need for us to put special logic in place for exporting certain classes and references ahead to other consumers.
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.
Possibly, I'll see if I can find someone who has context for this decision. As far as I can tell (and AI seems to agree) compiling A conceptually requires all its transitive deps, and trying to tease out which ones are "really" needed is a mug's game.
I had thought we were already doing this though, I'm a bit surprised if we're not. But if we were your use case would work, no? I assume we do correctly infer the deps A->B and B->C already?
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.
Correct, if we were already doing this, then I wouldn't have had compilation errors in my project for the A -> B -> C use case. But the other issue this PR is solving around inner classes still would have been a problem, but now it seems the two are unrelated aside from the fact they require fixes to the same files
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.
So it probably makes sense to split this into two PRs. The inner classes one is fairly straightforward and we can get it in ASAP. The other is a potentially more substantial change that would be best reviewed in isolation.
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.
Yeah, that's a good idea. I've split the inner class changes out into #22889 I'll rebase this PR to just include the transitive dep stuff after that other PR lands.