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

Spec changes related to specific types. #3302

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Aug 23, 2023

Address where the language specification refers to specific interfaces, and whether the spec must be changed to account for extension types now being able to subtype even final types like int.

The general rule is that the spec-invoked members of, e.g., Iterable, must always be invoked as instance members.

The few places where members are invoked without requiring a specific type (really just .call and object patterns), which is also where extension members can currently be invoked, extension type members can also be invoked.

  • Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

Address where the language specification refers to
specific interfaces, and whether the spec must be changed
to account for extension types now being able to subtype
even final types like `int`.

The general rule is that the spec-invoked members of, 
e.g., `Iterable`, must always be invoked as instance members.

The few places where members are invoked without requiring a specific type (really just `.call` and object
patterns), which is also where extension members can
currently be invoked, extension type members can also
be invoked.
@lrhn lrhn requested a review from eernstg August 23, 2023 17:59
Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting! I think we need to discuss this a bit before it is landed, also because it's a modification in accepted.

@@ -1461,3 +1461,319 @@ wrapper class is so huge that it is likely to be a major use case for
extension types that they can allow us to use a built-in class as the
representation, and still have a specialized interface—that is, an
extension type.

## Specification changes related to specific types
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be located before the 'Discussion' section, because it contains normative elements.

We might also separate this into normative material (probably rather short) and a background part.

For instance, the paragraph in lines 1467-1468 serves as an introduction to the overall topic ("this is about classes mentioned in the language specification"), but it starts by noting a fact which is already specified (in terms of rules about implements on extension types).

Comment on lines +1467 to +1468
Extension types can implement class types, even class types and their members,
which are mentioned in the language specification.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is somewhat easy to misunderstand or get confused about. Perhaps:

Suggested change
Extension types can implement class types, even class types and their members,
which are mentioned in the language specification.
Extension types can implement class types, even class types which are
mentioned in the language specification, and even class types that cannot
be implemented by other classes. They can also declare or redeclare
some members with names from the interfaces of such class types.

Comment on lines +1470 to +1475
By disallowing extension members with the same names as the members of `Object`,
we have reduced the problem to where the specification refers to members of
specific interfaces.
The specification needs to be updated to account for the existence of extension
types that are subtypes of those types, and which possibly shadow members from
the interface with extension members.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
By disallowing extension members with the same names as the members of `Object`,
we have reduced the problem to where the specification refers to members of
specific interfaces.
The specification needs to be updated to account for the existence of extension
types that are subtypes of those types, and which possibly shadow members from
the interface with extension members.
*An extension type instance member cannot have a name whose basename is also the basename of an instance member of `Object`. For other names, the specification needs to take potential extension type members into account in some situations. For instance, an extension type could redeclare `iterator` on a subtype of `Iterable`.*

Still commentary.

extension type which implements `Iterable` and shadows `iterator`.

**General Rule:** Where the existing language specification can invoke or access
an extension member, it can also invoke an extension type member, and where it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably, 'access' is needed in order to include tear-offs?:

Suggested change
an extension member, it can also invoke an extension type member, and where it
an extension member, it can also invoke or access an extension type member, and where it


**General Rule:** Where the existing language specification can invoke or access
an extension member, it can also invoke an extension type member, and where it
cannot invoke an extension member, it also will not invoke an extension type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'or access' again, twice?

not, and whether it defines its own `length`, `operator[]` or `containsKey`
extension type members. For all practical purposes, the value is cast to `Map<K,
V>`/`List<E>` for some `K` and `V`, or `E`, *before* accessing elements, and it
uses the caching behavior of instance elements.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we avoid the "magic" and simply make it a compile-time error for a collection pattern to invoke length etc. if it is an extension type instance member rather than a class instance member.

Next, I'm not 100% convinced that it should be an error. It could be a useful hook.

Just talked about this IRL: We might consider allowing extension type instance members and/or extension instance members to provide the required members to support iteration:

