ClojureScript

implements? may report false positives

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code

Description

The cljs.core/implements? checks whether a protocol is implemented via a property on the tested object. When implementing the protocol this property will be set to true.

// the implementation
protocol.check.Dummy.prototype.protocol$check$TheProtocol$ = true;

// the check (implements? TheProtocol x)
((!((x == null)))?(((false) || (x.protocol$check$TheProtocol$))?true:false):false)

// only the relevant bit
x.protocol$check$TheProtocol$

This works fine under :none but :advanced may rename this to something like x.A. If you now try to check (implements? TheProtocol js/window) for example it may (or may not) report a false positive. For larger projects the likelyhood of creating a false positives goes way up since window contains all the variables created by the advanced compiled js.

The attached patch changes the patch the emitted code to check x.protocol$check$TheProtocol$ === true which reduces the chance of a false positive by a lot. We might chose another sentinel value instead to reduce the chance some more, this seems good enough though.

Activity

Hide
Thomas Heller added a comment -

Implemented the same change for cljs.core/satisfies?.

Show
Thomas Heller added a comment - Implemented the same change for cljs.core/satisfies?.
Hide
Thomas Heller added a comment -

Benchmark on my MBP indicate that the change comes with a slight performance cost.

// v8 with patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 10 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 27 msecs

//v8 without patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 8 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 19 msecs
Show
Thomas Heller added a comment - Benchmark on my MBP indicate that the change comes with a slight performance cost.
// v8 with patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 10 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 27 msecs

//v8 without patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 8 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 19 msecs
Hide
Thomas Heller added a comment -
// spidermonkey with patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 68 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 146 msecs

// spidermonkey without patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 63 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 149 msecs
Show
Thomas Heller added a comment -
// spidermonkey with patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 68 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 146 msecs

// spidermonkey without patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 63 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 149 msecs
Hide
Thomas Heller added a comment -

JavascriptCore

// jsc with path
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 7 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 14 msecs

// jsc without patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 6 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 13 msecs
Show
Thomas Heller added a comment - JavascriptCore
// jsc with path
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 7 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 14 msecs

// jsc without patch
;;; satisfies?
[coll (list 1 2 3)], (satisfies? ISeq coll), 1000000 runs, 6 msecs
[coll [1 2 3]], (satisfies? ISeq coll), 1000000 runs, 13 msecs
Hide
Thomas Heller added a comment -

Added a second patch that uses a sentinel value as the protocol marker. Currently just a plain empty javascript object.

Running the benchmarks shows no real difference to just checking "true".

I also tried using a number or string as the sentinel value but could not find any significant difference either.

// emitted code on protocol impls
protocol.check.Dummy.prototype.protocol$check$TheProtocol$ = cljs.core.PROTOCOL_SENTINEL;

// the check (identical?)
cljs.core.PROTOCOL_SENTINEL === x.protocol$check$TheProtocol$
Show
Thomas Heller added a comment - Added a second patch that uses a sentinel value as the protocol marker. Currently just a plain empty javascript object. Running the benchmarks shows no real difference to just checking "true". I also tried using a number or string as the sentinel value but could not find any significant difference either.
// emitted code on protocol impls
protocol.check.Dummy.prototype.protocol$check$TheProtocol$ = cljs.core.PROTOCOL_SENTINEL;

// the check (identical?)
cljs.core.PROTOCOL_SENTINEL === x.protocol$check$TheProtocol$

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: