Skip to content

Add proposal for "Spread Collections". #48

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

Merged
merged 3 commits into from
Oct 25, 2018
Merged

Conversation

munificent
Copy link
Member

Here it is!

Verified

This commit was signed with the committer’s verified signature.
iluuu1994 Ilija Tovilo

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@mit-mit
Copy link
Member

mit-mit commented Oct 17, 2018

Associated feature issue: #47

then the downwards inference context type of a spread element in that list
is `Iterable<T>`.

* If a spread element in a list literal has type `Iterable<T>` for some `T`,
Copy link
Member

Choose a reason for hiding this comment

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

... has static type ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


1. Evaluate `iterator.current` to a value `newEntry`.

1. Call `map[newEntry.key] = value`.
Copy link
Member

Choose a reason for hiding this comment

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

value -> newEntry.value

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@lrhn
Copy link
Member

lrhn commented Oct 25, 2018

LGTM with comments.

This works because the individual elements in `numbers` do happen to have the
right type even though the list that contains them does not. However, the spread
object does need to be "spreadable"&mdash;it must be some kind of Iterable for a
list literal or a Map for a map literal.
Copy link
Member

Choose a reason for hiding this comment

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

Consider a comment to point out that this also doesn't require that the type of the thing being spread be assignable to the type of the List. e.g. spreading MyList into a List (neither is a subtype of the other).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


If implicit downcasts are disabled, then the "is assignable to" parts here
become strict subtype checks instead.

Copy link
Member

Choose a reason for hiding this comment

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

There is a specified error preventing duplicate keys in map literals when the keys are constant. Should we consider that for nested map literals + spread? Should the following be an error?

  var a = {3 : true,
               ...{3 : false}};

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 thought about duplicate keys, but the static checking for that is even pretty brittle for flat literals since it only works for literal keys (obviously):

var key = "key";
var map = {key: 1, key: 2}; // <-- No error.

So I figured the duplicate key checking wasn't very high value and wasn't worth extending into spread. Also, it might get weird if we introduce "conditional spread" features.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the rule about duplicate keys only apply to constant maps:

It is a compile-time error if two keys of a constant map literal are equal.

so since spreads do not apply to constant maps, that issue is moot.

@munificent munificent merged commit 0ae77e4 into master Oct 25, 2018
@munificent munificent deleted the spread-collections branch October 25, 2018 21:17
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.

None yet

5 participants