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

Can records be const? #2337

Closed
srawlins opened this issue Jul 7, 2022 · 7 comments · Fixed by #2413 or #2422
Closed

Can records be const? #2337

srawlins opened this issue Jul 7, 2022 · 7 comments · Fixed by #2413 or #2422
Assignees
Labels
question Further information is requested records Issues related to records.

Comments

@srawlins
Copy link
Member

srawlins commented Jul 7, 2022

The records spec does not currently use the word 'const' or 'constant'. Can a record be const? Something like

const x = 'one';
const y = 'two';

const r = (x, y);

// Separate, for patterns, which has an open TODO on const evaluation of destructuring.
const (d, e) = r;
@srawlins srawlins added question Further information is requested patterns Issues related to pattern matching. labels Jul 7, 2022
@lrhn
Copy link
Member

lrhn commented Jul 7, 2022

I'd say yes.

That only matters for whether the record can be used in a const expression, because records are already immutable and auto-canonicalizing (the way identical is defined on them). Any record which contains constant element expressions is automatically indistinguishable from being constant.
So let's also say that we allow them in const expressions.

@munificent
Copy link
Member

Yeah, I don't see any reason to disallow const records. I'll leave this open as a reminder to update the proposal to include it.

@leafpetersen
Copy link
Member

This should probably get added in soon.

@mit-mit
Copy link
Member

mit-mit commented Aug 16, 2022

@munificent kind reminder to get this into the spec.

@munificent munificent added records Issues related to records. and removed patterns Issues related to pattern matching. labels Aug 19, 2022
@lrhn
Copy link
Member

lrhn commented Aug 19, 2022

I've written up a proposal for constant record behavior (#2413).

I've made some choices in there that might need discussion:

  1. (e1, e2) is a constant expression iff e1 and e2 is.
  2. (e1, e2) is a potentially constant expression iff e1 and e2 is.

That's basically it. The consequences are:

No const

The expression (e1, e2), with constant e1 and e2, can be used as a parameter default value without a leading const.
Our grammar does not allow const (e1, e2), and I chose to not introduce it.

There is no way to request that final (int, int) p = (1, 2); creates a "constant record".
That should not make any difference because the effect of being "constant" is:

  • Deep immutability.
  • Canonicalization.

You get deep immutability anyway, if the field values of the record are deeply immutable. No need to request it.
You can't ensure canonicalization, since we don't promise that records have stable identity anyway.

@eernstg suggested that users might want to ask for canonicalization, in the hope that the record object would be more likely to take the fast path of an identical comparison with another constant record.
Or just to document that the expressions are expected to be constant, so nobody changes (1, 2) to (fib(2), fib(3)).

My personal opinion is that I'd prefer to steer users away from doing identical on records. It's fine to try, but in practice, if your code's performance being acceptable depends on the result of identical on records, you should probably be doing something else.

For the documentation, I'd be happier adding a general const context operator, where const e puts e in a constant context, whether it's a record or (1 + 1).
(Even that, I fear people will start doing const(4 + "abc".length) /* const for speed! */ to trigger constant evaluation for code that our compilers would easily recognize as constant anyway. Still, it does prevent someone from adding a non-constant component to the computation).

No const in initializer lists.

It will allow a constructor like const C(a, b) : pair = (a, b);.
That's the only kind of "object construction" we allow in a constructor list which is not required to be const.

The reason I want to allow this is that it's not actually object creation. It's just structure creation.
It's grouping, not necessarily allocation.

I don't want to allow

class Rect {
  final int top, left, right, bottom;
  const Rect(int top, int left, int bottom, int right) : top = top, bottom = bottom, right = right, left = left;
}

but not

class Rect2 {
 final (int, int) _topLeft, _bottomRight;
 const Rect2(int top, int left, int bottom, int right) : _topLeft = (left, top), _bottomRight = (right, bottom);
 int get top => _topLeft.$1;
 int get bottom => _bottomRight.$1;
 int get right => _bottomRight.$0;
 int get left => _topLeft.$0;
}

It's just a matter of grouping of the internal structure of the object, and I want both to be equally valid.

In short, if both e1 and e2 is constant/potentially-constant, so should (e1, e2) be.

Should destructuring be constant?

By my own argument above, records are just ways to group data.
If we allow

class Rect2.fromPoints((int x, int y) topLeft, (int x, int y) bottomRight) : _topLeft = topLeft, _bottomRight = bottomRight;

shouldn't we then also allow:

class Rect.fromPoints((int x, int y) topLeft, (int x, int y) bottomRight) 
    : top = topLeft.$1, left = topLeft.$0, bottom = bottomRight.$1, right = bottomRight.$0;

That is, invoking .$0 on a constant/potentially constant record should be allowed.
Probably same for named record fields.

That means that a dynamic invocation of someDynamic.foo may be valid in constant and potentially constant context. During constant evaluation, it will be an error unless someDynamic is a record with that field (or a string for someDynamic.length).

WDYT?

@eernstg
Copy link
Member

eernstg commented Aug 19, 2022

My personal opinion is that I'd prefer to steer users away from doing identical on records

