Skip to content
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.

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

refactor function functions to something better behaved #9362

Open
erights opened this issue May 13, 2024 · 4 comments
Open

refactor function functions to something better behaved #9362

erights opened this issue May 13, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@erights
Copy link
Member

erights commented May 13, 2024

A long time ago now we did a contentious analysis of which of JS's many ways of writing functions we should include in our normal programming style. The controversial answer, which I still hold to, is to almost always avoid function functions. The problem is that they have too much extraneous semantics beyond their normally intended use.

  • function functions have a construct behavior and a distinct call behavior.
  • function functions have a prototype property whose value is initially a novel empty object.
  • function function declarations hoist in initialization time, unlike every other scoping construct.
  • function function declarations are exported early in an import cycle, before the module has initialized, making it syntactically burdensome to ensure they are hardened before an importer can access them. This was the fatal problem.

All these harm ocap security in the small. Again, especially the last "fatal" problem listed above.

We still need to remove almost all uses of function. The only justified uses I know of are

  • to emulate class constructors, which have a new (construct) behavior. For this purpose, a new.target === undefined || Fail… can emulate the absence of a call behavior
  • to emulate a builtin function with a construct behavior.

#4608 tried applying an automatic refactoring tool to transform function functions to arrow functions. However, it was naively textual and had too many errors (for me) to trust it. IIRC @Chris-Hibbert says there’s a good such refactoring built into intellij, but it was unclear how to apply it to a large corpus.

Not all function functions should be transformed to arrow functions. A tiny number should remain function functions for one of the above two reasons. Of the remainder, those that mention this should probably become concise methods.

We already almost never use var and class, so removing function functions would leave us only let and const, both of which are extremely safe, predictable, and well behaved.

@erights erights added the bug Something isn't working label May 13, 2024
@erights
Copy link
Member Author

erights commented May 13, 2024

Currently, the occurrence of matches of function( ?\w* ?)\( are as follows. Note that this is an imperfect test, but it be a reasonable approximation. I first omit Moddable code which purposely follows in own unrelated style decisions. I also omitted patches which again should conform to other's styles, not ours. As expected, for historical reasons, SwingSet and closely related packages are much of the total.

image

@erights
Copy link
Member Author

erights commented May 13, 2024

Endo's problem is much smaller, but it surprises me more than the size of the agoric-sdk problem

image

@turadg turadg changed the title Still need to refactor almost all function functions to something better behaved refactor function functions to something better behaved May 13, 2024
@Chris-Hibbert
Copy link
Contributor

IIRC @Chris-Hibbert says there’s a good such refactoring built into intellij, but it was unclear how to apply it to a large corpus.

WebStorm has a standard intent, which is trivial to invoke (one at a time). My standard pre-check grep pattern includes "function", so I routinely notice and remove them.

@turadg
Copy link
Member

turadg commented May 13, 2024

One way forward would be to fix the problems we found in https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions

The 4608 attempt used version 3.1.4 and it's on 3.3.2 now, but I don't see the problems addressed in https://github.com/JamieMason/eslint-plugin-prefer-arrow-functions/blob/main/CHANGELOG.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants