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