Certainly! I would expect the implicitly induced operator == on record types to use identical to provide good performance based on pointer equality in the case where the receiver is boxed (and the compiler would presumably inline == as several separate tests, in the case where the receiver is an unboxed record).

Hopefully this would allow us to lint against identical(e1, e2) in all cases where the type of e1 or e2 is a record type, and it would then only occur in the implicitly induced == implementations.

We could treat structs / value classes the same, and get the same effect.

The other reason why I'm arguing that we might want to allow records to be explicitly const is

to document that the expressions are expected to be constant

that is: to document the intention and to get a compile-time error if its components aren't actually constant.

If we do not ensure any kind of record canonicalization explicitly then I suspect that we'll have it implicitly: How would backends otherwise be able to canonicalize objects with instance variables whose value is a record?

class C {
  final (int, int) pair;
  const C(int i1, int i2): pair = (i1, i2);
}

class D {
  final (C, C) pair;
  const D(C c1, C c2): pair = (c1, c2);
}

If constant expressions yielding an instance of C will have different representations of pair just because records are not canonicalized, then the decision about whether to canonicalize instances of D gets more complex than simply checking pointer identity of each instance variable. So I expect that we will have canonicalized records, because the backends need them.

Another thing is that if we do support const <recordLiteral> in constructor initializer lists, and include the case where some components of the record are only potentially constant (and check each constructor invocation), then we could also support const <listLiteral> and const <setOrMapLiteral> in constructor initializer lists:

class E {
  final List<int> xs;
  final int j;
  const E(int i, this.j): xs = const [1, i]; // OK.
}

void main() {
  C(42, 43); // OK.
  var i = 42;
  C(i, 43); // OK.
  C(42, i + 1); // Compile-time error.
}

This has been requested, e.g., recently in #823 (comment), and it would probably be a quite useful feature if we allow collection literals to be "constant after actual argument substitution".

Should destructuring be constant?

That would be nice, and it fits nicely into #2219.

@lrhn
Copy link
Member

lrhn commented Aug 19, 2022

I would expect the implicitly induced operator == on record types to use identical to provide good performance based on pointer equality in the case where the receiver is boxed

As pointed out elsewhere (last language-team meeting I think), that's not actually a valid optimization. There are objects which are identical, but not == to themselves.
Well, there is an object (singular): NaN.

Still, it means that (1, double.nan) must be un-equal to itself. If it tries to use identical checks to avoid that, it will give a wrong answer in the fast case. (Not what "fail fast" means!)

How would backends otherwise be able to canonicalize objects with instance variables whose value is a record?

Ouch. That is a good question, and a reason why we should perhaps canonicalize records.
Or, alternatively, we introduce the notion of structurally equivalent as:

Values a and b are structurally equivalent iff:

  • Both are reference values and it's the same reference (identical(a, b)), or
  • Both are records, they have the same shape, and for each pair of corresponding fields of a and b, those fields' values are structurally equivalent.

Then we say that we canonicalize objects to the same instance if the values of their instance variables are structurally equivalent. That's something we're willing to check at compile-time, even if we don't do it at runtime.
We don't have to make a constant identical(record1, record2) use structural equivalence.

Basically, we introduce the "recursive identical" functionality in the specification, but do not use it for the identical function (other than as specification that identical(r1, r2) may return true if r1 and r2 are structurally equivalent records).

That's the equivalence we really care about, we just don't want to make identical have to implement it.
We don't talk about canonicalization of records, because we don't canonicalize, but we still have a notion of being indistinguishable which does not include identity.

We could expose that relation, as a static Record.identical(r1, r2) which checks structural equivalence. Then you have to opt in to the more expensive version.

I would not allow const (1, i) in a constructor initializer list. That's not a constant. Also wouldn't allow const [1, i].
We could allow auto [1, i] which is constant in a constant invocation, and not in a non-constant invocation, but I'm wary about having the same code generate different kinds of objects depending on invocation mode.

munificent added a commit that referenced this issue Aug 19, 2022
- Support constant records. Fix #2337.
- Support empty and one-positional-field records. Fix #2386.
- Re-add support for positional field getters Fix #2388.
- Specify the behavior of `toString()`. Fix #2389.
- Disambiguate record types in `on` clauses. Fix #2406.
munificent added a commit that referenced this issue Aug 25, 2022
* Address a bunch of records issues.

- Support constant records. Fix #2337.
- Support empty and one-positional-field records. Fix #2386.
- Re-add support for positional field getters Fix #2388.
- Specify the behavior of `toString()`. Fix #2389.
- Disambiguate record types in `on` clauses. Fix #2406.

* Clarify the iteration order of fields in `==`.

* Copy-edit the sections on const records and canonicalization.

There should be no meaningful changes. I just:

- Fixed some misspellings.
- Used Markdown style consistent with the rest of the doc.
- Re-worded things to, I hope, read a little more naturally.
- Removed the parenthetical on identical() in a const context because
  that felt a little too academic.

* Leave the order that positional fields are checked in == unspecified.

* Clarify that positional fields are not sugar for named fields.

Specify the evaluation order of fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested records Issues related to records.
Projects
None yet
6 participants