analyzer-user-var-fusion directive seems to be ignored in nightly

[danielo515]

Hello.
I'm testing the 4.3.0-rc.1 build, and it seems to be ignoring the analyzer-user-var-fusion compiler directive.

I have the following directives set:

-D analyzer-module
-D analyzer-optimize
-D analyzer-user-var-fusion
-D analyzer-const_propagation
-D analyzer-copy_propagation
-D analyzer-local_dce
-D analyzer-fusion
-D analyzer-purity_inference

-D lua-vanilla
-D luajit

Take a look at the diff of the generated Lua code to understand the difference:

[RblSb]

Maybe this is extern function? Try to make it @:pure or provide reproducible sample.

[danielo515]

Maybe this is extern function? Try to make it @:pure or provide reproducible sample.

No, this happens with plain user functions. This is working with previous stable version of haxe. I can give you this particular example, but I can also point you to a repository where the current compilation does the right thing so you can also test it agains nightly?

[Simn]

I'll need a reproducible example here.

[danielo515]

reproduction steps:

1. clone this repo https://github.com/danielo515/haxe-nvim
2. Run haxelib install libs.hxml && haxe build.hxml
3. stage the generated code, or rename it or however you want
4. comment out the tink_core in libs.hxml, is not needed for this to compile and will fail with development version
5. Repeat step 2 with development branch (tested with 4.3.0-rc.1)
6. Diff the generated code with the previous one
   The output should look something like this

