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

[Records] Should records have an asMap method? #2673

Closed
leafpetersen opened this issue Nov 30, 2022 · 8 comments
Closed

[Records] Should records have an asMap method? #2673

leafpetersen opened this issue Nov 30, 2022 · 8 comments
Assignees
Labels
records Issues related to records.

Comments

@leafpetersen
Copy link
Member

In discussion , @rakudrama suggested the possibility of specifying that records provide an asMap method, producing either a Map<String, Object?> or perhaps a Map<Object, Object?> if we prefer to use integers to index the positional fields.

This would make writing generic code over records feasible in more places, e.g. we could make the toString method on records be a method on Record which used the asMap method + iteration.

We could also consider providing an asList method which provided an integer indexed view (perhaps using a canonical order for the named parameters, or else just providing a view on the positional fields).

cc @lrhn @munificent @eernstg @jakemac53 @natebosch @stereotype441 @kallentu @mit-mit

@leafpetersen leafpetersen added the records Issues related to records. label Nov 30, 2022
@munificent
Copy link
Member

I had that same conversion with @rakudrama. :)

An early version of this proposal had a static toMap() method on Record that behaved similarly. We removed it because we didn't want to require all implementations to support this potentially costly reflective information. It is helpful to know that the cost is apparently small in dart2js, but I would hesitate to assume that it is low-cost on all other implementations both today and henceforth forever.

I'm not opposed to adding something like it back—I can see it being useful—but our general experience has been that any reflective API ends up being regretful so I'm hesitant to do it unless we know there's real demand for it.

@lrhn
Copy link
Member

lrhn commented Dec 1, 2022

I'd prefer to not add such a method, but I'm less worried about performance.
It isn't really reflection, it's more like conversion. It doesn't provide access to anything that isn't directly accessible from the object itself. The result does not give you any hook back into the original record.

And, most importantly, there is no reification step in the other direction (you cannot go from an arbitrary map/list to a record).

It really is just a synthetic method on each record shape, so (?, ?, x: ?) would get a method:

Map<Object, Object?> toMap() => {0: $0, 1: $1, #x: x};

If we put an abstract method with that signature on Record, then you can use it to inspect the shape of any record, but you can't actually use that for much of anything.

@eernstg
Copy link
Member

eernstg commented Dec 1, 2022

I would certainly categorize asMap as a reflective mechanism, and the usual worries would apply: How much extra space is this going to take for any given (large) program if we store information (such as the name) about every non-Object getter in every record type? How costly is it going to be if the program contains one or more invocations of asMap with a receiver of type dynamic or Record? How does it interact with minimization? Etc.etc.

If all these issues are negligible then we should obviously have an asMap method on Object. ;-)

@natebosch
Copy link
Member

natebosch commented Dec 1, 2022

This would make writing generic code over records feasible in more places

Is this a goal?

This type of method was discussed early in the design, it was part of the original proposal and then we removed it. We cited performance concerns at the time.

It isn't really reflection

I think you previously argued that this functionality would belong in dart:mirrors. 😉

In any case, I think I agree with Lasse that "I don't want to allow you to do any operation on an untyped record, other than checking that it has a specific known structure."

@munificent
Copy link
Member

I could be persuaded otherwise, but I think the right answer is to leave this out.

@leafpetersen
Copy link
Member Author

It isn't really reflection

I think you previously argued that this functionality would belong in dart:mirrors. 😉

I'm fine leaving this out, but I don't think this requires any metadata that isn't already required for the toString method.

@natebosch
Copy link
Member

I don't think this requires any metadata that isn't already required for the toString method.

It does if it requires us to keep field names at runtime. We left toString deliberately unspecified for release builds so that implementations can omit field names, or even entire fields.

@lrhn
Copy link
Member

lrhn commented Dec 2, 2022

I do think it belongs in dart:mirrors more than on the object itself. It's borderline, but I prefer to err on the side of caution. It doesn't as much expose reflective information as it depends on it.

It's not reflection in the most dangerous sense, one that requires keeping source names at runtime and allowing access through them (resolving dynamically created names at runtime). It does retain names, though, and structure information that could otherwise possibly be dropped.

Other language features does that too, like Object.noSuchMethod which requires remembering every member name and named argument name of an invocation that might hit a user-defined noSuchMethod. It would be easier to optimize those away without user-defined noSuchMethods.
And dynamic invocation, which requires classes to allow member lookup of names whether they exist on the class or not (not something you can just do with a V-table). That looks like runtime name resolution, but it's limited to names that are mentioned in the source code.

I'd be happy to not make field access on records be normal instance methods, but more like extension/no-virtual methods on the static record type which accesses the fields directly, so that they cannot be accessed through dynamic invocations.
Those are operations on an untyped record, typed as dynamic. (So are the five methods of Object, but those are unavoidable, and we can implement those without retaining structure information or names, because we deliberately designed the behavior of those methods to allow it.).
We allowed those dynamic getter invocations anyway, mainly because we don't worry too much about performance of dynamic member access. It's OK for it to have to dig a little to find the extra information it needs.
It still means that we need to retain a mapping from member names to storage locations in the record representation, for each record shape, information we could otherwise have omitted from the compiled code.
(At least unless we can detect that the record is not used in any dynamic lookup, then we can omit the information anyway. But that's true for every class that might get hit by a dynamic lookup. That's the cost of dynamic, not so much a property of records.)

Adding a toMap on all record types, which can be accessed via a call on the static type Record, means that each record shape we have in the program must be able to describe its own structure. We have most of that structure implicitly in the dynamic member getter access (actually, dynamic invocation information doesn't need to distinguish a trailing fifth positional field from a named field named $4 in order to access the value. But it's close. If we make the map keys [$1] instead of 1, it's the same information).
So, it's not impossible to make a toMap. It requires some runtime structure information which is almost, but not completely, the same as the information needed for dynamic invocations.

That makes it one more way that that information will need to be retained in AoT builds. Which is a reason to avoid it. And it is specific to records, unlike dynamic invocation. Being a class method, it looks like its something you'd be encouraged to use (unlike dynamic invocations, where we try to help you avoid them). The affordances point in the wrong direction.

(Reflection is very useful. People like it when it's there, like in Java. But it only really works in a JIT setting, it costs too much for AoT builds, sometimes in unpredictable ways, which is why Dart has been moving away from reflection on everything but the JIT'ed VM. We still have dynamic invocation and noSuchMethod, because they are occasionally useful or even necessary. But they too do cost if you use them in AoT code, which is why there are lints and analysis tools to help you avoid them.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
records Issues related to records.
Projects
None yet
Development

No branches or pull requests

5 participants