ClojureScript

Compiler should warn on any attempted extension of protocols to base JavaScript types

Details

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

Description

...specifically, anything that is otherwise covered by cljs.core/base-type:

  • js/Object
  • js/String
  • js/Number
  • js/Array
  • js/Function
  • js/Boolean

Extending protocols to any of these is impolite (pollution of shared prototypes, esp. after Google Closure compilation), and leads to undefined behaviour, e.g.:

(defprotocol P (m [this]))

(extend-protocol P
  js/Boolean
  (m [this] "hi"))

ClojureScript:cljs.user> (m true)
"hi"
ClojureScript:cljs.user> (m false)
"Error evaluating:" (m false) :as "cljs.user.m.call(null,false)"
org.mozilla.javascript.JavaScriptException: Error: No protocol method P.m defined for type boolean: false

The warning should suggest the symbol that should be used to name the intended type, e.g. number instead of js/Number.

Activity

Hide
Rupa Shankar added a comment -

So sorry about that! This should work.

Show
Rupa Shankar added a comment - So sorry about that! This should work.
Hide
David Nolen added a comment - - edited

There is a typo in your patch. Don't forget to run the tests! Thanks.

Show
David Nolen added a comment - - edited There is a typo in your patch. Don't forget to run the tests! Thanks.
Hide
Rupa Shankar added a comment -

Sorry about that, here you go! Thanks for your help.

Show
Rupa Shankar added a comment - Sorry about that, here you go! Thanks for your help.
Hide
David Nolen added a comment - - edited

Looks good but it doesn't apply to master, can you rebase the patch?

Show
David Nolen added a comment - - edited Looks good but it doesn't apply to master, can you rebase the patch?
Hide
Rupa Shankar added a comment -

Fix for CLJS-528 using the new warning system and checking for warning suppression.

Show
Rupa Shankar added a comment - Fix for CLJS-528 using the new warning system and checking for warning suppression.
Hide
David Nolen added a comment -

Patch looks great but you need to check that this warning isn't being suppressed - otherwise the user might see the warning multiple times at the REPL, etc. Check with Travis, he needed to modify his patches to conform as well.

Show
David Nolen added a comment - Patch looks great but you need to check that this warning isn't being suppressed - otherwise the user might see the warning multiple times at the REPL, etc. Check with Travis, he needed to modify his patches to conform as well.
Hide
Rupa Shankar added a comment -

Fix for CLJS-528 using the new warning system.

Show
Rupa Shankar added a comment - Fix for CLJS-528 using the new warning system.
Hide
David Nolen added a comment -

CJS-636 is now resolved, can we get an updated patch using the new shiny warning system?

Show
David Nolen added a comment - CJS-636 is now resolved, can we get an updated patch using the new shiny warning system?
Hide
David Nolen added a comment - - edited

This looks good but will have to be applied after I clean up the warning mechanism. Thanks much!

Show
David Nolen added a comment - - edited This looks good but will have to be applied after I clean up the warning mechanism. Thanks much!
Hide
Rupa Shankar added a comment -

The attached patch gives the appropriate warning upon attempted extensions to base JavaScript types.

Show
Rupa Shankar added a comment - The attached patch gives the appropriate warning upon attempted extensions to base JavaScript types.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: