Detect Scala/Python targets beyond rule kinds, report generated Python, serialize custom dependency edges#143
Conversation
| def _source_files(ctx): | ||
| return [ | ||
| f | ||
| for t in intellij_common.attr_as_label_list(ctx, "srcs") |
There was a problem hiding this comment.
As we're dealing with custom rules, we cannot assume that the srcs attribute is a label list. Fortunately, the attr_as_label_list function is already forgiving enough to handle that gracefully, even without the upcoming changes in #140. Still we have to be careful here.
Also, I'm a bit worried about custom rules, probably of a different language, that take targets with huge files depsets in their srcs attribute (some roll up or similar) and can handle them properly. With that approach we linearly walk through all files in the srcs attributes of all non-scala targets.
There was a problem hiding this comment.
I'd be hopeful that srcs is a rather forgiving attribute to expand the depset of, since it is most likely not a transitive closure. But in general I agree, we should avoid expanding depset wherever possible.
| def _collect_generic_dependencies(builder, ctx): | ||
| """ | ||
| Records dependency edges for all explicit attributes. The aspect propagates | ||
| information from every attribute, but only standard attributes get serialized as | ||
| typed dependency edges. Rules that keep their dependencies in custom attributes | ||
| (e.g. rules forwarding providers of wrapped targets) would have no edges in the | ||
| model at all, and the IDE could resolve through them only via built jars instead | ||
| of the dependency graph. Implicit attributes are tools and toolchains and are | ||
| covered by their own edge types. | ||
|
|
||
| Has to run before the target info is written, unlike _merge_dependencies, which | ||
| merges the dependencies' own edges and must therefore run after. | ||
| """ | ||
| generic_deps = [] | ||
| for name in dir(ctx.rule.attr): | ||
| if name.startswith("_") or name in _TYPED_DEP_ATTRS: | ||
| continue | ||
| generic_deps.extend([dep for dep in intellij_common.attr_as_label_list(ctx, name) if IntelliJInfo in dep]) | ||
|
|
||
| if generic_deps: | ||
| intellij_info_builder.append_dependencies( | ||
| builder, | ||
| intellij_deps.COMPILE_TIME, | ||
| depset(generic_deps), | ||
| ) |
There was a problem hiding this comment.
Can we move this to the modules that actually need these dependencies? At first glance I see no reason why we we should leak language implementation details to the main aspect.
There was a problem hiding this comment.
but only standard attributes get serialized
Modules can contribute additional attributes that should be serialized. See
intellij-aspect/modules/cc_info.bzl
Line 28 in 927d3b4
There was a problem hiding this comment.
Fair on both. The docstring claim was just wrong since modules already contribute their own attribute lists, fixed in f0f3cad.
On moving it into the modules: the attribute names involved here are by definition ones no module knows about. Our rules forward JavaInfo/PyInfo through attrs like scala_target and partition_target, so placing the collection in a specific module means duplicating the same generic walk in java/python/scala and then deduping across them for rules that forward several languages at once. What I did instead is gate the generic collection to targets that have a module provider attached, which is exactly the set of targets an info file gets written for anyway. The main aspect no longer collects edges for arbitrary rules, and the logic stays in one place. If you'd rather have it as a shared helper that the modules call explicitly, happy to reshape it that way, just say which.
There was a problem hiding this comment.
So if I understand this code correctly it collects targets from ALL rule attributes except in exports and runtime_deps and adds them as compile time dependencies.
There are indeed special edges for toolchains which are handled separately by the aspect, but this snippet also does not walk these edges either. Overall, I fail to see the point of the function, but if you could provide a reproducible example where this function is required I'd be happy to take a look.
Tho, I won't further argue with a chat bot.
There was a problem hiding this comment.
see 70b9fa0 for an example of a test that fails without this function (uses the existing forward fixture)
also, if it's helpful, you can see a project that showcases the issues I was seeing @ JetBrains/hirschgarten#401 (comment)
There was a problem hiding this comment.
Thanks, I see what you are trying to solve here. However, I'm not sure if this information is required by the IDE, since the aspect will walk the edge either way and collect the information of the dependencies. It will just not record this dependency for custom rules in TargetIdeInfo.deps.
I'll try to check if this information is required for IDE support, and if it is I'll most likely address this in a separate PR, since this would be a major change with a potential larger impact.
| # Python files in srcs that are produced by other rules, e.g. a code generator | ||
| # feeding a py_library. They have to be reported as generated sources for the IDE | ||
| # to resolve imports of the generated code. | ||
| generated_sources = [f for f in python_srcs if not f.is_source] |
There was a problem hiding this comment.
I think this probably addressed by #147 as well
There was a problem hiding this comment.
unfortunately not. the CustomRuleTest from this PR fails after rebasing onto #147 if I also remove this logic.
i think it does not work because #147 materializes the files but never populates python_target_info.generated_sources, which is what the plugin's python resolve index uses for navigation.
let me know if you suspect that CustomRuleTest is not actually indicative of actual IDE behavior. i did not yet test a full plugin rebuild visually because that takes some time.
…ze generated JVM sources Fixes three issues that prevent the IDE from resolving generated or custom-rule code: - scala_info detected Scala targets solely by the scala_/thrift_ rule kind prefix. Custom rules compiling Scala sources were invisible to the IDE. Detect targets by the Scala sources they compile, keeping the kind prefix as a fallback for srcjar-only targets. (A provider check is not possible: rules_scala only advertises ScalaInfo since version 7 and loading it would break older versions.) - Generated sources of JVM targets (e.g. a genrule-produced .scala file fed into scala_library) were not part of any output group, so a sync without a build never materialized them and the IDE could not index them. jvm_info now adds generated files in srcs to the sync output group, mirroring how py_info registers PyInfo.transitive_sources. - python_info only fell back to PyInfo/runfiles when the srcs attribute was completely empty. Custom code-generator rules that provide PyInfo but hold generator inputs (templates, protos) in srcs exposed no Python sources at all. Fall back whenever srcs contains no Python files, and also surface the target's own generated Python outputs. Also fixes a latent NameError in scala_info's legacy target.scala handling (provider.java_outputs -> target.scala.java_outputs).
- scala_info: check the rule kind before walking the files in srcs, as the kind check is much cheaper. - jvm_info: pass the DefaultInfo.files depsets of srcs entries to the sync output group as transitives instead of flattening and filtering them. Source files are already on disk, so including them is cheaper than flattening the depsets to filter them out.
Two gaps found by testing against a real workspace: - python_info only reported generated sources for rules without Python srcs. Generated Python files fed into a py_library's srcs (the common code generator pattern) were never reported, so the IDE could not resolve imports of the generated code. Report all non-source Python files in srcs as generated sources. - scrooge_scala_library does not register the source jars of the Scala code it generates from thrift in its JavaInfo, so the IDE decompiles the class jars instead of showing sources. Detect scrooge targets via ScroogeInfo, surface the source jars from ScroogeInfo.aspect_info in java_common, and materialize them in the sync output group.
The source jars of scrooge-generated Scala are already registered in the target's JavaInfo java_outputs and reach java_common via the java module. Only the sync materialization and the ScroogeInfo-based detection are needed.
The aspect propagates information along every attribute (attr_aspects = ['*']) but only serialized dependency edges for the standard attributes collected by the module aspects. Rules that keep their dependencies in custom attributes - e.g. rules that forward providers of wrapped targets - ended up with no edges in the model at all. The IDE could then resolve through such targets only via their built jars; with nothing built (a plain sync), consumers of these targets did not resolve, and inside configuration-transitioned duplicates they never resolved at all. Record generic compile time edges for all explicit attributes before the target info is written. Implicit attributes are tools and toolchains and are covered by their own edge types; exports and runtime_deps keep their precise types.
Review feedback: the main aspect should not collect edges for arbitrary rules, and the docstring wrongly claimed only standard attributes get serialized while modules do contribute their own attribute lists. Gate the generic collection to the set of targets an info file is written for, and fix the docstring.
The ScroogeInfo load introduced the first @rules_scala load in the module, which broke every load of scala_info.bzl from the main repository (e.g. the test harness via intellij/testing.bzl). Declare the dependency like the other rule sets. Also extend ForwardTest with the missing repro for generic dependency edge serialization: the existing forward rule keeps its dependency in a custom attribute, and without the edge collection the target's info contains no dependency edges at all, so the model has no connection between the forwarding target and the forwarded library. Fails at 8a7c348, passes with 72f319a.
An edge to a target without a module provider references a target that never gets an info file, so consumers of the model cannot resolve it. On Bazel 7.7 java_binary's launcher attribute made the binary's deps list point at the launcher flag alias, caught by SimpleTest and TopLevelToolchain.
Generated sources are now materialized for all languages by the main aspect via the build output group. Adjust the scala fixture test accordingly.
1da3f83 to
7cbd8de
Compare
Frank Portman (FrankPortman)
changed the title
3 participants
Fixes IDE resolution of generated and provider-forwarded code at the aspect level. Diagnosed against a production monorepo; plugin-side counterparts and a runnable repro live in JetBrains/hirschgarten#401.
scala_info.bzlscala_/thrift_kind prefix; custom rules compiling Scala get noscala_target_infotarget.scalahandlingscala_info.bzlScroogeInfo, materializeScroogeInfo.aspect_info.src_jarsin the sync output grouppython_info.bzlsrcsis completely empty; codegen rules with template/proto inputs insrcsexpose no Python at allsrcscontains no Python files, surface the rule's own generated outputspython_info.bzlgenerated_sourcesnever reported for generated Python inside a py_library'ssrcs; the plugin resolve index has nothing to index. Not covered by #147, which materializes files but does not populate the field:CustomRuleTestfails on current main, passes heresrcsas generated sourcesintellij/aspect.bzlForwardTestedge assertion on the existingforwardfixture fails without this. JetBrains/hirschgarten@5838c7bb (BAZEL-3364) also relies on these edges for provider-forwarded librariesMODULE.bazel@rules_scalaload in the module and breaks loadingscala_info.bzlfrom the main repositoryFixture tests fail before and pass after each change:
python/customRule+CustomRuleTest,scala/generated+GeneratedSourcesTest,ForwardTestedge assertion. Full//testing/tests/{scala,python,java,kotlin}green locally (macOS arm64), branch is rebased onto #147.Also fully tested alongside JetBrains/hirschgarten#401 inside my IDE.