Skip to content

Comments

report arity errors in terms of the public API#100

Merged
bennn merged 1 commit intoracket:masterfrom
bennn:proc-rename
Aug 20, 2018
Merged

report arity errors in terms of the public API#100
bennn merged 1 commit intoracket:masterfrom
bennn:proc-rename

Conversation

@bennn
Copy link
Contributor

@bennn bennn commented Aug 11, 2018

Fix for #99

Now this program:

#lang racket

(require rackunit)
(check-exn (lambda () 'hi))

Errors with:

check-exn: arity mismatch;
 the expected number of arguments does not match the given number
  given: 1
  arguments...:
   #<procedure:...rackunit/foo.rkt:4:11>
  context...:
   "/..../rackunit/foo.rkt": [running body]
   for-loop
   run-module-instance!125
   perform-require!78

This still needs a regression test. (EDIT: added a test)

But also, I'm not happy adding an extra lambda to get the keyword arguments. Suggestions welcome.

@AlexKnauth
Copy link
Member

Does this fix #103 as well?

(procedure-rename
(λ (formal ... [message #f])
(λ (#:location [location (list 'unknown #f #f #f #f)]
#:expression [expression 'unknown])
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to flip these two lambdas, making it more like:

  (λ (#:location [location (list 'unknown #f #f #f #f)]
      #:expression [expression 'unknown])
    (procedure-rename
      (λ (formal ... [message #f])
        ..stuff..)
      'pub))

?

It would simplify the id case of the name macro below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see how that would be simpler by itself. Wouldn't we just go from this:

((apply check-impl args) #:loc .... #:expr ....)

to this:

(apply (check-impl #:loc .... #:expr ....) args)

... maybe its best to remove #:location and #:expression and have define-check expand to code that updates current-check-info manually

Copy link
Member

@AlexKnauth AlexKnauth Aug 17, 2018

Choose a reason for hiding this comment

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

I mean the id case could be simplified from this:

          #'(λ args
               ((apply check-impl args)
                #:location (syntax->location #'loc)
                #:expression 'chk))

To this:

          #'(check-impl
              #:location (syntax->location #'loc)
              #:expression 'chk)
  1. (λ args ((apply check-impl args) #:loc .... #:expr ....)) initially.
  2. (λ args (apply (check-impl #:loc .... #:expr ....) args)) after flipping the lambdas.
  3. (check-impl #:loc .... #:expr ....) after eta-reducing further.

Copy link
Member

Choose a reason for hiding this comment

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

It also has the benefit of (procedure-arity check-equal?) returning (list 2 3) instead of (arity-at-least 0).

@bennn
Copy link
Contributor Author

bennn commented Aug 17, 2018

yes this fixes #103

@bennn
Copy link
Contributor Author

bennn commented Aug 19, 2018

I'm planning to merge this tomorrow night

@AlexKnauth
Copy link
Member

To me, this looks good now.

@bennn
Copy link
Contributor Author

bennn commented Aug 19, 2018

wow, great timing!

change the internal procedures for `check-true` and others, so that:

- their arity matches the documentation
- their `object-name` names match the documentation
@bennn bennn merged commit eceb061 into racket:master Aug 20, 2018
@bennn bennn deleted the proc-rename branch August 20, 2018 05:02
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