Open Bug 1658319 Opened 4 years ago Updated 2 years ago

Fix Function.toString for builtins

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

People

(Reporter: yulia, Unassigned)

References

(Blocks 2 open bugs, )

Details

I think we're already following that change.

For example:

print(Object.getOwnPropertyDescriptor(Map.prototype, "size").get.toString())

Prints:

function size() {
    [native code]
}

which matches the grammar NativeFunction: function NativeFunctionAccessor? PropertyName[~Yield, ~Await]? (FormalParameters[~Yield, ~Await]) { [native code] }.


The PR basically only allowed to keep the V8/JSC results which instead print (V8):

function get size() { [native code] }

resp. in JSC:

function get size() {
    [native code]
}

Ah yep, you are right. It doesn't require the change, only allows get/set as jsc and v8 have. Would it make sense for us to add this anyway?

Making that change should be easy, I think we just need to remove these lines: https://searchfox.org/mozilla-central/rev/e07d5eb5646b40b10df3164e315d4a3108cf9101/js/src/vm/JSFunction.cpp#939-950,966-967,969

I'm just not sure if there web/addon code which could break because of this change. For example the second answer at https://stackoverflow.com/questions/6598945/detect-if-function-is-native-to-browser/7536972 will no longer work (the stackoverflow link is the first one when googling for "javascript test if function is native"). (Obviously that kind of code is already not working in JSC/V8.)

Do you know why TC39 decided to make the accessor prefix optional? Was there no agreement for one or the other way, so simply both alternatives were made valid?

The goal from the committee was to "standardize what is there" as far as I understood. So it makes both Spidermonkey and V8/JSC correct. The ideal case (which is unclear yet if it is web compatible) would be to only have "get" instead of "function".

Day 1 notes: https://github.com/tc39/notes/blob/master/meetings/2020-07/july-21.md#fix-functiontostring-for-builtins
Day 2 notes: https://github.com/tc39/notes/blob/master/meetings/2020-07/july-22.md#function-tostring-for-builtins-consensus-seeking-continuation-from-day-1

"I'm fine with this as an initial step. I'd like to see the syntax unified between user code and native code, but I understand that this would require research into web compatibility. Longer term I would prefer to get rid of the "function" keyword if we can and change function get name … to get name …." -- it seems like there was consensus around this idea and that we may move in this direction eventually.

Leaving off "function" will break lodash's native function detection: https://github.com/lodash/lodash/blob/master/isNative.js

Ok, what do you think about checking if this change breaks our addons, and if it does we close this, if it doesnt -- we can see what the temperature is with the team about changing it?

There is no mxr for addons replacement, is there? So all we could do to guard the change is #ifdef NIGHTLY_BUILD, right?

sgtm

No longer blocks: 1781205
You need to log in before you can comment on or make changes to this bug.