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

Qualified Pervasives.compare in Ord #1

Merged
merged 1 commit into from
Aug 11, 2014
Merged

Conversation

labichn
Copy link
Contributor

@labichn labichn commented Aug 11, 2014

Because when the annotated type is just 't' the comparison function is named 'compare' and captures what should be Pervasives.compare in its body. We need hygiene.

is just 't' the comparison function is named 'compare' and captures
what should be Pervasives.compare in its body. We need hygiene.
@whitequark
Copy link
Collaborator

@labichn Agreed on hygiene in general, however, in this case we explicitly declare a global identifier and then explicitly use a global identifier. Could hypothetical hygienic macros save us from this bug?

whitequark added a commit that referenced this pull request Aug 11, 2014
Qualified Pervasives.compare in Ord
@whitequark whitequark merged commit e7f7447 into ocaml-ppx:master Aug 11, 2014
@labichn
Copy link
Contributor Author

labichn commented Aug 11, 2014

Yep, especially because the bug still exists, it's just less likely to occur. It's not a bug in Ord, but how the rewriting is done. If we use Ord in the wrong environment, we'll still run into issues.

let cmp = Pervasives.compare

module Pervasives = struct
  let compare a b = -2
end

type t = A | B of int
(*  [@@deriving Ord] generates *)
let rec compare lhs rhs =
  match (lhs, rhs) with
  | (A ,A ) -> 0
  | (B lhs0,B rhs0) ->
      (match (fun (a : int)  -> fun b  -> Pervasives.compare a b) lhs0 rhs0
       with
       | (-1)|1 as x -> x
       | _ -> 0)
  | _ ->
      let to_int = function | A  -> 0 | B _ -> 1 in
      ((fun (a : int)  -> fun b  -> Pervasives.compare a b)) (to_int lhs)
        (to_int rhs)

let _ = print_endline (string_of_int (compare (B 2) (B 3))) ;; (* =>  0 *)

let _ = print_endline (string_of_int (cmp (B 2) (B 3))) ;;     (* => -1 *)

We want -1 to print, but since the generated match doesn't handle values outside the range [-1, 1] and our "accidentally" clobbering Pervasives.compare always returns -2, the generated compare will always return 0 for Bs, regardless of the integer inside. A hygienic macro system will ensure that names are bound in their original context, not moved around like text that then uses whatever context happens to exist once the movement is done. This Scheme macro (stolen from wikipedia) is safe to use even inside a redefinition of `not' since syntax-rules ensures it will not be captured, and message doesn't print.

(define-syntax my-unless
   (syntax-rules ()
     [(_ condition body)
      (if (not condition)
          body
          (void))]))

(let ([not (lambda (x) x)])
  (my-unless #t
    (displayln "This will not be printed!")))

;; Run this in Racket to see the expansion.
(require macro-debugger/stepper-test)
(expand/step-text #'(let ([not (lambda (x) x)])
                      (my-unless #t (displayln "This will not be printed!")))
                  (list #'my-unless))
;; =>
;; Macro transformation
#;(let ((not (lambda (x) x)))
    (my-unless #t (displayln "This will not be printed!")))
;;  ==>
#;(let ((not (lambda (x) x)))
    (if:1 (not:1 #t) (displayln "This will not be printed!") (void:1)))

If we had this mechanism, we could avoid capturing the redefinition of Pervasives.compare like we do in the above example.

@whitequark
Copy link
Collaborator

@labichn Agreed; unfortunately, there is no way to get "original" Pervasives in OCaml.

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