feature request: refactor to expand and collapse expressions

[sewbacca]

It would be nice to be able to pull out repeated expressions or expand an expression on demand.
So one could jump back and forth between for example:

function love.draw()
    local w, h = love.graphics.getDimensions()
    love.graphics.setColor(1, 0, 0)
    love.graphics.circle('fill', 50, 50, 22)
    love.graphics.setColor(1, 1, 0)
    love.graphics.circle('fill', 50, 100, 22)
    love.graphics.setColor(0, 1, 0)
    love.graphics.circle('fill', 50, 150, 22)
end

and

local g = love.graphics

function love.draw()
    local w, h = g.getDimensions()
    g.setColor(1, 0, 0)
    g.circle('fill', 50, 50, 22)
    g.setColor(1, 1, 0)
    g.circle('fill', 50, 100, 22)
    g.setColor(0, 1, 0)
    g.circle('fill', 50, 150, 22)
end

It would also be nice to do this for local variables, so that the change is only applied where all the nessecary variables are visible (i.e. for required modules within a scope or long paths in arguments), so one can easily switch between

local function compileInfo(project)
    return {
        root = project.main,
        compiler = project.conf.toolchain == 'gnu' and 'gcc'
            or project.conf.toolchain == 'windows' and 'msvc'
            or project.conf.toolchain == 'clang' and 'clang'
    }
end

and

local function compileInfo(project)
    local c = project.conf
    return {
        root = project.main,
        compiler = c.toolchain == 'gnu' and 'gcc'
            or c.toolchain == 'windows' and 'msvc'
            or c.toolchain == 'clang' and 'clang'
    }
end

or

local function compileInfo(project)
    local t = project.conf.toolchain
    return {
        root = project.main,
        compiler = t == 'gnu' and 'gcc'
            or t == 'windows' and 'msvc'
            or t == 'clang' and 'clang'
	}
end

This is most usefull for long namespaced paths, but it may also be usefull to extract out some repeated constants or function calls and expand them again, so one can switch between:

local function copy(src, dst)
    for f in lfs.dir(src) do
        local src = io.open(src..package.config:sub(1, 1)..f, "rb")
        local dst = io.open(dst..package.config:sub(1, 1)..f, "wb")
    end
end

and

local DIR_SEP = package.config:sub(1, 1)

local function copy(src, dst)
    for f in lfs.dir(src) do
        local src = io.open(src..DIR_SEP..f, "rb")
        local dst = io.open(dst..DIR_SEP..f, "wb")
    end
end

Ideally one could chose the name of the pulled out expression via an prompt.

It's a long time since I had a good look at your codebase, so I wouldn't know where to start if I wanted to implement such a feature myself. I might be willing to try, if someone gives me a few pointers though.

[CppCXY]

Based on your description, should I extract table.insert as table_insert for multiple calls? Due to the existence of metamethods in Lua, the result of accessing a.b twice can actually be different, which presents some definitional difficulties. However, I don't oppose it if you are willing to try.

[sewbacca]

Yeah exactly. Most of the time metamethods are not a problem, so it is up to the programmer to decide, whether this rewrite is without sideeffects. It would als help to improve speed for performance critical modules, as you can localize math.sin to local sin = math.sin and reduce a table lookup. I would limit these kinds of rewrites only to one file though.

It may be ambiguous sometimes when replacing expressions if a local variable depends only on external functions, so trying to swap could lead to this chain of rewrites:

local function foobar()
    local lfs = require "lfs"
    lfs.rmdir "mydir"
end

to

local function foobar()
    require "lfs".rmdir "mydir"
end

to

local lfs = require "lfs"
local function foobar()
    lfs.rmdir "mydir"
end

One could solve that via selecting a scope to pull out the variable into, but I don't think this happens so often that it is necessary, so selecting the largest scope seems good enough.

Do you have some pointers for me and or files you think are going to be crucial in implementing this feature?

[CppCXY]

I can't give you any useful advice because I don't maintain this project. I only review some PRs or look at some issues. However, the implementation approach is definitely related to references of local and global variables.

[sparr]

Moving this comment over from my dupe issue where I was trying to implement this as a diagnostic. I'm not sure how automatically moving one direction, let alone back and forth like described by OP here, would work, but I expect that just identifying the expressions is a necessary first step, so I'll keep focusing on that.

Here's an example test case for my supposed diagnostic:

TEST [[
local a = { b = { c = 1, d = 2 } }
local e, f = a.<!b!>.c, a.<!b!>.d
]]

This is identifying two different accesses to a.b which could be cached in a local to improve performance.

I've started with the structure of core/diagnostics/unused-local.lua and am trying to make my way to being able to iterate over references to table-types fields of tables.

    guide.eachSourceType(ast.ast, 'table', function (source)
        if not source.parent or source.parent.type ~= 'tablefield' then
            return
        end

The above code gets me to what I think are my target objects, i.e. table typed objects that are themselves a tablefield. In this test case this produces just b as identified by source.parent.field[1], which seems like what I want. Now I need to find direct references to a.b like hasGet(loc) ... loc.ref is expecting. I can find references to a as source.parent.parent.parent.ref, and I can chase those down to b via source.parent.parent.parent.ref[n].next.field but that feels like I'm on the wrong track.

I'd appreciate some pointers if anyone more familiar with the parser object tree can chime in.

EDIT: I started out just looking for table typed tablefields, but based on OP's example it is clear that I should also be aiming for methods as well. I expect the solution will be similar, so I'm not focusing on that distinction too hard right now when I haven't gotten the first half working yet.