<< Back to previous view

[CLJS-528] Compiler should warn on any attempted extension of protocols to base JavaScript types Created: 20/Jun/13  Updated: 07/Nov/13  Resolved: 07/Nov/13

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 528.patch    


...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
  (m [this] "hi"))

ClojureScript:cljs.user> (m true)
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.

Comment by Rupa Shankar [ 21/Oct/13 3:16 PM ]

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

Comment by David Nolen [ 21/Oct/13 6:27 PM ]

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

Comment by David Nolen [ 04/Nov/13 8:24 AM ]

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

Comment by Rupa Shankar [ 04/Nov/13 10:59 AM ]

Fix for CLJS-528 using the new warning system.

Comment by David Nolen [ 04/Nov/13 8:15 PM ]

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.

Comment by Rupa Shankar [ 05/Nov/13 10:38 AM ]

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

Comment by David Nolen [ 05/Nov/13 11:09 AM ]

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

Comment by Rupa Shankar [ 06/Nov/13 10:20 AM ]

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

Comment by David Nolen [ 06/Nov/13 11:36 AM ]

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

Comment by Rupa Shankar [ 07/Nov/13 10:44 AM ]

So sorry about that! This should work.

Comment by David Nolen [ 07/Nov/13 2:11 PM ]

fixed https://github.com/clojure/clojurescript/commit/6d9ba920ccd5c364df847a9633c1c6c614917981

Generated at Wed Jan 24 03:27:39 CST 2018 using JIRA 4.4#649-r158309.