Implementation and spec: clarify and implemen t common return type classes #28110

DanilaFe · 2025-12-02T00:43:15Z

This PR fixes issues with the spec and implements a missing behavior, thereby closes #14765.

Throughout this, I'll use the following four classes:

class Grandparent {}
class Parent : Grandparent {}
class Child1 : Parent{}
class Child2 : Parent {}

To start with, our spec seems to intend to allow the return type inference when one branch returns Child1 and one branch returns Child2. However, to Michael's surprise in the original motivator issue, today, this doesn't occur.

I believe the spec intends for this to work because it's stated in terms of there existing a "common type", which does exist for Child1 and Child2: that type is Parent. However, today, the Chapel compiler only computes the return type by checking if return types from all other branches can be coerced to the return type of one branch. Neither Child1 nor Child2 is a subtype of the other, so this fails.

However, I think the spec formulation is wrong, because it asks for there to be a unique type to which all branches are coercible. This is a very strong condition. For instance, it disallows inference in both of the following cases:

branch 1 returns Child1 and branch 2 returns Child2. There are two types to which Child1 and Child2 are both coercible: Parent and Grandparent. Thus, the uniqueness conditin is not met, and the return type according to spec should be an error. This seems needlessly restrictive.
branch 1 returns Child1, branch 2 returns Parent. Once again, both Parent and Grandparent are valid types to which both branches' returns can be coerced. So, according to spec, this should be disallowed. This one is particularly heinous because production compiler allows it.

Both of these deductions seem to go against the spirit of the spec, which I would say allows both conversions. The key insight is that although Parent and Grandparent are both valid types, Parent is more specific. Specifically, Parent can be coerced to Grandparent, but not the other way around. Thus, intuitively, Parent makes more sense.

I adjust a spec with this as an updated condition (that there needn't be a unique target type, but that if there are multiple target types, one needs to be convertable to all the others). This is, by the way, a classic coproduct construction. To clarify the rather dense definition, I add an example.

Not only is the behavior absent from the spec, but it's also not implemented. So, in this PR, before returning an error for failing to compute a return type by normal (all-branches-coercible-to-one) means, I add an additional procedure to compute the common class type where possible. At this time, I don't allow coercions to object to allow cases like accidentally returning unrelated classes and the return types unifying to object.

Testing

paratest

Signed-off-by: Danila Fedorin <[email protected]>

jabraham17

In general, this looks good.

But given that this modifies the spec in a significant way, this to me goes beyond just being a bug fix. At a minimum, I think this needs to be discussed with the wider community. It also may break "stability", in the sense that 1 program may compile with one version of the compiler but not another in the same edition. If this is just a bug fix, not a problem. But this feels like more than that (changing the specs meaning).

jabraham17 · 2025-12-02T16:18:35Z

doc/rst/language/spec/procedures.rst

+exist one type such that an implicit conversion is allowed
+between every other type and that type, and such that it can be implicitly
+converted into any other type that satisfies the previous condition. That
+type becomes the inferred return type. If the above requirements are not
+satisfied, it is an error.
+
+   *Example (infer-parent-type-from-classes.chpl)*.
+
+   In this example, two classes ``Child1`` and ``Child2`` inherit from
+   a parent class ``Parent``, which in turn inherits from the class


consider also adding an example that doesn't use classes

jabraham17 · 2025-12-02T16:19:11Z

doc/rst/language/spec/procedures.rst

+exist one type such that an implicit conversion is allowed
+between every other type and that type, and such that it can be implicitly
+converted into any other type that satisfies the previous condition. That
+type becomes the inferred return type. If the above requirements are not
+satisfied, it is an error.


please also make clear the stipulation about no coercion to RootClass

jabraham17 · 2025-12-02T16:22:32Z

test/functions/return-type-class-hierarchy/Classes.chpl

a few nits about the tests

please add some unmanaged test cases

please add some more test cases like the one in the spec with grandparents

jabraham17 · 2025-12-02T16:22:59Z

compiler/resolution/resolveFunction.cpp

 }

+// Helper function to find a common parent type for class hierarchies.
+// Returns NULL if no common parent can be found.


nit, returns nullptr not NULL

jabraham17 · 2025-12-02T16:23:42Z

compiler/resolution/resolveFunction.cpp

