Closed mppf closed 1 year ago
vasslitvinov
commented
1 year ago I see a good amount of benefits in switching from ref-maybe-const to const-ref. The API benefits stand out for me. I believe we can address the challenges from this switch.
It would be nice to allow maybe-const in prototype modules. However I do not think it is worth the extra complexity in the implementation -- and in the language spec. Also, we can add it later if we want.
vasslitvinov
commented
1 year ago How impactful would this change be? In a back-of-the-envelope experiment, I switched some? most? ref-maybe-const intents to const-ref intents. Running a paratest over these subdirectories in test/: release studies patterns puzzles constrained-generics, I got 748 successes and 368 failures, so about 1/3rd of all test runs failed. Almost all failures were in test/studies.
How does it affect my thinking from the previous comment? This looks like a change on par with nilable+managed classes w.r.t. magnitude. So, still worth it if we believe in the benefits.
P.S. Perhaps some of these errors will go away when we adjust promotion to work in this new world.
mppf
commented
1 year ago Are we willing to consider changing the default intent for all types to const ? That would drastically simplify the intents table and help with #16988. The main case other than ref-maybe-const discussed above is atomic / sync / single. I can see two potential directions, if we are willing to consider them:
const ref for functions but the default task / forall intent is still refconst they can be mutatedmppf
commented
1 year ago It might feel a little bit like it is coming out of left field, but this topic caused me to think about our default intents behavior over the last few days & that led me to create issue #21499 to ask if we need a new intent. It is relevant for this discussion because we could consider having the new intent be the default intent. However, when applied to cases where ref-maybe-const intent is used today (arrays, primarily), using the new intent as the default intent would mean the same thing as having the default intent for these types be const ref.
bradcray
commented
1 year ago The main case other than ref-maybe-const discussed above is atomic / sync / single.
Are atomic / sync / single really ref-maybe-const? I thought they were ref only, and the spec says this as well (and it's what I think they ought to be).
Specifically, I wouldn't have expected them to need ref-maybe-const support because I think of that as helping with a case where we historically wanted the default to be ref, but may have a const we want to pass in as an actual. Yet a const atomic, sync, or single is pretty meaningless ("It's a type for coordinating between tasks by changing its value, but its value never changes"). Thus, I don't feel there's a big need to worry about helping out a user who creates a const version of them in by default intent and getting an error. I might even go so far as to warn about, or disallow const atomics/syncs/singles to head off the potential such confusion.
Assuming I'm not mistaken, I don't see a problem with having them continue to just be ref—I think it still follows the principle of least surprise, even if it's not const like the others.
bradcray
commented
1 year ago Returning to an earlier Vass comment I've been meaning to get back to:
P.S. Perhaps some of these errors will go away when we adjust promotion to work in this new world.
Vass, when Michael and I were first talking about opening this issue, I convinced myself that promotion shouldn't be a problem. Specifically, for something like:
A = B + C;
D = sin(A);I was thinking of these as shorthand for:
forall (a, b, c) in zip(A, B, C) {
a = b + c;
}
forall (d,a) in zip(D, A) {
d = a;
}such that a const ref intent within the loop wouldn't be a problem since we're only referring to the elements there, not the arrays as a whole. Simultaneously, I was realizing that it'd help with the occasional performance-killing typo that's occurred when someone writes something like:
forall (d, a) in zip(D, A) {
D = A;
}since the compiler would error due to the assignment to D, forcing the user to rewrite as:
forall (d, a) in zip(D, A) with (ref D) {
D = A;
}Could your implementation be casting a tighter net than it should be by inspecting the loop's iterands rather than just its body?
mppf
commented
1 year ago The main case other than ref-maybe-const discussed above is atomic / sync / single.
Are
atomic/sync/singlereally ref-maybe-const? I thought they wererefonly, and the spec says this as well (and it's what I think they ought to be).
They are just ref. I was saying that they are the main outlier from "all default intents are const" beyond the ref-maybe-const cases that this issue focuses on. Even after 11 years, it is still appealing to me to have the default intent for everything be const. I am trying to understand if there is room for proposals in this area or if it is something that we don't have the time/ability to consider changing for 2.0.
vasslitvinov
commented
1 year ago I still like the default intent of const for everything... except atomic/sync/single -- const is so largely useless for those. At a glance, I also like Michael's cvref.
Vass, when Michael and I were first talking about opening this issue, I convinced myself that promotion shouldn't be a problem.
My "promotion is a problem" comes from #5218, which aired this concern. Seems like the problematic case is:
A[I] = 3; // where I is an array of index(A.domain)We can discuss this further. I do not want this to prevent "default intent is const for everything".
Could your implementation be casting a tighter net than it should be by inspecting the loop's iterands rather than just its body?
Absolutely, given that it was a quick experiment. However it may be the case that it accepted most of the promotion cases, especially because the above A[I] = 3 situations are uncommon.
bradcray
commented
1 year ago I was saying that they are the main outlier from "all default intents are const" beyond the ref-maybe-const cases that this issue focuses on.
Got it. I'd still prefer atomic/sync/single to default to the useful value of ref rather than the not-particularly-useful value of const. And I'd rather have the asymmetry be between types (given good rationale like we have here) than between argument passing and parallel intents.
A[I] = 3;
Oh, thanks, I didn't think of that case, and can see why it would be problematic:
forall i in I do
A[i] = 3;That does feel unfortunate, though I share your sentiment Vass that I wouldn't want this to be the reason we didn't consider const as a default intent for arrays (though I also don't know how I'd deal with it—forcing these cases to be written long-hand feels a little unfortunate, particularly when they're guaranteed to be parallel-safe like in this example. I could imagine an argument that:
A[I] = B;should be forced to be written with a forall loop since it could result in non-deterministic results if I contained duplicate indices.
mppf
commented
1 year ago I'm not really understanding why these promotion cases are a potential problem. Isn't what the details compiler does with promotion up to us & not directly part of the language design? If it's working today with ref-maybe-const arguments to the generated promotion wrapper, or something, can't it just keep doing that? Or is it some sort of argument that the language itself will be inconsistent, because promotion means forall and
mppf
commented
1 year ago Got it. I'd still prefer atomic/sync/single to default to the useful value of
refrather than the not-particularly-useful value ofconst. And I'd rather have the asymmetry be between types (given good rationale like we have here) than between argument passing and parallel intents.
But what about:
consider these types similar to class fields, so that even when they are const they can be mutated
I consider this a real option, even though it might seem a bit strange... it would allow us to have the various intent tables to be consistent in all of the ways ("Default intent is always const!" "Task intent is same as formal intent!") but would allow the current patterns to continue. We would basically be saying "In practice, const doesn't mean anything for sync/single/atomic".
I can take this to another issue if there is much more discussion about it -- I asked about it here because I was trying to understand if it is worth creating an issue (vs if the proposal is dead-on-arrival).
vasslitvinov
commented
1 year ago consider these types similar to class fields, so that even when they are const they can be mutated
I am not excited about yet another issue. However I did not follow the idea - @mppf could you explain this proposal in more detail?
bradcray
commented
1 year ago Or is it some sort of argument that the language itself will be inconsistent, because promotion means forall and ?
That's what I was imagining.
consider these types similar to class fields
I don't really like exposing them to the user as though they were classes. Specifically, once we (hopefully) have direct read/write on atomics (https://github.com/chapel-lang/chapel/issues/16237), I wouldn't want:
var x, y: atomic int = 1;
x = y;
y = 2;
writeln(x);to potentially be construed as printing 2 "because atomics are class-like." I also don't want to say "they're like classes, yet not classes because they store their values directly rather than in a distinct (modifiable) object, don't support memory management styles, don't support nilability." Basically, they don't seem sufficiently class-like that the benefit gained by "all intents are const" unity feels worth the confusion around "there is a new flavor of type that's class-like except when it isn't." To me a two-row table for default intents seems much more straightforward.
We would basically be saying "In practice, const doesn't mean anything for sync/single/atomic".
That's not particularly attractive to me since it sounds like it would permit someone were to write:
proc (const x: atomic int) {
x = 2; // or x.write(2);
}which seems "wrong".
vasslitvinov
commented
1 year ago Re promotion: maybe the case A[I] = <whatever> that is a promotion of = over for i in I should, indeed be disallowed, on the grounds that it can be racy? How do we generalize this scenario to see whether it supports our "all default intents are const" approach?
mppf
commented
1 year ago consider these types similar to class fields, so that even when they are const they can be mutated
I wasn't trying to suggest they change in aliasing behavior in any way; just that const means nothing for them (which is how I characterize the situation for e.g. const x: owned MyClass = ...; x.someField = 32;).
Anyway it sounds like this seems too "wrong" to do. (FWIW, it also seems "wrong" to me that the default intent for these is ref, but I think we have bigger fish, so I do not need to pursue it further).
bradcray
commented
1 year ago just that const means nothing for them (which is how I characterize the situation for e.g. const x: owned MyClass = ...; x.someField = 32;).
I don't view const as meaning nothing for classes, just that it says that the variable pointing/referring to the object can't be changed, with no implications for the object itself.
Anyway it sounds like this seems too "wrong" to do. (FWIW, it also seems "wrong" to me that the default intent for these is ref, but I think we have bigger fish, so I do not need to pursue it further).
:+1:
bradcray
commented
1 year ago Something that I've been feeling nervous about recently when considering getting rid of ref-maybe-const, if we did it everywhere that I wanted to capture here. It seems likely we've discussed this before, but I couldn't see it on this issue with a quick look (other than maybe a quick nod to how we got here?):
In the context of task intents, it seems as though we would still want something ref-maybe-const like in order to be able to continue writing loops like:
var A: [1..n] real;
const B: [1..n] real = ...;
forall i in 1..n do
A[i] = 2* B[i];without having to write it as:
forall i in 1..n with (ref A) do
A[i] = 2 * B[i];Specifically, if we decided that the default intent for arrays should be const ref then every loop that modifies a loop in parallel via indexing (which I think is a lot of them) would have to pass the array in by reference to override the default. Meanwhile, we can't say that the default intent is simply ref since B can't be passed by ref since it's const (at least, without analysis of the loop to see that it's not modifying B... which starts to feel similar to the ref-maybe-const analysis we already have). So it feels as though parallel loops would effectively want/need some sort of ref-maybe-const analysis.
Of course, we could also decide to have default argument intents and default task intents for loops differ from one another.
Am I rehashing old ground here?
bradcray
commented
1 year ago Related thought: Does the array's default iterator yield values by ref-maybe-const? I think of it as yielding by ref, but then... how would that work for a const array? Checking the sources, I'm seeing:
pragma "reference to const when const this"which effectively feels a bit like "ref maybe const" in another form (?).
mppf
commented
1 year ago Something that I've been feeling nervous about recently when considering getting rid of ref-maybe-const, if we did it everywhere that I wanted to capture here. It seems likely we've discussed this before, but I couldn't see it on this issue with a quick look
I think we discussed it (or similar issues) on #5218.
To my eyes, it would be pretty good to make the default formal intent for arrays be const ref but the default task/forall intent for arrays ref-maybe const. The reason this is appealing to me is that it would address the concerns in the top post of this issue: Compiler Architecture, Documentation and API Stability, Interfaces. These problems are really about interprocedural analysis. The task/forall intent being ref-maybe-const can be viewed as a relying on a within-function analysis which does not present the same problems.
Does the array's default iterator yield values by ref-maybe-const?
Yes, but this is really just a shorthand for writing a return intent overload. I think it is separable from the main issue here, which is about the default intent for arrays etc. The pragma "reference to const when const this" is activating something other than the default intent; and it is not user-facing and we can replace it with something else (such as a return intent overload) if needed.
bradcray
commented
1 year ago Great, thanks!
it would be pretty good to make the default formal intent for arrays be const ref but the default task/forall intent for arrays ref-maybe const.
I'm definitely open to this as well. Something you said above ("it would allow us to have the various intent tables to be consistent in all of the ways") made me wonder if you wouldn't be. Thanks for clarifying. Two things were making me wonder about these things: (1) mentally ramping up to mind-meld with Jade on it tomorrow; (2) writing documentation for arrays and loops.
mppf
commented
1 year ago To be clear, it would also be OK with me if you needed to write with (ref A) in your example. One nice outcome of requiring that would be that it makes the potential for race conditions clearer even when working with arrays.
bradcray
commented
1 year ago I agree we should consider it. I just worry that the changes required may be too great. But that's why we should consider it now. :)
vasslitvinov
commented
1 year ago One big unknown is how the switch from ref-maybe-const to const-ref will affect promoted expressions, like A[IdxArray] = 3;. (Perhaps Jade can help us here.)
Keeping the default task/forall intent for arrays ref-maybe-const while changing it for the formals is a big inelegance in language design. I would not want to take this route.
jabraham17
commented
1 year ago In a recent discussion (Cray/chapel-private#5093), we decided to deprecate ref-maybe-const for argument intents, this intents on record recievers, and task/forall intents. This should entirely remove ref-maybe-const from the user interface
We are preparing for the Chapel 2.0 release where the language is stable. This issue asks about whether or not we are confident that we want to keep the ref-maybe-const feature as it stands for 2.0.
Background & History
The abstract intents table describes the current situation. The default intent varies between
const in/const ref/refdepending on the type and other details. In particular:atomic,sync, andsingle, the default intent isrefreforconst ref. The compiler choosesrefif the body of the function modifies the array formal.thisformal for a method on a record, the default intent is similarly eitherreforconst ref& compiler choosesrefif the body of the function modifies the formal.This design has evolved over time.
Historically, arrays were in the same category as
sync/single/atomicand the default intent wasref. In 2012, we had some discussion on an email thread about potentially making these cases allconst ref. Then, in 2017, we ran into some unintended consequences when combining default intent functions (such aswriteln) with array element access that uses return intent overload (see issue #5217). In response to those issues, we considered 3 proposals: 1) justconst ref#5218 2) ref-if-modified aka ref-maybe-const #5219 3) specialize-blank #5220 and went with ref-maybe-const. One reason for that is that the just-const-ref proposal ran into some challenges with combining with task intents and promotion.Later, we have heard some feedback from a user that the default intents table was too complicated to remember and we sought to improve the documentation (PR #20700).
One more note -- partly because the default intent for arrays was historically
ref, the compiler allows mutation of array call-expr temporaries (such as withmodifyArray(returnArray())) even though it does not allow such mutation of literals/rvalues in other contexts (see this comment on issue #17195).We have also identified these related issues:
New Problems Identified
This issue discusses a few new problems with ref-maybe-const that, for the most part, have not been brought up previously.
Compiler Architecture
Presently, we are both trying to stabilize the language as well as re-implementing the resolution piece as part of the dyno effort. The current ref-maybe-const design is difficult to implement in full generality without creating a sort of full-program resolution phase (which is the normal mode of operation for the production resolver, but not for the prototype dyno resolver). In the context of the dyno resolver, these problems are particularly apparent with recursive functions.
In the past, we have considered whether or not ref-maybe-const is problematic for separate compilation. I think it is manageable for separate compilation, but at the very least it does add some complexity.
Documentation and API Stability
When writing and publishing a library, it's important to be able to clearly document the API and to be able to offer API stability by being aware of cases where the API changes. The ref-maybe-const intent causes problems for both of these, because, for function signatures using default intent, the function signature alone isn't enough to tell you if you can pass a
constvariable as an argument. In particular, if the function accepts an array argument, then it depends on the body of the function whether the array formal isconst reforref. But, the body of the function is not available in the documentation, and furthermore, somebody trying to understand how to they can use the function shouldn't have to look at the body of the function.It's certainly the case that we could try to improve the tooling in this area. The
chpldocoutput could include results of resolution & indicate the concrete intents. However, that isn't always possible for a generic function, since the intent can vary depending on the type it is instantiated with. Additionally, even if thechpldocoutput did clarify the situation, we would still have the problem that somebody creating a library could easily change one of these intents fromconst reftorefin the process of updating the implementation. They might update the implementation and produce a new minor version without ever realizing that they are changing the API.Interfaces
This one isn't really a new problem since it has its own issue #16988, but it deserves a few more words.
When writing an
interface, we need to be able to write a function signature without a body. So, the ref-maybe-const rule cannot apply because it's not possible to analyze the function body. See also #16988 which asks what array-typed formals should mean in a routine in an interface.Universal Method Call Syntax
One issue we face is how to request the version of a method for a particular interface. If resolved, this strategy can enable interfaces as a solution for special method naming. The way Rust handles it is to use universal method call syntax. With universal method call syntax,
receiver.method(arg)andmethod(receiver, arg)are interchangeable. That means that a specific interface can be requested withSomeInterface.method(receiver, arg). See also More https://github.com/chapel-lang/chapel/issues/19050#issuecomment-1375836834 . Note that we have not discussed unified method call syntax much yet (it probably deserves its own issue).The point here is that if we adopted unified method call syntax, it would seem really weird to have the 1st argument to certain routines have a different default intent from the 2nd argument. It depends on the details of the universal method call syntax how bad this would be. (Can you call a function that isn't declared as a method as a method, like
a.f(b)forproc f(a, b)? If so, does the special receiver intent apply toa?)Possible Approaches
I'll add links to issues for possible approaches here as I write them.
21399