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

WIP: rename refactor #203

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

WIP: rename refactor #203

wants to merge 5 commits into from

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Oct 23, 2019

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
Copy link
Member Author

@aviatesk aviatesk mentioned this pull request Oct 23, 2019
13 tasks
@aviatesk
Copy link
Member Author

aviatesk commented Oct 23, 2019

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
Copy link
Member Author

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
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member Author

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

@aviatesk aviatesk mentioned this pull request Oct 29, 2019
@aviatesk aviatesk force-pushed the avi/renamerefactor branch 3 times, most recently from 7d168ed to 32d6004 Compare November 1, 2019 18:04
@aviatesk aviatesk changed the title rename refactor WIP: rename refactor Nov 4, 2019
@pfitzseb
Copy link
Member

pfitzseb commented Nov 8, 2019

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
Copy link
Member

pfitzseb commented Nov 8, 2019

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
Copy link
Member Author

aviatesk commented Nov 9, 2019

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.

Copy link
Member Author

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

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

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.

2 participants