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

add keyword-apply/dict to racket/dict #2592

Merged
merged 2 commits into from
Oct 7, 2020

Conversation

AlexKnauth
Copy link
Member

See #2548. The keyword-apply/dict function is a variation on keyword-apply that solves two problems:

  • Keyword sorting
  • Parallel-lists where keyword and value are associated only by having the "same index"
(keyword-apply/dict proc kw-dict pos-arg ... pos-args) -> any
      proc : procedure?
   kw-dict : (dict/c keyword? any/c)
   pos-arg : any/c
  pos-args : (listof any/c)

@sorawee
Copy link
Collaborator

sorawee commented Apr 22, 2020

In the same spirit as apply, can keyword-apply/dict support #:<kw> kw-arg ... too?

@jackfirth
Copy link
Sponsor Contributor

jackfirth commented Apr 26, 2020

@sorawee That seems reasonable but also a little dangerous, since the static keywords could overlap with the dynamic ones. Example: (keyword-apply/dict some-keyword-function (hash '#:color "green") #:color "red" '()). I assume that would be an error?

@AlexKnauth The gen:dict interface allows duplicate keys (but leaves their behavior unspecified), and lists implement the dictionary interface by using assoc. What should this code do?

(keyword-apply/dict
  some-keyword-function
  '((#:color . "red") (#:color . "green"))
  '())

I think a naive implementation of keyword-apply/dict would ignore the second color keyword (because that's what dict-ref on an association list would do), but I'd much rather the implementation throw an error. If this happened in real code it almost certainly means there's a bug hidden somewhere.

@AlexKnauth
Copy link
Member Author

I think that keyword-apply/dict should interpret all valid dicts as dict operations would interpret them. Either that's an invalid dict and dict? should return false, or keyword-apply/dict should interpret the dict as the dict operations do.

@AlexKnauth
Copy link
Member Author

@sorawee It does support #:<kw> kw-arg ..., but I didn't document that. Maybe I should.

@jackfirth (keyword-apply/dict some-keyword-function (hash '#:color "green") #:color "red" '()) is an error just like (keyword-apply some-keyword-function '(#:color) '("green") #:color "red" '()) is, although the error message is about keyword-apply instead of keyword-apply/dict because it just delegates to keyword-apply of keyword-apply in that situation.

@AlexKnauth AlexKnauth force-pushed the better-keyword-apply branch 2 times, most recently from 69f6a74 to ed659b6 Compare May 15, 2020 07:21
@AlexKnauth
Copy link
Member Author

AlexKnauth commented May 15, 2020

@sorawee I have now documented that keyword-apply/dict supports #:<kw> kw-arg .... The rendered docs look like
docs

@AlexKnauth
Copy link
Member Author

@jackfirth I have updated the error message, to be about keyword-apply/dict instead of keyword-apply.

> (keyword-apply/dict void (hash '#:color "green") #:color "red" '())
keyword-apply/dict: keyword duplicated in dict and direct keyword arguments: '#:color


@defproc[
(keyword-apply/dict [proc procedure?]
[kw-dict (dict/c keyword? any/c)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is dict/c? I searched and only found one from mischief which I don't think is what you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I had assumed (dict/c keyword? any/c) was like (hash/c keyword? any/c), but for generic dicts and not just hash tables. But I suppose if it doesn't exist, maybe just dict? should go there? Although I would prefer it to say the keys must be keywords... would (and/c dict? (compose (listof keyword?) dict-keys)) work better? Kinda long though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've changed it to dict? for now

@AlexKnauth
Copy link
Member Author

Is it okay if I try to copy the implementation of hash/c and modify it to define dict/c?

@samth
Copy link
Sponsor Member

samth commented May 15, 2020

I think that should be a separate PR. Maybe just use dict? here.

@samth
Copy link
Sponsor Member

samth commented Jul 1, 2020

This needs some tests, and a history note. Also, I'm not sure that the dict documentation is the right place for this. In general, since this seems like a broadly-useful feature having it presented as dict-specific seems wrong.

@sorawee
Copy link
Collaborator

sorawee commented Jul 2, 2020

Perhaps racket/function is a more appropriate place?

@AlexKnauth
Copy link
Member Author

Currently racket/function doesn't depend on racket/dict or racket/private/dict. Is it okay to add that dependency?

If so then racket/function sounds like a good place.

@sorawee
Copy link
Collaborator

sorawee commented Jul 2, 2020

Yeah, that dependency will definitely slow down startup time a bit.

Following is the result of running for i in {1..10}; do time racket test.rkt; done on various programs:

#lang racket/base

#|
racket test.rkt  0.12s user 0.03s system 97% cpu 0.154 total
racket test.rkt  0.13s user 0.03s system 94% cpu 0.168 total
racket test.rkt  0.11s user 0.03s system 97% cpu 0.149 total
racket test.rkt  0.12s user 0.03s system 96% cpu 0.157 total
racket test.rkt  0.12s user 0.03s system 96% cpu 0.154 total
racket test.rkt  0.12s user 0.03s system 94% cpu 0.157 total
racket test.rkt  0.12s user 0.03s system 96% cpu 0.151 total
racket test.rkt  0.12s user 0.03s system 96% cpu 0.160 total
racket test.rkt  0.12s user 0.03s system 95% cpu 0.160 total
racket test.rkt  0.12s user 0.03s system 94% cpu 0.157 total
|#
#lang racket/base

(require racket/function)

#| 
racket test.rkt  0.12s user 0.03s system 95% cpu 0.161 total
racket test.rkt  0.14s user 0.04s system 89% cpu 0.196 total
racket test.rkt  0.12s user 0.03s system 97% cpu 0.158 total
racket test.rkt  0.12s user 0.03s system 96% cpu 0.159 total
racket test.rkt  0.12s user 0.03s system 95% cpu 0.155 total
racket test.rkt  0.12s user 0.03s system 97% cpu 0.154 total
racket test.rkt  0.13s user 0.03s system 95% cpu 0.169 total
racket test.rkt  0.12s user 0.03s system 95% cpu 0.158 total
racket test.rkt  0.12s user 0.03s system 98% cpu 0.156 total
racket test.rkt  0.12s user 0.03s system 97% cpu 0.151 total
|#
#lang racket/base

(require racket/dict)

racket test.rkt  0.20s user 0.05s system 96% cpu 0.256 total
racket test.rkt  0.20s user 0.05s system 97% cpu 0.253 total
racket test.rkt  0.20s user 0.05s system 96% cpu 0.251 total
racket test.rkt  0.20s user 0.05s system 93% cpu 0.272 total
racket test.rkt  0.19s user 0.05s system 96% cpu 0.251 total
racket test.rkt  0.20s user 0.05s system 95% cpu 0.257 total
racket test.rkt  0.21s user 0.05s system 95% cpu 0.273 total
racket test.rkt  0.20s user 0.05s system 95% cpu 0.260 total
racket test.rkt  0.20s user 0.05s system 95% cpu 0.269 total
racket test.rkt  0.19s user 0.05s system 97% cpu 0.246 total

And here are ones that use raco make (raco make test.rkt; for i in {1..10}; do time racket test.rkt; done)

#lang racket/base

#|
racket test.rkt  0.08s user 0.02s system 96% cpu 0.100 total
racket test.rkt  0.08s user 0.03s system 82% cpu 0.132 total
racket test.rkt  0.09s user 0.03s system 96% cpu 0.115 total
racket test.rkt  0.08s user 0.02s system 93% cpu 0.111 total
racket test.rkt  0.08s user 0.02s system 97% cpu 0.103 total
racket test.rkt  0.08s user 0.02s system 95% cpu 0.112 total
racket test.rkt  0.08s user 0.02s system 95% cpu 0.103 total
racket test.rkt  0.08s user 0.03s system 95% cpu 0.112 total
racket test.rkt  0.08s user 0.02s system 94% cpu 0.109 total
racket test.rkt  0.08s user 0.03s system 93% cpu 0.116 total
|#
#lang racket/base

(require racket/function)

#| 
racket test.rkt  0.08s user 0.02s system 95% cpu 0.108 total
racket test.rkt  0.09s user 0.03s system 94% cpu 0.123 total
racket test.rkt  0.09s user 0.03s system 95% cpu 0.119 total
racket test.rkt  0.08s user 0.02s system 95% cpu 0.108 total
racket test.rkt  0.08s user 0.02s system 94% cpu 0.111 total
racket test.rkt  0.08s user 0.03s system 93% cpu 0.116 total
racket test.rkt  0.08s user 0.03s system 96% cpu 0.109 total
racket test.rkt  0.08s user 0.02s system 95% cpu 0.108 total
racket test.rkt  0.08s user 0.02s system 96% cpu 0.106 total
racket test.rkt  0.08s user 0.02s system 97% cpu 0.104 total
|#
#lang racket/base

(require racket/dict)

racket test.rkt  0.15s user 0.04s system 96% cpu 0.202 total
racket test.rkt  0.15s user 0.04s system 95% cpu 0.196 total
racket test.rkt  0.14s user 0.04s system 95% cpu 0.183 total
racket test.rkt  0.14s user 0.04s system 95% cpu 0.188 total
racket test.rkt  0.15s user 0.04s system 93% cpu 0.199 total
racket test.rkt  0.14s user 0.04s system 95% cpu 0.186 total
racket test.rkt  0.15s user 0.04s system 91% cpu 0.210 total
racket test.rkt  0.14s user 0.04s system 96% cpu 0.188 total
racket test.rkt  0.14s user 0.04s system 97% cpu 0.178 total
racket test.rkt  0.15s user 0.04s system 94% cpu 0.195 total

@samth
Copy link
Sponsor Member

samth commented Jul 2, 2020

I'm fine with leaving it in racket/dict given that. The other option would be to skip generics entirely, and use hashes.

[Aside: the hyperfine tool is very useful for that kind of measurement.]

@AlexKnauth
Copy link
Member Author

I have added some tests in pkgs/racket-test/tests/racket/keyword-apply-dict.rkt.

@sorawee
Copy link
Collaborator

sorawee commented Oct 3, 2020

Can this be merged by 7.9 release?

@AlexKnauth
Copy link
Member Author

The last review mentioned 3 things: tests, a history note, and placement. I have added tests and a history note, and we agreed on placement in racket/dict. If there are no objections, can I merge at the end of the day today?

@AlexKnauth AlexKnauth merged commit 003ac9b into racket:master Oct 7, 2020
@AlexKnauth AlexKnauth deleted the better-keyword-apply branch October 7, 2020 13:29
maueroats pushed a commit to maueroats/racket that referenced this pull request Jun 17, 2021
* add keyword-apply/dict to racket/dict
* add history note
maueroats pushed a commit to maueroats/racket that referenced this pull request Jun 17, 2021
* add keyword-apply/dict to racket/dict
* add history note
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