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

Permit authors of domains and arrays (or general records?) to reason about their const-ness #10911

Open
bradcray opened this issue Aug 28, 2018 · 11 comments

Comments

@bradcray
Copy link
Member

bradcray commented Aug 28, 2018

As a Chapel developer, when I'm implementing a domain, there are sometimes benefits that could be obtained by being able to query whether the domain is constant (const) or not. This feature request asks for a means for a domain author to be able to make param-style decisions based on whether the domain is constant or not. While the motivation for this feature is specific to domains, perhaps the capability should be made available to all record authors (?).

As an example, domains currently maintain a list of all the arrays declared over them so that, should the domain be resized, the arrays can be reallocated as well. Yet, a const domain can't be re-assigned, so this list tends to be unnecessary. However, we currently pay this cost for all domains regardless of whether they're constant or not since we have no way of distinguishing between the two cases in module code (where this logic is implemented). As we've seen, this sometimes results in a ridiculous amount of overhead, as @ronawho measured in issue #9414.

As a second example, the following program currently works and permits D to be modified indirectly even though it is declared to be constant:

const D	= {1..10};

var A: [D] real;

A.push_back(4.2);

writeln(D);

It seems that this should be an error, and if @daviditen, as the author of .push_back(), were able to query whether or not A's domain was constant, [t]he[y] could trigger a (compile-time) error for this case.

As a third example, issue #7000 points out a way in which a const array can be illegally modified.

@bradcray bradcray changed the title Permit authors of domains (or general records?) to query their const-ness Permit authors of domains (or general records?) to reason about their const-ness Aug 28, 2018
@daviditen
Copy link
Member

The associative array set operators are another thing that could benefit from this in the same way as .push_back().

const D1 = {"one", "two", "three"},
      D2 = {"three", "four"};
var A: [D1] int,
    B: [D2] int;

A |= B;
writeln(D1);

@ben-albrecht
Copy link
Member

Yet another example -- the current behavior can lead to surprising results when using the following pattern to support empty array default arguments as a work-around for #11427:

const emptyArray: [1..0] int;

proc f(A: [] int = emptyArray) {
  A.push_back(1);
}

writeln(emptyArray); // []

f();
f();

writeln(emptyArray); // [1, 1]

In fact, this pattern is currently being used in the Spawn module for the default env: string arguments. Fortunately, env never gets modified within the spawn function.

@bradcray bradcray changed the title Permit authors of domains (or general records?) to reason about their const-ness Permit authors of domains and arrays (or general records?) to reason about their const-ness Oct 26, 2018
@bradcray
Copy link
Member Author