diff --git a/danielo_nvim/lua/danielo_nvim/kickstart.lua b/danielo_nvim/lua/danielo_nvim/kickstart.lua
index 54bd4ea..a504a11 100644
--- a/danielo_nvim/lua/danielo_nvim/kickstart.lua
+++ b/danielo_nvim/lua/danielo_nvim/kickstart.lua
@@ -1,3 +1,4 @@
+-- Generated by Haxe 4.3.0-rc.1
 local _hx_hidden = {__id__=true, hx__closures=true, super=true, prototype=true, __fields__=true, __ifields__=true, __class__=true, __properties__=true, __fields__=true, __name__=true}
 
 _hx_array_mt = {
@@ -172,8 +173,8 @@ local function _hx_new(prototype)
 end
 
 function _hx_field_arr(obj)
-    res = {}
-    idx = 0
+    local res = {}
+    local idx = 0
     if obj.__fields__ ~= nil then
         obj = obj.__fields__
     end
@@ -680,7 +681,8 @@ String.prototype.split = function(self,delimiter)
       end;
     end;
     if (newidx ~= nil) then 
-      ret:push(_G.string.sub(self, idx, newidx - 1));
+      local match = _G.string.sub(self, idx, newidx - 1);
+      ret:push(match);
       idx = newidx + #delimiter;
     else
       ret:push(_G.string.sub(self, idx, #self));
@@ -823,7 +825,7 @@ __kickstart__Kickstart_Kickstart_Fields_.main = function()
   __kickstart_Neodev.setup();
   __kickstart_Mason.setup();
   __kickstart_Fidget.setup();
-  __kickstart_Cmp.setup(({mapping = __kickstart_Cmp.mapping.preset.insert(
+  local mapping = __kickstart_Cmp.mapping.preset.insert(
 {
     ['<C-d>'] = __kickstart_Cmp.mapping.scroll_docs(-4),
     ['<C-f>'] = __kickstart_Cmp.mapping.scroll_docs(4),
@@ -851,7 +853,8 @@ __kickstart__Kickstart_Kickstart_Fields_.main = function()
       end
     end, { 'i', 's' }),
   }
-      ), snippet = ({expand = function(args) 
+      );
+  __kickstart_Cmp.setup(({mapping = mapping, snippet = ({expand = function(args) 
     __kickstart_Luasnip.lsp_expand(args.body);
   end}), sources = ({({name = "luasnip"}),({name = "nvim_lsp"})})}));
   __kickstart_MasonLspConfig.setup_handlers(({function(server_name) 
@@ -909,7 +912,8 @@ end
 
 __vim__TableTools_TableTools_Fields_.new = {}
 __vim__TableTools_TableTools_Fields_.concat = function(tableA,tableB) 
-  do return vim.list_extend(vim.list_extend(({}), tableA), tableB) end;
+  local result = vim.list_extend(({}), tableA);
+  do return vim.list_extend(result, tableB) end;
 end
 
 __vim__VimTypes_LuaArray_Impl_.new = {}
@@ -977,6 +981,14 @@ end;
 
 _hx_array_mt.__index = Array.prototype
 
+if package.loaded.luv then
+  _hx_luv = _G.require("luv");
+else
+  _hx_luv = {
+    run=function(mode) return false end,
+    loop_alive=function() return false end
+  }
+end
 local _hx_static_init = function()
   __lua_StringMap.tnull = ({});
   
@@ -986,5 +998,8 @@ local _hx_static_init = function()
 end
 
 _hx_static_init();
-_G.xpcall(__kickstart__Kickstart_Kickstart_Fields_.main, _hx_error)
+_G.xpcall(function() 
+  __kickstart__Kickstart_Kickstart_Fields_.main();
+  _hx_luv.run();
+end, _hx_error)
 return _hx_exports

[Simn]

Unless there's a simpler way to reproduce, I won't have time to look into this for the next release.

[danielo515]

Unless there's a simpler way to reproduce, I won't have time to look into this for the next release.

How can I make the reproduction simpler? The issue happens with the above mentioned steps. If you have any other requirements please lete know

[danielo515]

What about this minimal reproduction?
https://try.haxe.org/#a161fa3f

This is the haxe code:

extern class Vim {
	static function list_extend<T>(target:Array<T>, source:Array<T>):Array<T>;
}

class Test {
	function concat<T>(tableA:Array<T>, tableB:Array<T>):Array<T> {
		final result = Vim.list_extend([], tableA);
		return Vim.list_extend(result, tableB);
	}

	static function main() {
		trace("Haxe is great!");
	}
}

If you compile it (you can do it in the playground link) with the latest nightly you get this:

// Generated by Haxe 4.3.0-rc.1+b24ae68
(function ($global) { "use strict";
class Test {
	concat(tableA,tableB) {
		let result = Vim.list_extend([],tableA);
		return Vim.list_extend(result,tableB);
	}
	static main() {
		console.log("Test.hx:12:","Haxe is great!");
	}
}
class haxe_iterators_ArrayIterator {
	constructor(array) {
		this.current = 0;
		this.array = array;
	}
	hasNext() {
		return this.current < this.array.length;
	}
	next() {
		return this.array[this.current++];
	}
}
{
}
Test.main();
})({});

This is the interesting part:

	concat(tableA,tableB) {
		let result = Vim.list_extend([],tableA);
		return Vim.list_extend(result,tableB);
	}

If you compile it with 4.2.5 instead, this is what you get:

	concat(tableA,tableB) {
		return Vim.list_extend(Vim.list_extend([],tableA),tableB);
	}

[Simn]

That looks like #10432.

[danielo515]

So this is the expected behaviour now?

[Simn]

Perhaps #10432 was a bit too brutal: Instead of refusing fusion in general, we should just treat extern field access as a read/write change for the interference report. I'll check if this explodes horribly.

[Simn]

@RblSb Could you check the test change to #9943 in the linked commit? The analyzer would allow fusion for this:

var c = el.offset + issues_Issue9943.foo;
issues_Issue9943.values(a,b,c);

This looks safe to me because no matter what el.offset does, there's nothing between the definition and usage of c that could be affected by it.

I adjusted the test by making the values function dynamic, in which case el.offset could theoretically rebind it.

[RblSb]

Everything can explode with extern call, but i don't see el.offset bad caching after your change, so DOM api is not doomed for us yet. You can close this issue if you're done here.

[RblSb]

Only one diff regression i found on my js project:

var el = e.target;
var ratio = Math.min(max / el.videoWidth,max / el.videoHeight);
- el.style.width = "" + el.videoWidth * ratio + "px";
- el.style.height = "" + el.videoHeight * ratio + "px";
+ var tmp = "" + el.videoWidth * ratio;
+ el.style.width = tmp + "px";
+ var tmp = "" + el.videoHeight * ratio;
+ el.style.height = tmp + "px";

In other cases there is less temp vars. This one is right from extern logic?

[Simn]

Hmm, I don't really see how this change could cause more temp vars, given that it's a less severe restriction now. Could you isolate that case out? I'd like to take a look at what happens there.

[RblSb]

class Main {
	static function main() {
		final el:js.html.VideoElement = null;
		el.style.width = '${el.videoWidth}px';
		el.style.height = '${el.videoHeight}px';
	}
}

[Simn]

Thanks!

This might be related to my favorite issue of all time with the analyzer temp-varring the string concat past the el.style.width and then refusing to fuse it back. The latter is the correct behavior here, the problem is that the order of operations is changed during the initial transformation.

I guess I wasn't exactly right when I said the restriction was less severe: It is actually stricter for the expression that are checked as part of the interference report. Still, the change itself is fine, and I'll see if I can construct a test case for it.

[Simn]

I'll not make this change for 4.3, it's too risky and the actual problem is not that big of a deal anyway. I'd like to deal with #7765 first in order to avoid strange reordering problems, which should then have good synergy with the change I want to make here.