Pretty Print Expanding Function Definitions Inside Calls

[Aws0mee]

I found this issue when I used pretty print with a call to timer.Create. The call looked something like this:

timer.Create( "name", 30, 0, function()
    if true then return end
end)

Upon hitting pretty print it turned into the following which is clearly not "pretty"

timer.Create(
    "name",
    30,
    0,
    function()
        if true then return end
    end
)

Some more examples where this behavior happens:

func(function() if true then return end end)
func(function() anything() end)

turns into:

func(
    function()
        if true then return end
    end
)

func(
    function()
        anything()
    end
)

Oddly, enough the following 2 function calls seem to work perfectly fine:

func(function() return true end)

func(function() return anything() end)

turns into:

func(function() return true end)
func(function() return anything() end)

[FPtje]

The pretty printer keeps track of whether something should be printed as single line or multiline. When it decides it should be multiline, the entire statement will be printed as such. The reason for that is that the closing parenthesis should be placed on a new line, unindented.

I'm afraid doing this any tighter would be much more complicated, so closing as won't fix.

[Aws0mee]

The pretty printer keeps track of whether something should be printed as single line or multiline. When it decides it should be multiline, the entire statement will be printed as such. The reason for that is that the closing parenthesis should be placed on a new line, unindented.

I'm afraid doing this any tighter would be much more complicated, so closing as won't fix.

Fair enough, maybe you could add some additional checks for cases like this so the entire call doesn't get expanded just because of the function. Also, maybe change what would cause the statement to be outputted as multiline, because I don't see why a simple function containing another function call or a single if statement would need to be expanded like this.

[Aws0mee]

Also, I'd like to point out this bug seems to be quite widespread. It seems like a recent change caused this, because I never remember seeing this before and now it seems to happen quite frequently. I was writing a script to whitelist certain weapons to certain players and pretty printing gave me the following results.

hook.Add( "PlayerCanPickupWeapon", "NoPistolGiveFists", function( ply, wep )
    if ply:SteamID() == "" then return end
    local class = wep:GetClass()
    if weps[class] then return false end
end )

hook.Add(
    "PlayerCanPickupWeapon",
    "NoPistolGiveFists",
    function(ply, wep)
        if ply:SteamID() == "" then return end
        local class = wep:GetClass()
        if weps[class] then return false end
    end
)

[FPtje]

That was indeed part of the fix that added forcing multiline support. Actually the former was the bug, since the fact that the anonymous function was printed as multiline was forgotten at the point of the arguments, meaning that the function call still thought it was single line.

Fair enough, maybe you could add some additional checks for cases like this so the entire call doesn't get expanded just because of the function. Also, maybe change what would cause the statement to be outputted as multiline, because I don't see why a simple function containing another function call or a single if statement would need to be expanded like this.

The logic now is: a function is single line if it doesn't have any statements, and its return value is also not multiline. I could change that to not being multiline if there is at most one statement with that statement being a small if-statement, but it feels like the logic would get very hard to understand.

I guess what may be surprising here, is that returns and statements are counted differently. You can have a simple return, but not a simple statement instead of a return. Language wise a return is different from a statement, because it must be the last thing in a block. Conceptually though, it lives on the same level.

Maybe I can change the logic to one simple statement or a simple return 🤔

[Aws0mee]

Maybe, I'm not entirely sure what the best fix would be but all I know is that the current state is sub-optimal for sure. Was this multiline feature just added or something? If so, is there a specific case it was added to fix?

[FPtje]

No, it was always there, but the recent addition of the format: multiline also came with the above mentioned fix. That fix was needed for that feature as well to work.

[Aws0mee]

No, it was always there, but the recent addition of the format: multiline also came with the above mentioned fix. That fix was needed for that feature as well to work.

