Skip to content

Support record updates on records with field named hasOwnProperty #2391

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 23, 2016
Merged

Support record updates on records with field named hasOwnProperty #2391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions examples/passing/HasOwnProperty.purs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module Main where

import Control.Monad.Eff.Console (log)

main = log ({hasOwnProperty: "Hi"} {hasOwnProperty = "Done"}).hasOwnProperty
2 changes: 1 addition & 1 deletion src/Language/PureScript/CodeGen/JS.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ moduleToJs (Module coms mn imps exps foreigns decls) foreign_ =
evaluate = JSVariableIntroduction Nothing evaluatedObj (Just obj)
objAssign = JSVariableIntroduction Nothing newObj (Just $ JSObjectLiteral Nothing [])
copy = JSForIn Nothing key jsEvaluatedObj $ JSBlock Nothing [JSIfElse Nothing cond assign Nothing]
cond = JSApp Nothing (JSAccessor Nothing "hasOwnProperty" jsEvaluatedObj) [jsKey]
cond = JSApp Nothing (JSAccessor Nothing "call" (JSAccessor Nothing "hasOwnProperty" (JSObjectLiteral Nothing []))) [jsEvaluatedObj, jsKey]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this have any performance implications?

Copy link
Contributor Author

@no-longer-on-githu-b no-longer-on-githu-b Oct 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may may allocate {} and look up call every time. But this should be benchmarked. Alternative implementations:

  • Cache hasOwnProperty in a global, e.g. var __hasOwnProperty = {}.hasOwnProperty. Then no {} has to be allocated.
  • Prohibit hasOwnProperty as a field name, this may additionally avoid issues with thousands of broken JS libraries out there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It most certainly does not optimize {}, or didn't not too long ago - we got a major performance benefit from avoiding those (they were quite pervasive though). Using Object.prototype.hasOwnProperty would be the better option I think.

Copy link
Contributor Author

@no-longer-on-githu-b no-longer-on-githu-b Oct 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that would break when a data constructor named Object is in scope. What about (0).hasOwnProperty?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could add Object to the list of reserved JS identifiers?

Copy link
Contributor Author

@no-longer-on-githu-b no-longer-on-githu-b Oct 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, when a data ctor named Object is defined, this would still work I think :)
Not when importing a module named Object though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(0).hasOwnProperty is kinda weird but I prefer it to the cache option for sure. Making Object reserved is probably an excellent idea if we don't already do that though, as I imagine it could muck up a thing or two.

assign = JSBlock Nothing [JSAssignment Nothing (JSIndexer Nothing jsKey jsNewObj) (JSIndexer Nothing jsKey jsEvaluatedObj)]
stToAssign (s, js) = JSAssignment Nothing (accessorString s jsNewObj) js
extend = map stToAssign sts
Expand Down