WIP: rename refactor

[aviatesk]

Purpose

Implement rename refactoring command:

• rename local binding
• rename global bindings (module-based)

Approach

• use MacroTools.jl for easy tokenizing and code replacing
• for module-based global refactoring, CSTParser-based static file detection is used:
  • since our Revise-like module file detection will easily fail with the refactorings, which breaks precompilation cache
• global refactoring are designed to work only when it's called on the definition place
• first tries local refactoring, and then move to global refactoring if needed
• rename refactoring on fields is all suppressed, since not every code is statically typed and so I think we would end up with lots of false positive

Task / Achievements

• local refactor
• module-based global refactor
• graceful messages
  • notice the need of refactoring in the parent module if the refactored binding is imported/overloaded
  • report back error message
  • notice refactored files
  • show context
• handle edge cases
  • suppress keyword refactoring
  • suppress field refactoring
  • handle global refactoring that are not called at a definition place
  • handle global refactoring on an unsaved editor
  • handle global refactoring on files without write permission detected
  • global refactoring with dot accessor like Atom.isdebugging(), Base.countlines(args...)
  • global refactoring on macro
• test

[aviatesk]

xref: JunoLab/atom-julia-client#641
xref: JunoLab/atom-ink#236

[aviatesk]

Here are some edge cases I've noticed but still I'm not sure about how to handle.

local refactoring

If we have the code like:

function outer()
  val = 10
  function inner(val)
    2 * val
  end
  return inner(val)
end

and we refactor on outer's val, the inner's vals are also refactored, even though we may want to only refactor outers.
This is obviously because the inner function is included in the outer function scope, but I still not found any good way to handle this case with MacroTools's source walk.
As an alternative, this case would be easily solved if we can directly manipulate CSTParser's expressions and then reconstruct the source appropriately. But I'm not sure even if it's possible.

global refactoring

If a module contains code like:

function fun() end

function other()
  fun = 10
end

and we rename fun to fun2, then the binding fun in other will also be renamed to fun2. This is because I intentionally looks just for identifier symbols (like fun), not for something more complicated like call sites (the code here), since in Julia every binding is an object, and we can have a code like map(fun, itr).
Current behavior can be annoying, but at least doesn't break code. If we just looks for call sites, then we may have false negatives, which would break the code. So imho the current behavior may suffice.

[aviatesk]

I agree the logic inside renamerefactor would look messy, and please feel free to ask me if you find anything weird -- I probably miss some edge cases.

Atom.jl/src/refactor.jl

Lines 18 to 77 in 86b3b2c

	function renamerefactor(
	old, full, new, path,
	column = 1, row = 1, startrow = 0, context = "",
	mod = "Main",
	)
	# catch keyword renaming
	iskeyword(old) && return Dict(:warning => "Keywords can't be renamed: `$old`")
	mod = getmodule(mod)
	head = first(split(full, '.'))
	headval = getfield′(mod, head)
	# catch field renaming
	head ≠ old && !isa(headval, Module) && return Dict(
	:warning => "Rename refactoring on a field isn't available: `$obj.$old`"
	)
	expr = CSTParser.parse(context)
	items = toplevelitems(expr, context)
	ind = findfirst(item -> item isa ToplevelBinding, items)
	bind = ind === nothing ? nothing : items[ind].bind
	# local rename refactor if `old` isn't a toplevel binding
	if islocalrefactor(bind, old)
	try
	refactored = localrefactor(old, new, path, column, row, startrow, context, expr)
	return isempty(refactored) ?
	# NOTE: global refactoring not on definition, e.g.: on a call site, will be caught here
	Dict(:info => contextdescription(old, mod, context)) :
	Dict(
	:text => refactored,
	:success => "_Local_ rename refactoring `$old` ⟹ `$new` succeeded"
	)
	catch err
	return Dict(:error => errdescription(old, new, err))
	end
	end
	# global rename refactor if the local rename refactor didn't happen
	try
	kind, desc = globalrefactor(old, new, mod, expr)
	# make description
	if kind === :success
	val = getfield′(mod, full)
	moddesc = if (headval isa Module && headval ≠ mod) ||
	(applicable(parentmodule, val) && (headval = parentmodule(val)) ≠ mod)
	moduledescription(old, headval)
	else
	""
	end
	desc = join(("_Global_ rename refactoring `$mod.$old` ⟹ `$mod.$new` succeeded.", moddesc, desc), "\n\n")
	end
	return Dict(kind => desc)
	catch err
	return Dict(:error => errdescription(old, new, err))
	end
	end

[codecov]

Codecov Report

Merging #203 into master will decrease coverage by 6.47%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
- Coverage   55.18%   48.71%   -6.48%     
==========================================
  Files          42       42              
  Lines        2399     2184     -215     
==========================================
- Hits         1324     1064     -260     
- Misses       1075     1120      +45

Impacted Files	Coverage Δ
src/debugger/stepper.jl	1.64% <ø> (+0.21%)	⬆️
src/Atom.jl	100% <ø> (ø)	⬆️
src/refactor.jl	0% <0%> (ø)
src/docs.jl	0% <0%> (-75%)	⬇️
src/display/markdown.jl	33.33% <0%> (-33.34%)	⬇️
src/debugger/debugger.jl	28.57% <0%> (-11.43%)	⬇️
src/display/view.jl	60% <0%> (-5%)	⬇️
src/outline.jl	65.51% <0%> (-3.45%)	⬇️
src/static/static.jl	89.87% <0%> (-1.16%)	⬇️
src/debugger/breakpoints.jl	0% <0%> (ø)	⬆️
... and 15 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97483de...8c5f32d. Read the comment docs.

[aviatesk]

#208, #209, #210 need to be merged in order to finish up this PR (this only thing remain is to add tests)

[pfitzseb]

Both of these edge cases should be avoidable by checking each new local scope for assignments that shadow a global, right? We already should have the logic for that.

[pfitzseb]

We'll also need to think about proper UIs for all of this, especially the global refactors. I'd love to reuse find-and-replaces pane, but that doesn't seem to be possible.

So we could/should build one ourselves and ideally make it reuseable for something like this.

[aviatesk]

We'll also need to think about proper UIs for all of this, especially the global refactors. I'd love to reuse find-and-replaces pane, but that doesn't seem to be possible.

So we could/should build one ourselves and ideally make it reuseable for something like this.

Yeah, would be great to have, and I'm willing to implement that.
To me it seems better to start with implementing the "Find reference" feature, and then enable the global rename-refactor within that as an additional feature.

[aviatesk]

Needs MikeInnes/SourceWalk.jl#1 to be merged for this to work.