haxe icon indicating copy to clipboard operation
haxe copied to clipboard

Invalid handling of curly brackets in `<platform>.Syntax.code` methods

Open RealyUniqueName opened this issue 2 years ago • 4 comments

js.Syntax.code('var d={exports:{}};');

generates (with Haxe 4.2)

var d={exports:{};

but it should generate (and it does with Haxe 4.0)

var d={exports:{}};

RealyUniqueName avatar Feb 02 '22 11:02 RealyUniqueName

So there's this code:

| Str.Delim a :: Str.Delim b :: tl when a = b ->
	i := !i + 2;
	f_string a;
	loop tl

This looks like some dorky escape mechanism. What confuses me is that this change was made 8 years ago, so I don't know what changed between 4.0 and development. A bisect would be quite interesting, but I can't be arsed to deal with all the dependency crap that comes with it.

Not sure what to do here because unless I'm totally misreading the implementation, this kind of double-escaping is working as intended. I don't know who intended it like that and why, but I'm reluctant to randomly change it...

Simn avatar Mar 01 '22 08:03 Simn

Ah, this comes from https://github.com/HaxeFoundation/haxe/commit/68f6e0face85af1983ba4c60032a92c0902dc605

Before, the code wouldn't even go through interpolate_code if there were no additional arguments, so the problem was hidden. We can reproduce the issue in 4.0 as well by doing js.Syntax.code('var d={exports:{}};', 1);, which triggers that code path.

We can restore the 4.0 behavior, but I'm not even sure if it's worth it. I've removed the Hotfix milestone though because this isn't a recent regression.

Simn avatar Mar 01 '22 09:03 Simn

I'd like to rpoperly fix the aforementioned ocaml code instead of just avoiding the bug because it affects multiple targets.

RealyUniqueName avatar Mar 01 '22 10:03 RealyUniqueName

But that requires declaring it a bug, while it looks very intentional that {{ becomes { and }} becomes }. The original implementation had that already.

Frankly, I think this kind of escaping is questionable, but changing it in a minor release could be questionable as well.

Simn avatar Mar 01 '22 10:03 Simn

@kLabz I'm interested in your opinion on this one.

Simn avatar Mar 24 '23 09:03 Simn

I have removed the regression label because if anything it's an old regression, which also means that this isn't particularly urgent.

Simn avatar Mar 30 '23 12:03 Simn