Clean up uses of aget / aset on objects

Description

With CLJS-2148 new warnings were added which emit diagnostics when inference indicates invalid aget or aset usage, and the usages were also addressed.

This ticket asks that other uses of aget and aset be fixed (where the use site does not have sufficient inference to trigger the warnings).

Environment

None

Activity

Show:
David Nolen
July 8, 2017, 4:43 PM

Actually just going to fix the source map changes myself.

David Nolen
July 8, 2017, 4:20 PM

This looks good but the changes to source map are unnecessary `sources` and `name` are in fact JavaScript arrays. Otherwise looks good.

Mike Fikes
July 8, 2017, 4:43 AM

Attaching a revised CLJS-2163-2.patch that patches a few more missed uses in the ObjMap implementation and two important ones:

  1. The printing of JavaScript objects in pr-writer-impl

  2. The js->clj implementation

The two above use unsafe-get in the patch, presuming they need to be extra performant.

Mike Fikes
July 5, 2017, 9:51 PM

For the two changes in ObjMap, I used unsafe-get, in case efficiency matters. (It looks like this deftype is deprecated, so I didn't bother to add tests for it)

For the change in js-invoke, I used unsafe-get in case efficiency matters. I added a unit test for js-invoke.

For the two changes in source-map, I used unsafe-get thinking that efficiency might really matter here. I didn't add any unit tests for these changes.

I ran a browser REPL (QuickStart), script/noderepljs, the Nashorn, and Rhino REPLs to ensure they didn't break. Interestingly, the Node REPL needed unsafe-get, otherwise you get "Cannot read property 'get' of undefined" for goog.object.get. Presumably this is too early in startup to use goog.object.

The changes in js-obj appear to already have coverage in the test suite. I also manually tested at the REPL.

Completed

Assignee

David Nolen

Reporter

Mike Fikes

Labels

Approval

Vetted

Patch

Code and Test