Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable fusion for impure externs #10432

Merged
merged 5 commits into from
Oct 27, 2021
Merged

Conversation

RblSb
Copy link
Member

@RblSb RblSb commented Oct 14, 2021

Pretty scary bug, i think.

There is 5 test failures:

unit.TestExceptions
  testExceptionStack: FAILURE .FFFFF.....
    line: 255, _without_ throws: {
	file : /home/runner/work/haxe/haxe/tests/unit/bin/js/-Dgithub-DLinux-Djs-classic/unit.js, 
	line : NaN, 
	method : stacksWithoutThrowLevel2
} is expected, but got {

}
    line: 255, _without_ throws: {
	file : /home/runner/work/haxe/haxe/tests/unit/bin/js/-Dgithub-DLinux-Djs-classic/unit.js, 
	line : NaN, 
	method : stacksWithoutThrowLevel2
} is expected, but got {

}
    line: 255, _without_ throws: {
	file : /home/runner/work/haxe/haxe/tests/unit/bin/js/-Dgithub-DLinux-Djs-classic/unit.js, 
	line : NaN, 
	method : stacksWithoutThrowLevel2
} is expected, but got {

}
    line: 255, _without_ throws: {
	file : /home/runner/work/haxe/haxe/tests/unit/bin/js/-Dgithub-DLinux-Djs-classic/unit.js, 
	line : NaN, 
	method : stacksWithoutThrowLevel2
} is expected, but got {

}
    line: 255, _without_ throws: {
	file : /home/runner/work/haxe/haxe/tests/unit/bin/js/-Dgithub-DLinux-Djs-classic/unit.js, 
	line : NaN, 
	method : stacksWithoutThrowLevel2
} is expected, but got {

}

Please review this test and tryHaxeStack js impl, because diff change seems like a fix for Haxe code:
Haxe tryHaxeStack function
And unit.js diff for her:

haxe_NativeStackTrace.tryHaxeStack = function(e) {
	if(e == null) {
		return [];
	}
	var oldValue = Error.prepareStackTrace;
	Error.prepareStackTrace = haxe_NativeStackTrace.prepareHxStackTrace;
+	var stack = e.stack;
	Error.prepareStackTrace = oldValue;
-	return e.stack;
+	return stack;
};

closes #9943

@Simn
Copy link
Member

Simn commented Oct 18, 2021

Change makes sense to me. @RealyUniqueName Could you check the failing tests?

@RealyUniqueName
Copy link
Member

@RblSb 30d639b fixes the issue. Merge it to your branch.

@RblSb
Copy link
Member Author

RblSb commented Oct 27, 2021

Diff for my SyncTube project (~9000 generated LoC). Looks fair for me. Can be improved with @:pure on extern classes with extern generators patches. Current html extern generator handles pure js stuff from Web IDLs btw, these idls just poor about pure stuff for now.

nodejs server:

-                       var events = JSON.parse(js_node_Fs.readFileSync(path,{ encoding : "utf8"}));
+                       var text = js_node_Fs.readFileSync(path,{ encoding : "utf8"});
+                       var events = JSON.parse(text);

-               res.end("File " + js_node_Path.relative(server_HttpServer.dir,filePath) + " not found.");
+               var rel = js_node_Path.relative(server_HttpServer.dir,filePath);
+               res.end("File " + rel + " not found.");

-       js_node_Fs.createReadStream(filePath,{ start : start, end : end}).pipe(res);
+       var videoStream = js_node_Fs.createReadStream(filePath,{ start : start, end : end});
+       videoStream.pipe(res);

-       var proxy = server_HttpServer.proxyRequest(StringTools.replace(req.url,"/proxy?url=",""),req,res,function(proxyReq) {
+       var url = StringTools.replace(req.url,"/proxy?url=","");
+       var proxy = server_HttpServer.proxyRequest(url,req,res,function(proxyReq) {

-       var proxy = (url1.protocol == "https:" ? js_node_Https.request : js_node_Http.request)({ host : url1.hostname, port : Std.parseInt(url1.port), path : url1.pathname + url1.search, method : req.method},function(proxyReq) {
+       var options = { host : url1.hostname, port : Std.parseInt(url1.port), path : url1.pathname + url1.search, method : req.method};
+       var request = url1.protocol == "https:" ? js_node_Https.request : js_node_Http.request;
+       var proxy = request(options,function(proxyReq) {

+               var isAdmin = this.config.localAdmins && req.socket.localAddress == ip;
                var client = new Client(ws,req,id,name,0);
-               client.setGroupFlag(ClientGroup.Admin,this.config.localAdmins && req.socket.localAddress == ip);
+               client.setGroupFlag(ClientGroup.Admin,isAdmin);

js client:

-               if(smilesBtn.classList.toggle("active")) {
+               var isActive = smilesBtn.classList.toggle("active");
+               if(isActive) {

-               if(client_Buttons.isElementEditable(e.target)) {
+               var target = e.target;
+               if(client_Buttons.isElementEditable(target)) {

        onKeyDown: function(e) {
-               switch(e.keyCode) {
+               var key = e.keyCode;
+               switch(key) {

-                       url = "" + protocol + "//" + $global.location.hostname + ":" + $global.location.port + url;
+                       var host = $global.location.hostname;
+                       var port = $global.location.port;
+                       url = "" + protocol + "//" + host + ":" + port + url;

-               this.setItemElementType(this.videoItemsEl.children[pos],this.videoList.items[this.videoList.pos].isTemp);
+               var el = this.videoItemsEl.children[pos];
+               this.setItemElementType(el,this.videoList.items[this.videoList.pos].isTemp);

-       return client_Settings.checkData(JSON.parse(client_Settings.storage.getItem("data")));
+       var data = JSON.parse(client_Settings.storage.getItem("data"));
+       return client_Settings.checkData(data);

-       trackEl.track.mode = "showing";
+       var track = trackEl.track;
+       track.mode = "showing";

@RblSb
Copy link
Member Author

RblSb commented Oct 27, 2021

Also without Meta.NoClosure check:

-               js_node_Fs.writeFileSync("" + crashesFolder + "/" + (DateTools.format(new Date(),"%Y-%m-%d_%H_%M_%S") + "-" + type) + ".json",JSON.stringify(data,null,"\t"));
+               var name = DateTools.format(new Date(),"%Y-%m-%d_%H_%M_%S") + "-" + type;
+               js_node_Fs.writeFileSync("" + crashesFolder + "/" + name + ".json",JSON.stringify(data,null,"\t"));

-               window.visualViewport.addEventListener("resize",function(e) {
+               var viewport = window.visualViewport;
+               viewport.addEventListener("resize",function(e) {

Without Meta.CoreApi check there is little more purity fails, don't know if it should be removed too.

@Simn Simn merged commit c50774b into HaxeFoundation:development Oct 27, 2021
@Simn
Copy link
Member

Simn commented Oct 27, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[js] analyzer-optimize can re-order api (extern) calls what can cause a mess
3 participants