Copilot commented on code in PR #6785:
URL:
https://github.com/apache/incubator-kie-drools/pull/6785#discussion_r3591602521
##########
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/declaredtype/DescrTypeDefinition.java:
##########
@@ -165,13 +180,95 @@ private static Optional<TypeDeclarationDescr>
getSuperType(TypeDeclarationDescr
@Override
public List<FieldDefinition> findInheritedDeclaredFields() {
- List<FieldDefinition> fields = findInheritedDeclaredFields(new
ArrayList<>(), getSuperType(typeDeclarationDescr, packageDescr));
- if (fields.isEmpty()) {
- abstractClass.ifPresent(superClass ->
fields.addAll(inheritedFieldsFromSuperClass(superClass)));
+ return inheritedFieldsOf(typeDeclarationDescr, packageDescr,
typeResolver);
+ }
+
+ /**
+ * Computes the inherited (supertype) fields of a declared type,
base-first, following the
+ * supertype chain across packages and, at the end of the chain, into a
resolvable Java
+ * superclass. This covers a declared type extending another declared type
in the same OR a
+ * different package, and a declared type (possibly several levels up)
extending a Java class on
+ * the classpath. A field-less re-declaration of a classpath class
("declare X end") does not
+ * hide that class's {@link Position} fields. See
+ * DeclaredTypesTest#testExtendPojoInheritedFieldsConstructor and the
cross-package inheritance
+ * cases.
+ */
+ private List<FieldDefinition> inheritedFieldsOf(TypeDeclarationDescr td,
PackageDescr pkgDescr, TypeResolver resolver) {
+ List<FieldDefinition> fields = new ArrayList<>();
+ String superName = td.getSuperTypeName();
+ if (superName == null) {
+ return fields;
+ }
+
+ Optional<DeclaredSuperType> declaredSuper =
findDeclaredSuperType(superName, pkgDescr);
+ if (declaredSuper.isPresent()) {
+ DeclaredSuperType st = declaredSuper.get();
+ // the super's own inherited fields first, then the super's own
declared fields
+ fields.addAll(inheritedFieldsOf(st.descr, st.packageDescr,
st.resolver));
+
st.descr.getFields().values().stream().map(DescrFieldDefinition::new).forEach(fields::add);
+ if (!fields.isEmpty()) {
+ return fields;
+ }
+ // The declared supertype chain contributed no fields: it may be a
field-less
+ // re-declaration of a Java class on the classpath ("declare X
end" over an imported
+ // class), so fall through to that class's @Position fields.
}
+
+ // The supertype is (or re-declares) a Java class. Resolve it in the
declaring package's
+ // scope (its imports) and contribute its @Position fields.
+ resolver.resolveType(superName)
+ .filter(c -> !c.isInterface())
+ .ifPresent(c ->
fields.addAll(inheritedFieldsFromSuperClass(c)));
return fields;
}
+ /**
+ * Locates a declared supertype by simple name, searching the current
package first then every
+ * other package in the build, returning the descriptor together with its
declaring package's
+ * descriptor and type resolver (needed to resolve that package's own
supertypes and imports).
+ */
+ private Optional<DeclaredSuperType> findDeclaredSuperType(String
superName, PackageDescr currentPkgDescr) {
+ Optional<TypeDeclarationDescr> inCurrent =
currentPkgDescr.getTypeDeclarations().stream()
+ .filter(td -> td.getTypeName().equals(superName))
+ .findFirst();
+ if (inCurrent.isPresent()) {
+ return Optional.of(new DeclaredSuperType(inCurrent.get(),
currentPkgDescr, typeResolver));
+ }
+ if (allPackages != null) {
+ for (PackageDescr pkg : allPackages) {
+ Optional<TypeDeclarationDescr> found =
pkg.getTypeDeclarations().stream()
+ .filter(td -> td.getTypeName().equals(superName))
+ .findFirst();
+ if (found.isPresent()) {
+ return Optional.of(new DeclaredSuperType(found.get(), pkg,
resolverForPackage(pkg.getNamespace())));
+ }
+ }
+ }
+ return Optional.empty();
+ }
Review Comment:
findDeclaredSuperType(...) searches other packages purely by simple name and
returns the first match. If multiple packages declare the same type name (but
the current package imports only one of them), this can resolve the wrong
declared supertype depending on iteration order of allPackages, producing an
incorrect inherited-fields list.
Consider narrowing the cross-package search using the current package’s
explicit imports for that superName (when present), and make the fallback
iteration deterministic (e.g., sort by namespace) so builds don’t depend on
Collection iteration order.
##########
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/declaredtype/POJOGenerator.java:
##########
@@ -64,19 +65,33 @@ public class POJOGenerator implements CompilationPhase {
private InternalKnowledgePackage pkg;
private PackageDescr packageDescr;
private PackageModel packageModel;
+ private final Collection<? extends PackageDescr> allPackages;
+ private final PackageRegistryManager pkgRegistryManager;
private static final List<String> exprAnnotations =
Arrays.asList("duration", "timestamp");
- public POJOGenerator(BuildResultCollector builder,
InternalKnowledgePackage pkg, PackageDescr packageDescr, PackageModel
packageModel) {
+ public POJOGenerator(BuildResultCollector builder,
InternalKnowledgePackage pkg, PackageDescr packageDescr, PackageModel
packageModel,
+ Collection<? extends PackageDescr> allPackages,
PackageRegistryManager pkgRegistryManager) {
this.builder = builder;
this.pkg = pkg;
this.packageDescr = packageDescr;
this.packageModel = packageModel;
+ this.allPackages = allPackages;
+ this.pkgRegistryManager = pkgRegistryManager;
packageModel.addImports(pkg.getTypeResolver().getImports());
}
+ public POJOGenerator(BuildResultCollector builder,
InternalKnowledgePackage pkg, PackageDescr packageDescr, PackageModel
packageModel) {
+ this(builder, pkg, packageDescr, packageModel, null, null);
+ }
Review Comment:
POJOGenerator’s legacy constructors hard-code (allPackages,
pkgRegistryManager) to null, which disables cross-package declared-supertype
resolution. Another multi-package compilation entrypoint
(ExplicitCanonicalModelCompiler) still uses the 4-arg POJOGenerator ctor (see
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/tool/ExplicitCanonicalModelCompiler.java:132-137),
so the #6782 fix won’t apply on that path. Please update that call site to
pass (packages, pkgRegistryManager) like DeclaredTypeCompilationPhase does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
