Improve Function.prototype.toString() to return source text for non-native functions

[adams85]

I took a stab at implementing Function.prototype.toString() according to the spec. So this is another attempt at what #1726 tried to achieve, it's just takes the correct path instead of trying to convert the AST back to source code.

However, this requires the full source text to be kept around as it would be a performance killer to materialize the source text for functions eagerly. So this is deferred until execution. Once the source text is created, the reference to the full code is released. This way GC can at least collect that once source text for every function is materialized.

It passes the relevant test262 tests (it's failing here because it requires a minor non-breaking change on the parser side, which hasn't been released to Nuget yet). According to my manual tests, it also produces correct results for crazy dynamic cases like

eval('function fn() {}');
fn.toString()

or

(async () => {
  async function f() {}
  var AsyncFunction = f.constructor;
  var p1 = "a", p2 = "/*a */ b, c";
  var g = /* before */AsyncFunction(p1, p2, "return Promise.resolve(a+b+c) /* d */ //")/* after */; 
  console.log(g.toString());
  console.log(await g(1, 2, 3));
})()

If the PR is welcome, I'll finalize it soon.

[lahma]

I don't mind having the feature, some seem to want it. Should this be behind engine option and default to false?

[adams85]

If you ask me, I'd rather do it the opposite way: this is the expected behavior per spec, so it shouldn't really surprise anyone. We could offer an option to opt-out for those who want the original behavior for some reason.

As a matter of fact, this option is kind of in place already as FunctionToStringHandler takes precedence in the current implementation.

Performance doesn't seem like a concern either as the feature is implemented in the most unobtrusive way possible, it has practically zero overhead in execution time:

Jint.Benchmark.EngineComparisonBenchmark

Diff	Method	FileName	Mean	Error	Allocated
Old	Jint	array-stress	4,880.426 μs	535.6403 μs	7375.95 KB
New			4,938.106 μs (+1%)	564.0625 μs	7375.95 KB (0%)
Old	Jint	dromaeo-3d-cube	13,952.082 μs	2,027.4208 μs	6452.84 KB
New			14,026.564 μs (+1%)	385.8361 μs	6453.13 KB (0%)
Old	Jint	droma(...)odern [22]	13,965.460 μs	173.5485 μs	5961.02 KB
New			14,086.022 μs (+1%)	183.6659 μs	5961.31 KB (0%)
Old	Jint	dromaeo-core-eval	2,671.576 μs	88.8974 μs	1342.63 KB
New			2,656.229 μs (-1%)	792.9672 μs	1342.82 KB (0%)
Old	Jint	droma(...)odern [24]	2,682.818 μs	369.2471 μs	1341.6 KB
New			2,720.592 μs (+1%)	108.2627 μs	1341.79 KB (0%)
Old	Jint	dromaeo-object-array	33,915.158 μs	2,907.8592 μs	96765.05 KB
New			32,461.839 μs (-4%)	10,175.1483 μs	96765.24 KB (0%)
Old	Jint	droma(...)odern [27]	33,840.658 μs	4,398.4353 μs	96765.79 KB
New			34,199.724 μs (+1%)	10,104.2520 μs	96765.97 KB (0%)
Old	Jint	droma(...)egexp [21]	204,142.800 μs	277,588.0228 μs	170474.5 KB
New			173,740.267 μs (-15%)	480,651.9765 μs	169559.47 KB (-1%)
Old	Jint	droma(...)odern [28]	176,038.967 μs	225,061.3871 μs	165437.6 KB
New			180,598.533 μs (+3%)	334,234.0092 μs	164292.48 KB (-1%)
Old	Jint	droma(...)tring [21]	228,110.650 μs	137,962.1279 μs	1304410.85 KB
New			228,521.917 μs (0%)	84,231.1604 μs	1304402.61 KB (0%)
Old	Jint	droma(...)odern [28]	227,456.017 μs	16,460.4063 μs	1304371.45 KB
New			231,120.000 μs (+2%)	178,480.1623 μs	1304301.85 KB (0%)
Old	Jint	droma(...)ase64 [21]	29,455.272 μs	1,628.7930 μs	3780.37 KB
New			29,761.508 μs (+1%)	414.2663 μs	3780.51 KB (0%)
Old	Jint	droma(...)odern [28]	37,180.871 μs	6,129.2827 μs	3780.88 KB
New			34,807.456 μs (-6%)	3,102.5169 μs	3781.02 KB (0%)
Old	Jint	evaluation	15.821 μs	0.8582 μs	34.15 KB
New			16.131 μs (+2%)	0.8501 μs	34.16 KB (0%)
Old	Jint	evaluation-modern	16.004 μs	0.8976 μs	34.38 KB
New			15.745 μs (-2%)	3.7471 μs	34.39 KB (0%)
Old	Jint	linq-js	1,499.484 μs	614.6211 μs	1285.57 KB
New			1,306.322 μs (-13%)	93.3401 μs	1286.91 KB (0%)
Old	Jint	minimal	3.596 μs	4.3819 μs	17.09 KB
New			2.914 μs (-19%)	0.5037 μs	17.09 KB (0%)
Old	Jint	stopwatch	235,069.667 μs	52,165.8581 μs	72705.09 KB
New			226,450.100 μs (-4%)	30,575.0868 μs	72705.19 KB (0%)
Old	Jint	stopwatch-modern	293,589.450 μs	69,044.6314 μs	72921.33 KB
New			277,459.650 μs (-5%)	67,190.9670 μs	72921.42 KB (0%)

Numbers are a bit suprising here and there but probably that's because it was run using short jobs. The extra footprint resulting from referencing the original full source code also looks negligible compared to the total memory usage.

That being said, either way is fine by me. I'll leave the decision up to you.

[lahma]

Thanks for the analysis, you make a strong point so seems that new behavior being the default is the way to go 👍🏻

[adams85]

Cool, I'll finish this up soon!

[adams85]

Okay, I think it's ready now.

I also made an improvement to FunctionToStringHandler:

There are special cases where the IFunction node doesn't contain the necessary info to create the source text. So I changed the logic to pass the right node to the FunctionToStringHandler callback.

This is, however, a behavioral breaking change. Let me know if you don't agree with it. Otherwise, it would probably a good idea to mention it in the release notes.

[lahma]

Awesome, thank you again for a top-quality contribution! The breaking change is for quite advanced API so I wouldn't be that concerned 👍🏻