Fix resolution of generated and provider-forwarded code in the IDE#401
Fix resolution of generated and provider-forwarded code in the IDE#401Frank Portman (FrankPortman) wants to merge 6 commits into
Conversation
|
We need a demo project which highlights the problems being fixed. Though the changes are looking reasonable (with some flaws), it is impossible for me to evaluate the PR without seeing the root cause |
|
Evgenii Pasynkov (@noxvost) please see the demo project linked in this PR. I tried to make it as little code as possible, while showcasing the exact setup we have in our monorepo. I am not sure if some of what happened in my monorepo is a "perfect storm" scenario, but you can see the README (LLM generated, but I verified) in the demo project which shows the 4 things that were broken in our monorepo. Really it's 1 thing (Scala/Python codegen did not resolve) but I discovered stuff along the way to trying to fix that at varying levels. To restate them plainly:
|
NothingButYou (Krystian20857)
left a comment
There was a problem hiding this comment.
Thanks for PR,
However I'll take a look at provided reproduction cases myself and check what's exactly is going on inside.
| // binaryOutputs and not outputJars: the latter is filtered to files that exist | ||
| // on disk when the hard links are created, so jars that were never built (e.g. | ||
| // in a configuration produced by a transition) would silently drop out of the | ||
| // index and the corresponding libraries would never be redirected to their | ||
| // source module. Identity comes from the aspect info and does not require the | ||
| // files to exist. | ||
| (target.findBuildData<JvmBuildTarget>()?.binaryOutputs ?: SourceFileCollection.EMPTY).getFiles() | ||
| .map { it.normalizeOutputPath() to target.targetKey } |
There was a problem hiding this comment.
This one was suppose to substitute compiled JARs (mainly hjars) which are pulled by manually constructed JavaInfo provider by code generator.
There was a problem hiding this comment.
from what I understand, the problem with outputJars for this index is the existence filter, i.e. in a transitioned configuration the jars are never built locally, so exactly those entries drop out and the libraries never redirect to their source modules.
binaryOutputs carries the same identities without needing files on disk. if outputJars has to stay as the hjar-substituted view for the codegen case, happy to point the index at a dedicated field instead.
There was a problem hiding this comment.
Fair point, I'll take closer look at this particular case
| @VisibleForTesting | ||
| fun calculateTargetsToMarkAsTest( | ||
| targets: Set<RawBuildTarget>, | ||
| labelToTargetInfo: Map<Label, RawBuildTarget>, | ||
| executableTargets: Map<ResolvedLabel, List<Label>>, | ||
| ): Set<Label> { | ||
| val (testTargets, nonTestTargets) = | ||
| targets.partition { it.isTestTarget() } | ||
| val libraryTargets = nonTestTargets.filter { it.kind.ruleType == RuleType.LIBRARY }.map { it.id } | ||
| val directTestDependencies = libraryTargets.filter { library -> | ||
| val executables = executableTargets[library]?.mapNotNull { labelToTargetInfo[it] }.orEmpty() | ||
| val executablesWithoutSelfReference = executables.filterNot { it.id == library } | ||
| val directExecutables = executablesWithoutSelfReference.filter { it.dependsOn(library) } | ||
| return@filter directExecutables.isNotEmpty() | ||
| && executablesWithoutSelfReference.all { it.isTestTarget() } | ||
| && directExecutables.any { it.sources.isEmpty() && it.id.packagePath == library.packagePath } | ||
| }.toSet() | ||
|
|
||
| // Everything any non-test executable depends on, transitively, is production code and | ||
| // must never be marked as test sources. ExecutableTargetsComputer cannot be used to | ||
| // answer this: its same-package attribution shortcut and its result cap are run | ||
| // configuration heuristics, and both can hide production dependents of a library that | ||
| // also has a test right next to it (e.g. a code generation macro emitting a library | ||
| // together with a same-package sourceless test). | ||
| val dependenciesOf = targets.associate { it.id to it.dependencies.map { dep -> dep.targetKey.label } } | ||
| fun reachableFrom(roots: Collection<Label>): Set<Label> { | ||
| val visited = HashSet<Label>(roots) |
There was a problem hiding this comment.
Marcin Kocot (@mrkocot) Can you take a look at this?
There was a problem hiding this comment.
i think this ended up being the fix for what #401 (comment) was also related to. I am hoping the configuration stuff was not actually needed. will check
There was a problem hiding this comment.
I've tested it with both normal and tricky cases I remember used to be problematic, works for them well
There was a problem hiding this comment.
can i resolve this comment?
| // Dual-language targets (e.g. custom rules forwarding both PyInfo and JavaInfo) are | ||
| // owned by the JVM importer: both importers derive the same module name, so creating | ||
| // a Python module here would clobber the JVM module and break JVM consumers. | ||
| if (target.rawBuildTarget.kind.isJvmTarget()) continue |
There was a problem hiding this comment.
This is a known issue, however we should aim for more generic solution
https://youtrack.jetbrains.com/issues/BAZEL?preview=BAZEL-2214
There was a problem hiding this comment.
that link doesn't seem to take me to any specific issue
There was a problem hiding this comment.
ah, issue is private, but related to java + python module names over lapping, we're working on this one
There was a problem hiding this comment.
Frank Portman (@FrankPortman) could you please check whenever removing this check impact code resolution inside your repository?
There was a problem hiding this comment.
tested it: unfortunately this breaks resolution. claude claims it breaks non-deterministically based on a race condition (the workspace model rejects the second write for dual-language targets), but I did not try a bunch of cold runs because of how long it takes to sync my full monorepo.
Verbatim from the log:
MutableEntityStorageImpl - Entity with symbolicId: ModuleId(name=<redacted>-514860d) already exist
In this case many objects were fine but I happened to see a few breaking in different test files that were fine before. Confirmed it's normal again after adding this commit back.
One extra data point, that Claude seems to think is relevant: even with this change I still see LibraryId collisions between the *_python entities of configuration duplicates, so the overlap isn't just java vs python.
Verbatim from Claude:
Yes, exactly. On the current review-tip build (skip in place), this session's log shows:
- ModuleId collisions: 0 — the java-vs-python module clobber is fully gone
- LibraryId collisions: 126 — all shaped like <pkg>.<target>_python-514860d already exist, i.e. the Python importer's own source-dependency libraries colliding with themselves: two instances of the same Python target (config/aspect duplicates) both writing a library under the same name
It's benign in practice — the colliding libraries are content-identical duplicates, so whichever write wins carries the same roots, which is why everything stays green. But it demonstrates the naming overlap is python-vs-python too, not just java-vs-python — useful scope information for their generic BAZEL-2214 fix, which is why it earns the one-sentence mention in the comment.
Source jars produced by Bazel rules (e.g. scrooge_scala_library) use the .srcjar extension. Without the archive mapping, library source roots pointing at them cannot be opened by the VFS: the IDE decompiles class jars even though the source jar is attached, and manually attaching sources fails too. The legacy plugin registers the extension the same way, see BlazeFileTypeFactory in bazelbuild/intellij.
Library roots for source jars with the .srcjar extension were written as file:// URLs, which cannot serve as sources roots: the IDE cannot look inside the archive. Matches the extension handling of the legacy plugin's LibraryModifier (jar, srcjar, zip).
…braries A jar already provided by a target's library got a second, synthetic jdeps library without source jars. The duplicate competes with the proper library during resolution and the IDE ends up decompiling class files even though sources are attached to the real library.
The resolve index mapped generated sources to their hard link copy, which is not covered by any module content root: navigation opened an orphan file in which no reference resolves further. The module's content roots are anchored at bazel-bin, so prefer the real output path and keep the hard link only as a fallback for files that are not on disk.
Both the Python and the JVM importer derive the same module name for a target that carries both PythonBuildTarget and JVM data (e.g. a custom rule forwarding PyInfo and JavaInfo). The Python module has no JVM dependencies, so when it wins, every JVM consumer of the target stops resolving. Skipping such targets in the Python importer is an interim fix; the proper modelling is a Python facet on the JVM module, as done by the legacy plugin.
One minimal Bazel project with four packages, one per problem class fixed in this PR and in the companion aspect PR: generated Scala and Python that do not resolve after a plain sync, multi-hop navigation between generated libraries, scrooge thrift sources resolving to decompiled class files, and a library classified as test sources because its codegen macro emits a same-package sourceless test next to it while production code reaches it only through a provider-forwarding rule. The README maps each package to the fixing commits. Builds standalone with Bazel 9.1.1.
61017e3 to
54f37d3
Compare
Companion to JetBrains/intellij-aspect#143. Fixes resolution of generated and provider-forwarded code, diagnosed and verified against a production monorepo (generated Scala/Python, scrooge thrift, provider-forwarding custom rules). Runnable repro of all problem classes:
pluginTests/testData/testProjects/generated_code_resolution..srcjaris not an archive file type, sojar://...srcjar!/sources roots are dead: decompiled view, manual attach fails too.srcjaras ARCHIVE (legacy:BlazeFileTypeFactory)toJarUrlString()writesfile://URLs for.srcjar, unusable as sources rootsjar,srcjar,zip(legacy:LibraryModifier)MutableEntityStorageImpl: symbolicId already exist), so whichever importer runs second loses its module, nondeterministically per target and syncTwo fixes still needed from intellij-aspect#143:
python_target_info.generated_sources(feeds the resolve index,CustomRuleTestthere), and dependency edge serialization for custom attributes, which BAZEL-3364's test classification relies on for provider-forwarding rules.