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

Chapel 2.0: Is the interpretation of R for a generic record/class type that's not fully defaulted correct? #18214

Closed
bradcray opened this issue Aug 12, 2021 · 12 comments

Comments

@bradcray
Copy link
Member

This is something that I seem to have internalized incorrectly, which makes me want to check that it was by design rather than by accident before declaring Chapel 2.0:

If a record R is generic, but all its generic fields have defaults, as here:

record R {
  type t = int;
  param rank: int = 2;
}

then we interpret R as being R(int, 2) when used as a type constraint. The rationale here was to permit a user to create their own int-style type as a record, in the sense that it would have a default (e.g., int(64)) that could be made available for convenience as a shorthand (e.g., int), while still supporting explicit variants (e.g., R(real, 2), R(int, 3)). If someone wants to indicate a generic R, they should use R(?) or R(?t, ?rank) or the like.

I really like the above and am not personally second-guessing it. Note that our built-in range type currently is considered to be fully defaulted (int indices, bounded, non-stridable), which means that declarations like var r: range; are legal, but also that var r: range = 1..n by 2; generates an error.

Now, if R is generic, but none of its generic fields have default, such as:

record R {
  type t;
  param rank: int;
}

then R is interpreted as being the same as R(?) (rather than, say, not being a legal type specifier because it wasn't complete in and of itself like in the previous case).

I think that this is probably right and am definitely open to it ("why make people type more than they have to?")—and would prefer not to change things of this magnitude at this point. But I wanted to be sure since I had mis-internalized this as not being a well-defined type-constraint. Note that our built-in domain type—while not a typical record—follows this behavior in that domain is treated as "any domain".

Finally, if its generic, but some of its generic fields are lacking a default, such as:

record R {
  type t;
  param rank: int = 2;
}

then R is again interpreted as being the same as R(?) (rather than, say, being interpreted as R(?t, 2)).

This case is perhaps slightly less comfortable to me since it feels like it doesn't quite sit cleanly between the previous two cases (the defaulted case is not a constraint; yet it's no different than nothing being a default). But if others felt strongly that this was the right thing, I'd be happy for an excuse not to change it. It just hasn't landed neatly in my mind.

#18213 adds some tests that exercise these possibilities.

@bradcray
Copy link
Member Author

@e-kayrakli , @lydia-duncan , @gbtitus : Tagging you on this since you were the most involved in this part of the conversation in today's range module review meeting (though I'd obviously be interested in comments or thoughts from anyone, including @mppf who was on vacation today).

@e-kayrakli
Copy link
Contributor

I agree that parts of the status quo is not perfectly clean/consistent. But I think all the three scenarios have easy-to-explain rules that improve usability of the language. I am happy with where we are in general.

@mppf
Copy link
Member

mppf commented Aug 23, 2021

This issue brings up 3 behaivors:

  1. generic fields with defaults, R means the same as R(int, 2) e.g.. I don't think we should change this (although in the past I had argued it should be R(?) and IIRC we tried that and decided it wasn't good)
  2. generic with no defaults, R means the same as R(?) - IMO this is definitely seems like the right behavior
  3. generic fields where some have defaults and others do not - at first glance, I think that having it interpreted as R(?t, 2) (or R(rank=2, ?)) is the right thing long-term but in the near term I'm not sure it will be easy to have it work correctly since it is effectively a kind of partial instantiation and we have some worrying bugs there (partial instantiation equivalence? #14286).

In the near term I think it would be reasonable to have a --warn-unstable warning for scenario 3. For one thing, that would allow us to continue to debate whether scenario 3 should be R(?t, 2) or R(rank=2, ?).

@mppf
Copy link
Member

mppf commented Aug 23, 2021

FWIW I also think that the question about case 3 is related to the purpose of the field defaults. If the purpose is only to "make writing initializers easier" then I think it's a reasonable argument that the current behavior is reasonable.

@vasslitvinov
Copy link
Member

I find Michael's 3-behavior analysis above to be a reasonable approach.

I thought of an analogy with a function where some or all of formal arguments have defaults. However that did not help me gain clarity on this issue.

@bradcray
Copy link
Member Author

bradcray commented Sep 1, 2021

If the purpose is only to "make writing initializers easier" then I think it's a reasonable argument that the current behavior is reasonable.

I think for me it's probably primarily that.

