Skip to content
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

init= and = with different-type rhs #15838

Closed
mppf opened this issue Jun 15, 2020 · 9 comments
Closed

init= and = with different-type rhs #15838

mppf opened this issue Jun 15, 2020 · 9 comments

Comments

@mppf
Copy link
Member

mppf commented Jun 15, 2020

PR #14956 added checks that either both T.init=(_) and = (T, _) are user-provided or both are compiler-provided. It added this as a check that occurs after resolution. The strategy of making this case an error was discussed in #8065.

However this strategy leaves it unclear what happens for = and init= functions that operate with a different RHS type. Issues #15717 and #15806 ran into this.

This issue is a spin-off from #15806 which already included some discussion.

From #15806, the leading strategies are:

A. Additional error checking

Make it an error if a type T has proc =(ref lhs:T, rhs:T2) but not proc T.init=(rhs:T2) or if it has proc T.init=(rhs:T2) but not proc =(ref lhs: T, rhs:T2) (the argument names and intents are not important here but the types are important).

This would make the patterns users are trying to write in #15717 and #15806 impossible to write for the time being, because neither tuples nor arrays currently use init=. If we wanted to be able to write those patterns specifically, we could make an exception to the error for tuples and arrays. (Then the compiler would run default-init and = at the initialization point instead of copy-initialization).

So then we have two variants of A:

A1. = for tuple or array cannot be overridden by users
A2. Tuples and arrays have an exception to the error message and behave as if an init= existed that ran =

B. Generate init= from =

Change from the strategy in #8065 / #14956 of generating an error to a strategy of generating proc init= from a proc = that is provided and vice versa. (This was brought up in #8065 (comment) and we chose to start with less permissive strategy).

For B., if a type has = but not init=, then the compiler could generate

proc MyType.init=(rhs: OtherType) {
  this.init(); // i.e. default initialize me please
  this = rhs; // call the assignment overload
}

Likewise, if a type has init= but not =, then the compiler could generate

proc =(ref lhs: MyType, rhs: OtherType) {
  var tmp: MyType = rhs; // i.e. copy initialize / run init=
  deinit lhs;
  move lhs, rhs;
}

