Skip to content

Allow spread element types to disambiguate a set/map literal. #168

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Allow spread element types to disambiguate a set/map literal. #168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions accepted/future-releases/spread-collections/feature-specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,18 +331,28 @@ An expression like:
```

Is syntactically parsed as `mapOrSetLiteral`. To determine whether it actually
is a map or set, the surrounding context is used. Given an `mapOrSetLiteral`
with context type C:
is a map or set, the upwards and downwards type inference contexts are used.

* If `Set<Null>` is assignable to C, and `Map<Null, Null>` is not assignable
to C, then the collection is a set literal.
Given a `mapOrSetLiteral`:

* Otherwise, it is a map literal.
1. If `Map<Null, Null>` is assignable to the literal's context type, then it
is a map literal.

2. Else, if `Set<Null>` is assignable to the literal's context type, then the
Copy link
Member

Choose a reason for hiding this comment

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

Note that this is not the criteria that is specified for empty set literals, but there's active discussion about this here: dart-lang/sdk#35431

collection is a set literal.

3. Else, if it has spread elements and all of the spread element expression
Copy link
Member

Choose a reason for hiding this comment

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

I would phrase this differently. I think it's probably better to say that that if there is a non-empty context, you resolve based on the context type (as above, or however we decide). Only if there is no context type do you resolve based on the upwards inference. Otherwise, I think it's guaranteed to be an error anyway, and you're just making things confusing by still trying to use upwards inference.

Copy link
Member

Choose a reason for hiding this comment

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

Agree. You only need to address the case where there is no context type and no type arguments. The rest are already handled and is unaffected by spreads or comprehensions.

I think we need to rewrite everything around this so that we only have "setOrMapLiteral" as a syntactic category. It's going to be complicated to have:

setLiteral ::= `{` setElements `}`
mapLiteral ::= `{ mapEntries `}`
mapEntry ::= expression `:` expression |  `...` expression | `if` `(` expression `)` mapEntry (`else` mapEntry)? | `for` `(` ... `in` expression `)` mapEntry ;
setElement ::= expression  |  `...` expression | `if` `(` expression `)` setElement (`else` setElement)? | `for` `(` ... `in` expression `)` setElement ;
setOrMapPart ::= |  `...` expression | `if` `(` expression `)` setOrMapPart (`else` setOrMapPart)? | `for` `(` ... `in` expression `)` setOrMapPart ;

because they do overlap unless deep-down in the structure there is either an e element or an e1:e2 entry.

So, I'd go for unifying sets and maps, and then use the context type or content parts to determine which one it is. And I wouldn't separate spreads and if/for constructs, they both need the same treatment anyway.

Let me try doing that.

types are subtypes of `Iterable`, then it is a set literal.

**TODO: Is dynamic a subtype of Iterable? If so, this needs to be tweaked.
Copy link
Member

Choose a reason for hiding this comment

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

No, it's not.

If all the spreads are dynamic, then it should be a map literal.**

4. Otherwise, it is a map literal.

In other words, if it can only be a set, it is. Otherwise, it's a map. In cases
where the context type is not specific enough to disambiguate, we could make it
an error instead of defaulting to map. However, that would be inconsistent with
how empty collections are handled. Those have to default to map for backwards
where the types are not specific enough to disambiguate, we could make it an
error instead of defaulting to map. However, that would be inconsistent with how
empty collections are handled. Those have to default to map for backwards
compatibility.

### Const spreads
Expand Down