So the requirement for being usable in a for loop could be (1) must be an Iterable<T> for some T (the static type could be dynamic or some S <: Iterable<T>), or (2) must have a static type which is an extension type that has the required members (iterator, yielding an Iterator<T> for some T, or returning an object typed as E which is an extension type that has moveNext() and current).

We might want to treat for loops differently than other built-in constructs, because they are so performance critical. (For instance, it might be very important that we could perhaps use a native kind of iteration on a JSArray.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we avoid the "magic" and simply make it a compile-time error for a collection pattern to invoke length etc. if it is an extension type instance member rather than a class instance member.

That could apply to collection patterns in declaration patterns, but not in refutable patterns.

That is, it's a compile-time error if the matched value type of an irrefutable list pattern (in a declaration/assignment pattern) has an extension type instance member named length or operator[], and if the matched value type of an irrefutable map pattern has an extension type instance member named contains or operator[].

Or we can say that the list/map patterns always perform class instance member invocations for those methods, which is possibly what it says today. (But most likely it says nothing conclusive, because there used to be nothing to say.)

That rewrite is no longer valid with extension types. The type `T` may be an
extension type which *does* implement `Iterable<S>` for some `S`, but which then
*shadows* the default `iterator`, and returns something entirely unrelated to
`Iterator<S>`. _We do not intend to invoke that extension type member._
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, we could do it anyway.

on was `VeryInts`, which means that `_$id2` will get static type
`Iterator<int>`, and its `.current` is then assignable to `int i`. With the new
specification, we don’t look at the members of the actual type, instead we
immediately up-cast to `Iterable<E>` and work with that.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd definitely prefer that the developer must write that upcast and the treatment after that point is completely standard, rather than getting into a situation where some members are called based on the type Iterable<E> for some E while the receiver has an extension type V which has an iterator member with a different behavior.

The synchronous `yield*` operator takes an expression which, like `for`/`in`,
must be assignable to `Iterable<T>`, and it too must use only instance members to
iterate the operand where it state that it invokes `iterator`, `moveNext` and
`current`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably have the same level of support for a structural extension-or-extension-type based iteration in a for loop and in a yield* statement.

iterate the operand where it state that it invokes `iterator`, `moveNext` and
`current`.

#### Stream and `await for`, asynchronous `yield*`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should use rules that are as similar as possible with for and await for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, then the rule should be to not invoke extension type members.

I really do not want to get into defining which part of the Stream interface that await for and yield* uses.

  • Call listen with three functions arguments, and cancelOnError: true, get object back. (This is the simple step.)
  • Occasionally call pause() or pause(Future), maybe resume and cancel on the returned object.
  • Maybe read the isPaused getter. (Not necessary, but can be used as optimization, to never pause more than once.)
  • Possibly call onData, onError, onDone methods with function arguments (or maybe even with null). That's a valid approach to changing the continuation that the value comes back to, instead of keeping state and a trampoline on the side.
  • Maybe even call asFuture (but I doubt that's useful).
  • Await the result of calling cancel.

Basically, you have to return something which implements the entire StreamSubscription interface.
For Iterator, that's easy - it's two members and we always call both of them.
For StreamSubscription, you can possibly get away with only pause, resume and cancel. And if we let you get away with that, then it becomes a breaking change for us to change the implementation in the future.

So I want to enforce that whatever listen returns, it implements StreamSubscription.

Also, the compilation of async operations is much more complicated than a simple for/in loop. The way lowering and compilation of async functions works, we are likely reusing some functionality that does class instance member invocations. Having to create new compiler paths to remember an extension type, just to call an extension type pause method on something which is a StreamSubscription, is just not worth it.

The story of "An await for doesn't see your static type, only the Stream<T> that it implements, because that's all it needs" is simple and consistent.
That's why that's what I went with for non-async for/in too.

@dart-lang dart-lang deleted a comment Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants