Skip to content

Port "Direct function calls" from Gren #41

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

Merged
merged 2 commits into from
Mar 20, 2025
Merged

Port "Direct function calls" from Gren #41

merged 2 commits into from
Mar 20, 2025

Conversation

Janiczek
Copy link
Collaborator

@Janiczek Janiczek commented Mar 14, 2025

Quick Summary: Ports gren-lang/compiler#251 to Elm/Lamdera, cf. Discord

  • Ported the code
  • Built the code
  • Tested the code
    • Direct functions get generated
    • Direct functions get used
  • Minimized the diff wrt. Lamdera.& and other fun stuff

@Janiczek
Copy link
Collaborator Author

Janiczek commented Mar 14, 2025

For Elm program

module Main exposing (main)

import Html

fn1 x =
    "fn1"

fn2 x y =
    "fn2"

fn3 x y z =
    "fn3"

main =
    Html.div []
        [ Html.text (fn1 0)
        , Html.text (fn2 0 0)
        , Html.text (fn3 0 0 0)
        , let fn2_ = fn2 0 in Html.text (fn2_ 0)
        , let fn3_ = fn3 0 in Html.text (fn3_ 0 0)
        , let fn3_ = fn3 0 0 in Html.text (fn3_ 0)
        ]

this is the diff:

 var $author$project$Main$fn1 = function (x) {
 	return 'fn1';
 };
-var $author$project$Main$fn2 = F2(
-	function (x, y) {
-		return 'fn2';
-	});
+var $author$project$Main$fn2$ = function (x, y) {
+	return 'fn2';
+};
+var $author$project$Main$fn2 = F2($author$project$Main$fn2$);
-var $author$project$Main$fn3 = F3(
-	function (x, y, z) {
-		return 'fn3';
-	});
+var $author$project$Main$fn3$ = function (x, y, z) {
+	return 'fn3';
+};
+var $author$project$Main$fn3 = F3($author$project$Main$fn3$);
 var $author$project$Main$main = A2(
 	$elm$html$Html$div,
 	_List_Nil,
 	_List_fromArray(
 		[
 			$elm$html$Html$text(
 			$author$project$Main$fn1(0)),
 			$elm$html$Html$text(
-			A2($author$project$Main$fn2, 0, 0)),
+			$author$project$Main$fn2$(0, 0)),
 			$elm$html$Html$text(
-			A3($author$project$Main$fn3, 0, 0, 0)),
+			$author$project$Main$fn3$(0, 0, 0)),
 			function () {
 			var fn2_ = $author$project$Main$fn2(0);
 			return $elm$html$Html$text(
 				fn2_(0));
 		}(),
 			function () {
 			var fn3_ = $author$project$Main$fn3(0);
 			return $elm$html$Html$text(
 				A2(fn3_, 0, 0));
 		}(),
 			function () {
 			var fn3_ = A2($author$project$Main$fn3, 0, 0);
 			return $elm$html$Html$text(
 				fn3_(0));
 		}()
 		]));

@Janiczek
Copy link
Collaborator Author

Similarly for type constructors:

type Type
    = Ctor0
    | Ctor1 ()
    | Ctor2 () ()
    | Ctor3 () () ()

    let
        c0 = Ctor0
        c1 = Ctor1 ()
        c2 = Ctor2 () ()
        c3 = Ctor3 () () ()
    in

->

	var c3 = $author$project$Main$Ctor3$(_Utils_Tuple0, _Utils_Tuple0, _Utils_Tuple0);
	var c2 = $author$project$Main$Ctor2$(_Utils_Tuple0, _Utils_Tuple0);
	var c1 = $author$project$Main$Ctor1(_Utils_Tuple0);
	var c0 = $author$project$Main$Ctor0;

@Janiczek
Copy link
Collaborator Author

Janiczek commented Mar 14, 2025

Here's another possibly tricky case (Can.Unbox and Opt.VarBox for newtype optimization)

type Newtype a
    = N a

type Newtype2 a b
    = N2 a b

    let
        x = N ()
        y = N
        z = y ()
        n2 = N2 () ()
        n2a = N2 ()
        n2b = n2a ()
    in
	var y = $author$project$Main$N;
	var z = y(_Utils_Tuple0);
	var x = $author$project$Main$N(_Utils_Tuple0);
	var n2a = $author$project$Main$N2(_Utils_Tuple0);
	var n2b = n2a(_Utils_Tuple0);
	var n2 = $author$project$Main$N2$(_Utils_Tuple0, _Utils_Tuple0);

The Gren PR does things slightly differently here (generateCall around VarBox), and I'm not sure if I'm not missing some edge case. I couldn't make it misbehave, but if you'd feel safer with Robin's code more closely copied, I can do that.

in
case graph ! global of
-- @LAMDERA
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question not related to this PR: @LAMDERA annotations are there to help distinguish what was added in Lamdera but some of the annotations are for Lamdera features, while others (like this one) is for improvements to the Elm compiler. Would it be useful to have 2 annotations, one for Lamdera features and one for "improvements"? In order for these improvements to be easier to backport (to the Elm compiler, to elm-dev, ...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case it might be better we start a differences.md to track key modifications by PR or commit, rather than introduce varying annotations which still won’t give clear per-feature insight.

In this PR for example we’ve got a somewhat wide-sweeping and invasive that variable introduction, which is not the usual case for mods we’ve made so far. Hopefully that doesn't cause major pains when merging future upstream Elm changes.

But I can see Martin's tried to adhere to our code injection principles so seems he decided to draw the line somewhere re tagging/alternative implementation vs direct modification which is fair enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@supermario I wonder whether what I did was a good tradeoff. Since that new argument needed to be threaded through everything, it seems like I'd have to make a new implementation of all those functions, mostly same but with the new arg, and dispatch to it in some parent call. That didn't seem worth it.

@supermario
Copy link
Member

Looking good! Would be good if the diff examples posted could become actual tests, I think there should already be a test precedence of compiling Elm code and checking things in the output.

@Janiczek
Copy link
Collaborator Author

@supermario Added!

Screenshot 2025-03-19 at 21 17 34

@Janiczek Janiczek marked this pull request as ready for review March 19, 2025 20:21
Copy link
Member

@supermario supermario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thanks @Janiczek !

Happy for this to be merged, over to you @MartinSStewart .

@MartinSStewart
Copy link
Member

I tested the changes on a local project and it seems to work. Though I didn't test it with --optimize enabled. Seems like something we'll need to test after this is merged so we can try it out on some Lamdera apps?

@Janiczek
Copy link
Collaborator Author

Janiczek commented Mar 20, 2025

@MartinSStewart FWIW the added test is running with _optimize = True, you can see it in the expectTextContains jsOutput "n1 = 0" case where it unwraps the newtype and changes () to 0.

@MartinSStewart MartinSStewart merged commit 7bb3b2f into lamdera:lamdera-next Mar 20, 2025
@MartinSStewart
Copy link
Member

Sounds like everyone is happy with it so I'll press the merge button then!

@MartinSStewart
Copy link
Member

Last step is to just test this in production (maybe there's some weird interaction between Lamdera production stuff and this PR though I can't think of what it would be).

Eitherway, I think you've done your part well @Janiczek. Thanks for porting this!

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.

4 participants