On 12/05/2022 13:45, Jacek Caban wrote:
Hi Gabriel,
On 5/6/22 15:19, Gabriel Ivăncescu wrote:
On 06/05/2022 02:56, Jacek Caban wrote:
On 5/6/22 01:50, Jacek Caban wrote:
Honestly, I don't particularly care about toString, it's just the only builtin I could think off the top of my head that could be tested for this. Because right now we handle it in exec_source, which is only for interpreted functions... but it's a good test case of such behavior.
Do you think a fix like in 2/2 is more appropriate (also for quirks modes, but with null instead of undefined)? And perhaps fix whatever is causing toString to fail, which seems unrelated I guess.
That might mean moving it out of exec_source though.
Same story there, why do you think it's somehow related to detached scopes? The attached test fails with your patch.
I attached a wrong test, here is the right one.
Jacek
Because without handling detached scopes we actually *trigger the assertion* so it's quite bad. But that's just for toString, it's not really important. I've added a test with hasOwnProperty now, which *must* fail on detached scopes, but it doesn't currently (since they're js objects). They need special handling either way.
The attached test looks to me like a different issue: it passes null instead of undefined, which is an ES5 specific thing, and probably deserves a separate fix. It's not much of a problem either way.
I'm not convinced that it's unrelated, see below.
Note that we are *already* handling detached scopes in exec_source, so it's not like I'm adding custom code to deal with that separately; we already do. I'm just moving it out of exec_source because:
- Nothing in the (specified) spec says it's only for source functions.
- This will become a problem with proxy functions later (which is
where it's needed); I'd rather not duplicate that logic in them for no reason. 3) We are already handling it in exec_source "as a special case".
I'm trying to find fix for the attach problem and send it separately but honestly I can't see how such a fix would fix detached scopes at all.
The main problem of those patches is that they seem like random special cases fixing specific tests without enough understanding of the problem. Tests that I attached were meant to help you find the root cause of problems, but v2 is not really looking convincing yet. For example, do we really want to use builtin info class type for this? I'm attaching another test.
BTW, any time detached scopes need special treatment is a worrying sign. There is no such thing as detached scope in the spec. It's purely our implementation detail (it's in fact just an optimization). If they have observable differences in behaviour, that's alarming on its own.
Jacek
Right. The spec I referenced (but also the code in exec_source for 3rd edition) does state that it's replaced with null/undefined under certain conditions. One of them being not a reference. Since detached scopes aren't supposed to exist, doesn't that apply to them?
I don't know if builtin_info is the best way to handle it either, I was just moving the logic out of exec_source (which uses that), because this needs to happen for all function calls, not just interpreted functions, that's all.
Oh, and the reason I added the JS_GLOBAL workaround is to avoid potential regressions, for the moment. It will be completely removed when the JS global becomes window (which is how it should work, but need it to be a proper JS object first with prototype etc, so it will be a while).
I'm not looking to fix builtin functions specially here. The main motivation is for proxy functions later which depend on the same "logic", it just doesn't make sense to keep it in exec_source and specific to interpreted functions. But for now we only have builtin functions to test with, other than exec_source (since bind functions replace 'this' anyway, so they can't be tested).
Well I will look into it but tbh, fixing builtins (at least in jscript, not sure about mshtml) seems to require some sort of extra internal flag passed in call/apply when invoking, to ALL builtins. I don't think it's worth it. Maybe I can figure out a different way, but either way, that logic definitely needs to be moved out of exec_source, whether it's fixed differently or not...
Thanks, Gabriel