When I think about this issue (which I've been doing a bit obsessively lately), it's difficult for me to think of approaches that don't rely on an explicit or implicit boolean param field that would indicate const-ness and bifurcate these record types into const vs. non-const generic variants. I'm curious whether other @chapel-lang/core-contributors have other thoughts about approaches?

@mppf
Copy link
Member

mppf commented Oct 26, 2018

@bradcray - if we didn't want to change the type, we could make it a non-param bool field in a domain, and then related module code could reason about it at run-time.

@bradcray
Copy link
Member Author

The reason I was thinking "param" was that it seems that it'd make it easier to generate compile-time error messages for the cases that were related to semantic violations rather than simply performance overheads.

@benharsh
Copy link
Member

benharsh commented Oct 26, 2018

If we want third-party domain maps to be able to check constness, then I think this is a property of the Base* classes and not the record wrappers, right? From the domain map perspective there is currently no access to the _array or _domain records.

I think there's a bigger question that informs the response to this issue: do we intend to allow modification of an array's domain through an array? We certainly allow it today, but some (myself included) have expressed concerns about the Vector operations on DefaultRectangular. There is a similar issue with DefaultAssociative, but that hasn't been as prominent an issue as the Vector ops.

If we decide not to allow such modifications, can we actually enforce it? Domain map authors can implement their own custom methods on their arrays and such methods could modify the domain if they want. The presence of a param constant doesn't require anyone to check anything either, so I think we'd just be giving domain map authors an opt-in mechanism for better error checking.

So it seems like we're really establishing a convention. Maybe the convention should be that the domain-mutable-via-array types shouldn't actually be proper array types? As has been said before, constrained generics ought to allow users to indicate desires for certain array behaviors (iteration, indexing).

I think a runtime property for constness would be enough to avoid the performance issues.


There's still the issue of addressing const-ness within an array or domain. For example, how could we allow compile-time checking for a custom third-party dmap method?

const D = {1..10} dmapped MagicDist();
D.mutatingMethod();

Today we implement this with forwarding. When resolving such calls for arrays and domains, could we treat the result of _value with the same constness as the record wrapper? For example:

// D.mutatingMethod();
const chpl__temp_value = D._value;
chpl__temp_value.mutatingMethod();

But that depends on the this-intent of classes, and our view on the depth of const.

@mppf
Copy link
Member

mppf commented Oct 29, 2018

But that depends on the this-intent of classes, and our view on the depth of const.

Right, I've brought it up before, but D considers pointers from a const record to be also pointing to const. They report good results with this strategy.

I think for the array types at least, this is the property we want. I'd probably argue that a domain map that implements methods that modify elements when the array is const - or adds indices when the domain is const - is a domain map with a bug.

Anyway, since we're planning to significantly change push_back, I'd like to understand more what we're doing there (in issue #9452) before using it as design justification here.

As an example, domains currently maintain a list of all the arrays declared over them so that, should the domain be resized, the arrays can be reallocated as well. Yet, a const domain can't be re-assigned, so this list tends to be unnecessary.

I think a runtime property for constness would be enough to avoid the performance issues.

I agree about this one.

The reason I was thinking "param" was that it seems that it'd make it easier to generate compile-time error messages for the cases that were related to semantic violations rather than simply performance overheads.

I think whether this is a performance optimization or a correctness-errors type problem depends on how we handle issue #9452.

@bradcray
Copy link
Member Author

bradcray commented Nov 5, 2018

Issue #7000 is another case where inability to tell const-ness (of an array, though, not a domain) is incorrect:

const A = [1, 2, 3];

for a in A do
  a += 1;

writeln(A);

I'm imagining that if we had the ability to query const-ness of an array, we could have the _array record use a where-clause to squash the ref overload in favor of a value-based overload.

Offline, @benharsh points out that we could just do special stuff in the compiler for this case because we own arrays, but my goal would be to give users creating their own ADTs the ability to similarly define what const-ness means for them (where I'm skeptical that simply making the whole record const vs. not would be sufficient for cases like this).

@benharsh
Copy link
Member

benharsh commented Nov 5, 2018

Perhaps we could handle constness on a per-method or per-iterator basis without specializing each one by allowing users to express some link between the this-intent and return-intent. Maybe something like this?

// 'C' behaves like a param bool
// this kind of thing could replace the 'reference to const when const this' pragma
proc const(?C) this(i : idxType) const(C) ref { }

If we could know about this kind of link, then we wouldn't have to instantiate a separate version of 'this' and could enforce const-ness at the callsite?

var A : MyRecord;
A[i] = 5;
// becomes...
// ref temp = A[i];
// temp = 5;

const B : MyRecord;
B[i] = 5;
// becomes...
// ref temp = B[i];
// const ref const_temp = temp;
// const_temp = 5; // error!

Sorta feels like rust lifetimes.

@vasslitvinov
Copy link
Member

Also allow querying param-ness?

proc myproc(param(?P) p) param(P) {.....}

@mppf
Copy link
Member

mppf commented Nov 7, 2018

@benharsh - we already have a pragma that does that, in a manner, pragma "reference to const when const this". The return intent overloading code uses this property in many cases.

Issue #7000 is another case where inability to tell const-ness (of an array, though, not a domain) is incorrect:

I view this one more as showing limitations in our const-checking around iterators and iteration. I would expect the late const checking to be able to catch this case as the language is now; it's just got some gaps for one reason or another.

bradcray added a commit that referenced this issue May 10, 2019
Test concurrent remote changes to a slice's domain

[reviewed by @ronawho]

In recent discussions about slicing optimizations, we've been discussing when it's legal to RVF an array or domain. I'd been wondering whether any slicing domain that can be serialized might be amenable to RVFing (where my current PR #12985 only RVFs privatized domains and arrays); however, Elliot and BHarsh pointed out that in cases where the slicing domain can be concurrently modified (as in this test), it would not be legal to RVF the slicing domain's value. Thus, my updated thinking is that RVFing is legal if the domain is privatized or if it's constant (which, unfortunately, we don't currently have a good way to check... see issue #10911).
bradcray added a commit that referenced this issue Mar 18, 2020
…-nilable

Issue a halt when someone tries to resize an array of non-nilable classes

[reviewed by @mppf]

This PR makes arrays of non-nilable classes safer by generating a halt in the event that the domain is resized.

Making this a compiler error would be better, but is challenging because: (a) a domain doesn't know statically what types of arrays it is storing; (b) the dsiReallocate() routines on given arrays are subject to dynamic dispatch resolution.  What I think we'd really like to do is not support arrays of non-nilable classes that are declared using non-const domains.  However, this is blocked by issue #10911.

This check is also currently a bit weak as it only prevents against arrays of non-nilable classes and not arrays of records with fields that are non-nilable classes and aren't default initialized.  That more general check is currently blocked by #14851 .

Related issues:
* #14936 
* #15094 
* #15233
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

6 participants