I see, well it looks like the fix unintentionally made other cases worse. ;(

[FPtje]

I've started work here: https://github.com/FPtje/GLuaFixer/pull/164

The change is a rather big rework of things, because I found glualint to go in an infinite loop. I've decided to take a more drastic approach to prevent that from happening again in the future. One benefit of it should be that comments are rendered nicer as well.

You can already scroll to the test inputs and outputs to see how the WIP PR renders things. Note that there are still some known bugs that are clearly shown in these tests.

[Aws0mee]

I've started work here: #164

The change is a rather big rework of things, because I found glualint to go in an infinite loop. I've decided to take a more drastic approach to prevent that from happening again in the future. One benefit of it should be that comments are rendered nicer as well.

You can already scroll to the test inputs and outputs to see how the WIP PR renders things. Note that there are still some known bugs that are clearly shown in these tests.

Sounds good, I appreciate all the good work. I'll try to test within the next few days.

[FPtje]

I've made some more changes. I think things are looking pretty good now. What do you think?

In the current version of the PR, your code prints like this:

-- These should not be marked as multiline, because there is only one statement
-- in the body of each example.
timer.Create("name", 30, 0, function() if true then return end end)
timer.Create("name", 30, 0, function() return end)
timer.Create("name", 30, 0, function() end)
timer.Create("name", 30, 0)
func(function() if true then return end end)
func(function() anything() end)
-- These, on the other hand, _should_ be marked as multiline
timer.Create(
    "name",
    30,
    0,
    function()
        -- comment here
        if true then return end
    end
)

timer.Create(
    "name",
    30,
    0,
    function()
        a = 1
        return
    end
)

[Aws0mee]

I've made some more changes. I think things are looking pretty good now. What do you think?

In the current version of the PR, your code prints like this:

-- These should not be marked as multiline, because there is only one statement
-- in the body of each example.
timer.Create("name", 30, 0, function() if true then return end end)
timer.Create("name", 30, 0, function() return end)
timer.Create("name", 30, 0, function() end)
timer.Create("name", 30, 0)
func(function() if true then return end end)
func(function() anything() end)
-- These, on the other hand, _should_ be marked as multiline
timer.Create(
    "name",
    30,
    0,
    function()
        -- comment here
        if true then return end
    end
)

timer.Create(
    "name",
    30,
    0,
    function()
        a = 1
        return
    end
)

I still don't think it makes sense to expand the entire function call. I think my examples would still get the entire function call expanded because of this. I think only expanding the function would make the most sense since it is called "pretty print" after all and putting each function argument on a new line is definitely not pretty.

[FPtje]

I understand that you want this for the timer call, but it's really hard to extract some consistent logic out of that. If glualint were to consistently do things the way you suggest, the timer code would look nice, but the DarkRP example code would look something like this:

TEAM_CHIEF = DarkRP.createJob("Civil Protection Chief", {color = Color(20, 20, 255, 255), model = "models/player/combine_soldier_prisonguard.mdl", description = [[The Chief is the leader of the Civil Protection unit.
        Coordinate the police force to enforce law in the city.
        Hit a player with arrest baton to put them in jail.
        Bash a player with a stunstick and they may learn to obey the law.
        The Battering Ram can break down the door of a criminal, with a warrant for his/her arrest.
        Type /wanted <name> to alert the public to the presence of a criminal.
        Type /jailpos to set the Jail Position]], weapons = {"arrest_stick", "unarrest_stick", "weapon_deagle2", "stunstick", "door_ram", "weaponchecker"}, command = "chief", max = 1, salary = GAMEMODE.Config.normalsalary * 1.67, admin = 0, vote = false, hasLicense = true, chief = true, NeedToChangeFrom = TEAM_POLICE, ammo = {
        ["pistol"] = 60,
    }, category = "Civil Protection",})

Closer to your example, it already becomes awkward when there are arguments after the function:

timer.Create("name", 30, 0,
    function()
        -- comment here
        if true then return end
    end, true, "some string", function(a, b)
        print(3)
        return false
    end)

The ideal pretty printer balances pretty output with consistent and predictable logic. In the PR, things are multiline or they are not. The partial multiline of your request is harder to define logically.

Note also that glualint does not have the information to decide based on line length. Other printers, like black switch between single line and multiline based on the length of the original line. Glualint tries to prevent lines becoming too long by having some rules on when something should be multiline. It's not as good as black (which also cannot 100% guarantee a line length), but it does a pretty decent job. Especially after this update. Trying to do function arguments on one line as much as possible is going to make it way more likely that things are printed as long single lines.

Lastly, I'm a bit hurt by you schooling me about the definition of a pretty printer. I worked quite hard on this, and your original examples have demonstrably improved by the changes in the PR. I understand that this may still not be entirely what you want, but I don't appreciate unwarranted sassy responses.

[FPtje]

Closing, as the issue is fixed to the extent that I think has the best balance.

[Aws0mee]

I understand that you want this for the timer call, but it's really hard to extract some consistent logic out of that. If glualint were to consistently do things the way you suggest, the timer code would look nice, but the DarkRP example code would look something like this:

TEAM_CHIEF = DarkRP.createJob("Civil Protection Chief", {color = Color(20, 20, 255, 255), model = "models/player/combine_soldier_prisonguard.mdl", description = [[The Chief is the leader of the Civil Protection unit.
        Coordinate the police force to enforce law in the city.
        Hit a player with arrest baton to put them in jail.
        Bash a player with a stunstick and they may learn to obey the law.
        The Battering Ram can break down the door of a criminal, with a warrant for his/her arrest.
        Type /wanted <name> to alert the public to the presence of a criminal.
        Type /jailpos to set the Jail Position]], weapons = {"arrest_stick", "unarrest_stick", "weapon_deagle2", "stunstick", "door_ram", "weaponchecker"}, command = "chief", max = 1, salary = GAMEMODE.Config.normalsalary * 1.67, admin = 0, vote = false, hasLicense = true, chief = true, NeedToChangeFrom = TEAM_POLICE, ammo = {
        ["pistol"] = 60,
    }, category = "Civil Protection",})

Closer to your example, it already becomes awkward when there are arguments after the function:

timer.Create("name", 30, 0,
    function()
        -- comment here
        if true then return end
    end, true, "some string", function(a, b)
        print(3)
        return false
    end)

The ideal pretty printer balances pretty output with consistent and predictable logic. In the PR, things are multiline or they are not. The partial multiline of your request is harder to define logically.

Note also that glualint does not have the information to decide based on line length. Other printers, like black switch between single line and multiline based on the length of the original line. Glualint tries to prevent lines becoming too long by having some rules on when something should be multiline. It's not as good as black (which also cannot 100% guarantee a line length), but it does a pretty decent job. Especially after this update. Trying to do function arguments on one line as much as possible is going to make it way more likely that things are printed as long single lines.

Lastly, I'm a bit hurt by you schooling me about the definition of a pretty printer. I worked quite hard on this, and your original examples have demonstrably improved by the changes in the PR. I understand that this may still not be entirely what you want, but I don't appreciate unwarranted sassy responses.

I appreciate your work on this project, and I don't want you to take what I said the wrong way. I wasn't trying to be sassy or school you on the definition of a pretty printer, I was just trying to explain why this issue was relevant. I was trying to restate the goal of the function which is to make the code more visually appealing and explain that the current implementation was drastically worse than the previous one in many cases. I want to apologize because I was not trying to be rude and would not intentionally disrespect you especially after using so much of your work.

Moreover, I can test soon to see if your changes improved my test cases. I use the online glua linter very frequently, and this bug did not seem to be an edge-case and instead seemed to be quite frequent which is why I originally thought it was so important to report. Is the online version updated with this new PR? Also, what cases was the multiline feature intended to improve on? I'm still very curious about that.

[FPtje]

The PR is not on the online version yet. I can update that later today.

The cases this multiline intended to improve on are mostly functions with many arguments, like the DarkRP one down above. The original fix was genuinely a fix of a bug where multiline information would not propagate past arguments. That didn't seem to lead to syntax errors, but on some edge cases, quite odd looking code. To be specific, the closing parenthesis would be in an odd position.

This PR reduces how quickly it decides to go multiline with functions. Before, having one statement in the function caused it to go multiline. Now, it goes multiline if you have more than one statement or returns, or if the one statement/return is multiline. That's what fixes your examples in the original post of this issue.

Another great improvement in this PR is with comments. In function calls, comments would often just get collected and printed after the entire call. Now, Glualint nicely places them above or next to arguments.

[Aws0mee]

The PR is not on the online version yet. I can update that later today.

The cases this multiline intended to improve on are mostly functions with many arguments, like the DarkRP one down above. The original fix was genuinely a fix of a bug where multiline information would not propagate past arguments. That didn't seem to lead to syntax errors, but on some edge cases, quite odd looking code. To be specific, the closing parenthesis would be in an odd position.

This PR reduces how quickly it decides to go multiline with functions. Before, having one statement in the function caused it to go multiline. Now, it goes multiline if you have more than one statement or returns, or if the one statement/return is multiline. That's what fixes your examples in the original post of this issue.

Another great improvement in this PR is with comments. In function calls, comments would often just get collected and printed after the entire call. Now, Glualint nicely places them above or next to arguments.

Sounds good, thanks for the clarification. I'll test whenever it gets updated.

[FPtje]

The changes are now on glualint-web

[Aws0mee]

The changes are now on glualint-web

Alright I did some testing, and it looks like the output is perfect for short functions like the following example:

timer.Simple(0, function() if IsValid(ply) then ply:SetPos(pos) end end)

The new version doesn't change it at all which is perfect whereas the previous version would give this output

timer.Simple(
    0,
    function()
        if IsValid(ply) then
            ply:SetPos(pos)
        end
    end
)

However, with slightly longer functions it still expands everything which looks extremely odd in most cases. I'll pull another example from the darkrp repo to show.

The following code is extremely clean and easy to read, I believe the old version of the glua linter would have formatted it similarly to this.

hook.Add("PlayerInitialSpawn", "Arrested", function(ply)
    if not arrestedPlayers[ply:SteamID()] then return end
    local time = GAMEMODE.Config.jailtimer
    -- Delay the actual arrest by a single frame to allow
    -- the player to initialise
    timer.Simple(0, function()
        -- In case the timer ended right this tick
        if not IsValid(ply) or not arrestedPlayers[ply:SteamID()] then return end

        ply:arrest(time)
    end)
    DarkRP.notify(ply, 0, 5, DarkRP.getPhrase("jail_punishment", time))
end)

However, pretty printing will give us the following output which is much more difficult to read.

hook.Add(
    "PlayerInitialSpawn",
    "Arrested",
    function(ply)
        if not arrestedPlayers[ply:SteamID()] then return end
        local time = GAMEMODE.Config.jailtimer
        -- Delay the actual arrest by a single frame to allow
        -- the player to initialise
        timer.Simple(
            0,
            function()
                -- In case the timer ended right this tick
                if not IsValid(ply) or not arrestedPlayers[ply:SteamID()] then return end
                ply:arrest(time)
            end
        )

        DarkRP.notify(ply, 0, 5, DarkRP.getPhrase("jail_punishment", time))
    end
)

[FPtje]

I don't agree that it's harder to read, but I do understand that this is not how most Lua looks. The thing is that it's hard to fine a set of rules. As I said, you don't want a function call to look like this:

TEAM_CHIEF = DarkRP.createJob("Civil Protection Chief", {color = Color(20, 20, 255, 255), model = "models/player/combine_soldier_prisonguard.mdl", description = [[The Chief is the leader of the Civil Protection unit.
        Coordinate the police force to enforce law in the city.
        Hit a player with arrest baton to put them in jail.
        Bash a player with a stunstick and they may learn to obey the law.
        The Battering Ram can break down the door of a criminal, with a warrant for his/her arrest.
        Type /wanted <name> to alert the public to the presence of a criminal.
        Type /jailpos to set the Jail Position]], weapons = {"arrest_stick", "unarrest_stick", "weapon_deagle2", "stunstick", "door_ram", "weaponchecker"}, command = "chief", max = 1, salary = GAMEMODE.Config.normalsalary * 1.67, admin = 0, vote = false, hasLicense = true, chief = true, NeedToChangeFrom = TEAM_POLICE, ammo = {
        ["pistol"] = 60,
    }, category = "Civil Protection",})

This is precisely what I would happen if I were to take the shortest path of having your example code format the way you indicated. Going back to the old version doesn't work either because that would mess up the comments, and remove the -- format: multiline feature.

So to really have this stuff work nicely, I would need some set of rules. The current set of rules is as follows (high level):

• A function is multiline if there are more than one statement/return in it, or if the statements/return value are multiline
• If an argument of a function call is multiline, then the entire function call is considered multiline
• If there are comments above or next to arguments, then the entire function call is also considered multiline. This prevents code from being put after comments, causing syntax errors after format
• A multiline function call means that every argument is printed to a new line, with indentation = previous_indentation + 1

So to get your desired result without getting the bad consequence result, I would need some set of rules that achieves that. Preferably not too convoluted. Maybe something around "if the last argument to a function call is an anonymous function..."?

Instead of multiline/not multiline, do we need a third state semimultiline? Where

not multiline = foo(1,2, function() end) multiline =

foo(
  1,
  2,
  function() end
)

and semi-multiline is

foo(1,2, function()

end)

Those are the balances that glualint needs to strike. Previously, function calls pretended to always be single while some arguments were multiline. That was genuinely some bug that happened to look alright most of the time. Now that bug is fixed, it looks more consistent, but not exactly how you like it. Now we have to think about what we really want.

[Aws0mee]

I don't agree that it's harder to read, but I do understand that this is not how most Lua looks. The thing is that it's hard to fine a set of rules. As I said, you don't want a function call to look like this:

TEAM_CHIEF = DarkRP.createJob("Civil Protection Chief", {color = Color(20, 20, 255, 255), model = "models/player/combine_soldier_prisonguard.mdl", description = [[The Chief is the leader of the Civil Protection unit.
        Coordinate the police force to enforce law in the city.
        Hit a player with arrest baton to put them in jail.
        Bash a player with a stunstick and they may learn to obey the law.
        The Battering Ram can break down the door of a criminal, with a warrant for his/her arrest.
        Type /wanted <name> to alert the public to the presence of a criminal.
        Type /jailpos to set the Jail Position]], weapons = {"arrest_stick", "unarrest_stick", "weapon_deagle2", "stunstick", "door_ram", "weaponchecker"}, command = "chief", max = 1, salary = GAMEMODE.Config.normalsalary * 1.67, admin = 0, vote = false, hasLicense = true, chief = true, NeedToChangeFrom = TEAM_POLICE, ammo = {
        ["pistol"] = 60,
    }, category = "Civil Protection",})

This is precisely what I would happen if I were to take the shortest path of having your example code format the way you indicated. Going back to the old version doesn't work either because that would mess up the comments, and remove the -- format: multiline feature.

So to really have this stuff work nicely, I would need some set of rules. The current set of rules is as follows (high level):

* A function is multiline if there are more than one statement/return in it, or if the statements/return value are multiline

* If an argument of a function call is multiline, then the entire function call is considered multiline

* If there are comments above or next to arguments, then the entire function call is also considered multiline. This prevents code from being put after comments, causing syntax errors after format

* A multiline function call means that every argument is printed to a new line, with `indentation = previous_indentation + 1`

So to get your desired result without getting the bad consequence result, I would need some set of rules that achieves that. Preferably not too convoluted. Maybe something around "if the last argument to a function call is an anonymous function..."?

Instead of multiline/not multiline, do we need a third state semimultiline? Where

not multiline = foo(1,2, function() end) multiline =

foo(
  1,
  2,
  function() end
)

and semi-multiline is

foo(1,2, function()

end)

Those are the balances that glualint needs to strike. Previously, function calls pretended to always be single while some arguments were multiline. That was genuinely some bug that happened to look alright most of the time. Now that bug is fixed, it looks more consistent, but not exactly how you like it. Now we have to think about what we really want.

I was thinking about it for a little, and I'm not exactly sure how to make it work perfectly in both cases. I was thinking to maybe only multiline it if there is a table in the function call. Alternatively, maybe you could add a setting that could be accessible on the glualinter web since I believe there is already an option in the standalone gluafixer to enable/disable the entire multiline functionality. The whole multiline feature seems to work really well in some cases like the ones you pointed out with darkrp jobs and such. However, from my experience it seems like the cases where I would use the multiline functionality are pretty rare, and the cases where I would want it disabled (like when writing an anonymous function in a hook or timer) are very frequent. So, maybe if there can't be a good solution to determine which mode should be used then it could be best to just let the user decide.

[FPtje]

There is no option to disable multiline I'm afraid, as that would require a different implementation of dealing with comments. So I'm afraid it's not as simple as "let the user decide".

[Aws0mee]

There is no option to disable multiline I'm afraid, as that would require a different implementation of dealing with comments. So I'm afraid it's not as simple as "let the user decide".

Wouldn't it be possible to have a setting to revert to the old logic?

[Aws0mee]

Also, maybe it could be possible to only expand functions calls which are abnormally long or contain abnormally long tables in them.

[FPtje]

Wouldn't it be possible to have a setting to revert to the old logic?

No, it would not, because that reintroduce the previously mentioned bugs.

Also, maybe it could be possible to only expand functions calls which are abnormally long or contain abnormally long tables in them.

Deciding things based on line length would be nice, but also something that would need to be built.