+      // parent type for class hierarchies (addresses issue #14765).
+      if (retType == dtUnknown && retTypes.n > 1) {
+        Type* commonParent = findCommonParentForClasses(retTypes, retSymbols, fn);
+        if (commonParent != NULL) {


nit, nullptr not NULL

jabraham17 · 2025-12-02T16:25:55Z

compiler/resolution/resolveFunction.cpp

+  // Track the decorator (managed/borrowed/unmanaged) and nilability.
+  // If decorators don't match and one of them is borrowed, guess borrowed.
+  chpl::optional<ClassTypeDecoratorEnum> commonDecorator;
+  bool anyNilable = false, managerMismatch = false;


this worries me somewhat.

proc foo(x) { if x then return new shared ... else return new owned ...; }

The return type should either be owned or shared, not borrowed. Returning borrowed will cause memory issues (borrowed class to an owned/shared that no longer exists). Similarly,

proc foo(x) { if x then return myGlobalBorrowedClass; else return new owned ...; }

If the return type is borrowed, in the else branch case you get the same memory issue

In the first case the return is not computed. Borrowed is only an option if one of the returns is borrowed.

You also don't get memory issues because of lifetime checking. So if you mix new shared and borrowed, you get an error. That's why my tests create globals variables - they have to.

In the first case the return is not computed. Borrowed is only an option if one of the returns is borrowed.

Can you add tests of this?

You also don't get memory issues because of lifetime checking

Yeah I agree you would get lifetime checking errors, but it seems a little weird for the compiler to let you do that. Infering the return type to be borrowed when I return an owned because another branch returns an owned is surprising behavior to me (even if the global case works)

Yeah I agree you would get lifetime checking errors, but it seems a little weird for the compiler to let you do that.

It might be surprising, but it's part of the (original and updated) spec.

jabraham17 · 2025-12-02T16:29:00Z

compiler/resolution/resolveFunction.cpp

+  AggregateType* seenManager = nullptr;
+
+  // Extract canonical class types (strip decorators)
+  Vec<AggregateType*> canonicalClasses;


consider a small vec for this

jabraham17 · 2025-12-02T16:31:31Z

compiler/resolution/resolveFunction.cpp

+  INT_ASSERT(canonicalClasses.n == retTypes.n);
+
+  // Build a list of ancestors for the first class
+  Vec<AggregateType*> ancestors;


nit, consider smallvec

jabraham17 · 2025-12-02T16:32:02Z

compiler/resolution/resolveFunction.cpp

+  AggregateType* current = canonicalClasses.v[0];
+  ancestors.add(current);
+
+  // Walk up the parent chain
+  while (current->dispatchParents.n > 0) {
+    current = current->dispatchParents.v[0];
+    if (current == dtObject) break;
+    ancestors.add(current);
+  }


im surprised there is no existing helper for this

DanilaFe · 2025-12-02T17:51:12Z

In terms of stability, this is not a breaking change. It enables more patterns to work. It specifically modifies code that previously was guaranteed to emit a compiler error and exit. Historically we have been okay with such changes.

My changes to the spec do not change the semantics encoded therein. Rather, they fix an error in the spec. This is true unless you consider it an error for Child1 to coerce to Parent when there is a Grandparent present (the old spec considers this a bug). The spec always claimed it to be possible to coerce children to each other -- see the original formulation. This is explicitly allowed in the spec and not implemented in the compiler.

jabraham17 · 2025-12-03T19:00:51Z

I am not saying this is a breaking change, but there is an important distinction. If this was the intended behavior all along according to the spec, then this is a bug fix and I have no concerns. But if the spec changes (i.e. the language changes), then in compiler version A a program may not compile but in compiler version B it will. This breaks editions, because I can no longer guarantee my code will work within a given edition (barring bug fixes).

My changes to the spec do not change the semantics encoded therein. Rather, they fix an error in the spec.

Thats not how I read it. The statement "Thus, the uniqueness condition is not met, and the return type according to spec should be an error. This seems needlessly restrictive.". Per this statement, you are relaxing the spec for what determines the return type, which is a language change

DanilaFe added 3 commits December 1, 2025 16:23

Adjust spec with return type narrowing

0e76493

Signed-off-by: Danila Fedorin <[email protected]>

Add a procedure for finding the common type to return

b70a25e

Signed-off-by: Danila Fedorin <[email protected]>

Add tests

a706b9c

Signed-off-by: Danila Fedorin <[email protected]>

jabraham17 self-requested a review December 2, 2025 00:45

jabraham17 requested changes Dec 2, 2025

View reviewed changes

DanilaFe added the post-release label Dec 12, 2025

DanilaFe added stat: Needs Design Review and removed post-release labels Jan 7, 2026

Implementation and spec: clarify and implemen t common return type classes #28110

Are you sure you want to change the base?

Implementation and spec: clarify and implemen t common return type classes #28110

Conversation

DanilaFe commented Dec 2, 2025

Testing

Uh oh!

jabraham17 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanilaFe Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanilaFe commented Dec 2, 2025

Uh oh!

jabraham17 commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DanilaFe Dec 2, 2025 •

edited

Loading