@mppf
Copy link
Member

mppf commented Aug 16, 2022

Noting this issue is related to #19120

@mppf
Copy link
Member

mppf commented Jan 6, 2023

Responding to these bits --

Finally, if its generic, but some of its generic fields are lacking a default, such as:

record R {
  type t;
  param rank: int = 2;
}

then R is again interpreted as being the same as R(?) (rather than, say, being interpreted as R(?t, 2)).

3. generic fields where some have defaults and others do not - at first glance, I think that having it interpreted as R(?t, 2) (or R(rank=2, ?)) is the right thing long-term but in the near term I'm not sure it will be easy to have it work correctly since it is effectively a kind of partial instantiation and we have some worrying bugs there (

In the near term I think it would be reasonable to have a --warn-unstable warning for scenario 3. For one thing, that would allow us to continue to debate whether scenario 3 should be R(?t, 2) or R(rank=2, ?).

I think I had misunderstood what the current behavior is. I don't think it's quite accurate to describe it as ignoring that some fields have defaults.

What I am seeing:

  • the record B { type t = int; param rank: int;} is not the same as either B(?) or the partial instantiation B(t=int).
  • B(int) is a generic type B(int, ?) (but this would be an error with a partial instantiation)
  • B(3) results in an error (but this would work with a partial instantiation)
  • B(rank=3) is the concrete type B(int, 3) (but this would be a generic type without the defaults).

Anyway, I think the current behavior appears to be that the defaults are used in the type constructor. I'm comfortable with this behavior, under the assumption that we keep the current interpretation for what defaults mean on type/param fields (i.e. things would be different if we went for #21075).

record A {
  type t;
  param rank: int = 2;
}
type Aint = A(int);
compilerWarning(Aint:string); // A(int(64),2)
type Aint_n = A(t=int);
compilerWarning(Aint_n:string); // A(int(64),2)
type Aint3 = A(int, 3);
compilerWarning(Aint3:string); // A(int(64),3)
//type Aint2x = Aint(2); error: Aint is not generic
//compilerWarning(Aint2x:string);

record B {
  type t = int;
  param rank: int;
}

type Bint = B(int); // B(int, ?)
compilerWarning(Bint:string);
type Bint_n = B(t=int); // B(int, ?)
compilerWarning(Bint_n:string);
type Bint3 = Bint(3); // B(int, 3)
compilerWarning(Bint3:string);
type B3_n = B(rank=3); // B(int, 3)
compilerWarning(B3_n:string);
//type B3 = B(3); // error: invalid type specifier
//compilerWarning(B3:string);

// compare with a partial instantiation
record G {
  type t;
  param rank;
}

type BB = C(t=int);
type BB3 = BB(3);
compilerWarning(BB3:string);

@mppf
Copy link
Member

mppf commented Jul 11, 2023

In looking at https://github.com/Cray/chapel-private/issues/5033 I ran into this again. I created #22714 for the case I was immediately concerned with.

@mppf
Copy link
Member

mppf commented Aug 24, 2023

Finally, if its generic, but some of its generic fields are lacking a default, such as:

record R {
  type t;
  param rank: int = 2;
}
type t = R;

// below can be run to measure it
if t.rank == ? then
  writeln(".rank is uninstantiated");
else
  writeln(".rank: ", t.rank:string);

Here the question is, what is t? It is currently R(?) but it could mean R(rank=2, ?). I think it would be possible to do either.

(I'm writing this with the type variable because even if we say R on its own means R(rank=2, ?), we would still allow a call like R(t=real, rank=3), just as we would for the fully-defaulted case. So, here we are only talking about what R means on its own outside of a type construction call).

I'm not seeing a clear answer in our discussion here, and my other work uncovered some odd behavior in this case in #22714. As a result, I'm recommending that we make types that have some generic fields with defaults and others without as unstable for the upcoming release.

@lydia-duncan
Copy link
Member

I think we've done all we're going to do with this and feel as though the language is in a better state than it was when the issue was opened. Is it okay to close this issue, or is there more we feel we should think about (potentially even for 3.0?)

@bradcray
Copy link
Member Author

bradcray commented Apr 3, 2024

I agree, thanks for pointing this out Lydia. I feel much better about where we are now after our recent discussions and @mppf's improvements.

@bradcray bradcray closed this as completed Apr 3, 2024
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

5 participants