Since the lhs variable might be on a different locale this also runs into questions about records and move initialization across locales (issue #15676). We could alternatively consider making the compiler only generate init= from = and not the other way around (which would avoid that particular issue). So there are two variants of this option:

B1. Enable generation of init= from = and also generation of = from init=
B2. Enable only generation of init= from =

@lydia-duncan
Copy link
Member

I lean towards A, personally, though I'm not up to date on what's standing between us and init= for arrays and tuples. My order is probably A1, B1, A2, B2.

@mppf
Copy link
Member Author

mppf commented Oct 14, 2020

I am thinking about a case with a record that allows copy-initialization from int by a function like proc R.init=(arg: int) and proc =(ref lhs: R, rhs: int). What if the record also wishes to allow initializing int from a record? (This is relevant for #16554 ). Presumably they would write proc =(ref lhs: int, rhs: R).

In that event, would an init= be required for int ? It is not a record so init= isn't allowed today. However we might imagine that init= for an int would be the way to provide for copy-initialization from another type, as in

var x: int = myRecordWrappedInt;

Some of the alternatives (e.g. Option 3 in this comment #15806 (comment) ) would generalize smoothly to non-record types even if init= is not available.

That leads to two general directions for the case that the LHS is not a record:

  1. Such types can override = to allow both assignment and copy-initialization from another type
  2. Extend init= to non-record types and such types can override init= to allow copy-initialization and = to allow assignment from another type

Let's consider what these imply if in #16576 we decide that in intent functions can allow implicit conversions as part of in intent handling.

  • Suppose we chose 1. That also mean that, for non-record types, a = overload with different types allows implicit conversions
  • In contrast, if we choose 2 above, that would only allow such automatic conversions for types with init= with a different argument type.

What about the impact on out intent? Suppose that in #16582 we decide that out intent functions can allow implicit conversions. For records, we could have it check for either init= or = for any of A1/A2/B1/B2 proposed in this issue. But, for something converting from a record to an int with an out argument:

  • if we chose 1. above, we allow implicit conversions based upon = overloads
  • vs. if we choose 2 above, init= is the way to opt into implicit conversions

@bradcray
Copy link
Member

I'd generally be open to extending init= to other value types than records.

I was going to write that approach 1 might also have value in terms of simplicity in the sense that the distinction between initializing a scalar value and assigning it is pretty minimal compared to other types (e.g., there's no need to distinguish between creating vs. replacing some existing dynamically allocated state in the two cases). However, I feel nervous about your observation that supplying a = would allow implicit conversions / coercions. That seems like opening up too big of a pit to step into to me.

@mppf
Copy link
Member Author

mppf commented Nov 17, 2020

At the moment, I am not thinking that init= or = should allow implicit conversions for in formal arguments (due to a desire to avoid cycles - my current thinking is described in issue #16729).

As a result I think that approach 1 in the previous comment is reasonable.

@mppf
Copy link
Member Author

mppf commented Nov 19, 2020

I am currently thinking that the compiler would be allowed to implement split-init with a cast function of some kind rather than just init= - see #16732 (comment) . That would imply that we would allow a relevant cast function in addition to a cross-type init= in approach A; in approach B it might mean that we generate the relevant cast function from =.

@mppf
Copy link
Member Author

mppf commented Feb 5, 2021

I am looking at implementing A1 and additionally requiring a cast function exists if = or init= can convert between 2 types.

Some things I observed in initial testing:

I added casts for these cases:

  • sync,single,atomic variables supported init= to create e.g. sync int from int but not cast
  • list has an init= from a RHS range but not a cast to do the same thing. (I added one)

I adjusted the checking to work around these cases:

  • there isn't currently a cast from iterator to array (even though we have "initCopy" that does this)
  • we have assignment like iterator = array but it doesn't make sense to have cast from array to iterator
  • we don't have cast supported between different array types even thought we have =. Likewise we don't have a cast from e.g. 1 to array of int.

@mppf
Copy link
Member Author

mppf commented Feb 5, 2021

PR #17092 is close to passing testing and demonstrates the changes that were required to module/test code.
The cast function added is always boilerplate (since it pretty much just calls an existing init= in a prescribed way). It would be nice to not require this cast but it is something we can relax in the future.

@mppf
Copy link
Member Author

mppf commented Feb 8, 2021

PR #17092 now passes testing.

mppf added a commit that referenced this issue Feb 11, 2021
Add compilation error for missing implied conversions

This is philisophically a continuation of PR #14956.  Issue #15838
contains design discussion.

This PR adds compilation errors according to an idea that conversions
between types come in 4 levels. The idea is that a programmer should be
able to access any of these 4 levels, but the higher levels imply the
lower ones.


 * level 4 (implicit conversion) implies 3,2,1 (assign, initialize, cast)
 * level 3 (assign) implies 2,1 (initialize, cast)
 * level 2 (initialize) implies 1 (cast)
 * level 1 (cast) doesn't imply any of the others

To support that idea, this PR adds errors for
 * no `init=` between two types when `=` is present
 * no cast between two types when `init=` is present.

For now this PR uses the existing `proc _cast(type t: toType, other:
fromType)` syntax to declare the casts. That is so that it does not
interfere ongoing changes to operators.

Additionally, because it's not currently possible to declare tertiary
initializers or initializers on `extern` types, this PR leaves out the
missing `init=` error for these types for now.

For Cray/chapel-private#1626

Reviewed by @lydia-duncan - thanks!

- [x] full local futures testing
@mppf
Copy link
Member Author

mppf commented Feb 19, 2021

PR #17092 implements A1 and is merged.

In the future we might decide to relax this constraint. Issue #17200 asks about doing more automatic inference in this case.

@mppf mppf closed this as completed Feb 19, 2021
mppf added a commit that referenced this issue Mar 4, 2021
Update spec to describe cast operator, requirements on other conversions

This PR updates the conversions chapter of the language specification and makes related changes.

For Cray/chapel-private#1751 and Cray/chapel-private#1752.

Follow-up to #15838  #17092 #17146.

Future work:
 * It might not be the clearest to describe assignment or `init=` between
   types as a kind of conversion. Maybe we should just call these
   assignment between different types or copy initialization between
   different types.
 * The division in the exposition between subtype conversions, generic
   type conversions, and class conversions is not a very satisfying
   division and seems to have a lot of overlap. Perhaps the subtype
   concept could be described separately, as a concept, and then the
   sections can consider conversion to a generic type or class
   conversions.

Reviewed by @vasslitvinov - thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants