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

Underspecified Set<T> and Map<K,V> semantics #472

Closed
DartBot opened this issue Nov 16, 2011 · 7 comments
Closed

Underspecified Set<T> and Map<K,V> semantics #472

DartBot opened this issue Nov 16, 2011 · 7 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant type-enhancement A request for a change that isn't a bug

Comments

@DartBot
Copy link

DartBot commented Nov 16, 2011

This issue was originally filed by alexey.v.var...@gmail.com


The corelib interface Set<T> does not define any particular way to distinguish objects, for checking uniqueness. Given there is also more specialized interface HashSet<E extends Hashable>, the Set is naturally assumed as a generic container for objects of any type.
However current Set declaration refers to factory HashSetImplementation<E>. As class Object does not implement Hashable, nor the interface Set mentions Hashable as a type bound, this provokes confusion and induces unexpected runtime errors.

The same issue applies to Map<K,V> vs HashMap<K extends Hashable, V>.

Suggested solutions:
Add equivalence function as a parameter to Set (Map), and provide a proper factory class for it.

Alternative (half-)solution:
Drop the "factory" clause from the generic Set (Map) and leave it as a pure abstraction, for future extensions. The HashSet (HashMap) remains a viable implementation with clear semantics.

Another alternative solution:
Make all objects hashable. This would address the Set/HashSet controversy as well.

@DartBot
Copy link
Author

DartBot commented Nov 16, 2011

This comment was originally written by alexey.v.varl...@gmail.com


There is an issue #167, which may be relevant to this one, yet these issues are not close duplicates. This issue highlights particular flaw in Set&Map declaration so is a bug rather than enhancement.

@peter-ahe-google
Copy link
Contributor

The interfaces Map and Set are currently underspecified. The specification should be:

interface Set<E> extends Collection<E> factory HashSetImplementation<E extends Hashable> {
...

interface Map<K, V> factory HashMapImplementation<K extends Hashable, V> {
...

However, none of our implementations allow this syntax yet. So you'll have to fill it in yourself when reading the API :-)


Removed Type-Defect label.
Added Type-Enhancement, Area-Library, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Nov 16, 2011

This comment was originally written by alexey.v.var...@gmail.com


Hmm, the chosen approach to add extra type bounds to the factory clause seems to be mere "technical" (or, syntactical) solution. Conceptually, it is still a bit weird to define a factory for an interface with incomplete contract - and apparently the choice of equivalence function is a significant part of Set meaning.

@kevmoo
Copy link
Member

kevmoo commented May 29, 2012

STRONGLY agreed w/ Comment #­3 from Alex.

If Set<E> and Map<K, V> doen't explicitly require Hashable keys, then their default implementations should not require Hashable keys. Period.

Better to eliminate the default implementations entirely and push folks towards HashSet and HashMap respectively.

The current implementation feels VERY broken.

@kevmoo
Copy link
Member

kevmoo commented Nov 27, 2012

Given that Hashable has gone away (everything is hashable) I think this issue can be closed.

@floitschG
Copy link
Contributor

Not a problem anymore since everything is now hashable.

That said: we are going to add means to provide equivalence functions to the map and set.


Added AssumedStale label.

@kevmoo
Copy link
Member

kevmoo commented Dec 18, 2012

Excellent.

@DartBot DartBot added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant labels Dec 18, 2012
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
nex3 pushed a commit that referenced this issue Aug 31, 2016
Allow analyzer master failures in Travis
copybara-service bot pushed a commit that referenced this issue Oct 31, 2022
Changes:
```
> git log --format="%C(auto) %h %s" 93d0eee..49eefd2
 https://dart.googlesource.com/markdown.git/+/49eefd2 Refactor AutolinkExtensionSyntax (#471)
 https://dart.googlesource.com/markdown.git/+/07e2683 Optimise TableSyntax (#472)
 https://dart.googlesource.com/markdown.git/+/9b61871 Make helper class private that should not have been exposed (#476)
 https://dart.googlesource.com/markdown.git/+/299964e Return list for link nodes creation (#452)
 https://dart.googlesource.com/markdown.git/+/aee6a40 validate code coverage on CI (#474)
 https://dart.googlesource.com/markdown.git/+/88f3f8a Fix html entity and numeric character references (#467)

```

Diff: https://dart.googlesource.com/markdown.git/+/93d0eee771f6355be6737c2a865f613f6b105bf1~..49eefd211e7840bac7e11257cd966435ae3cb07f/
Change-Id: I2a88d7c386f567738226701be4edcd7c4818744f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/266760
Auto-Submit: Devon Carew <devoncarew@google.com>
Commit-Queue: Oleh Prypin <oprypin@google.com>
Reviewed-by: Oleh Prypin <oprypin@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. closed-obsolete Closed as the reported issue is no longer relevant type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants