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

RFC: module validation check within CSTParser-based module traverse #215

Merged
merged 13 commits into from
Nov 8, 2019

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Nov 3, 2019

main purpose

fixes module escaping issues within CSTPraser-based module traverse

  • fixes that it can enter into submodules when searched from the parent module
  • fixes that it can include items outside of modules in the entry file (e.g. isdebugging in Atom module when we traverse Atom.JunoDebugger)

That would make goto and rename refactors (#203) more "correct", and may also help JunoLab/Juno.jl#411.

sub note

I also included changes that enables gotos within unsaved editors

@codecov
Copy link

codecov bot commented Nov 3, 2019

Codecov Report

Merging #215 into master will increase coverage by 0.24%.
The diff coverage is 86.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   40.22%   40.47%   +0.24%     
==========================================
  Files          40       40              
  Lines        1857     1868      +11     
==========================================
+ Hits          747      756       +9     
- Misses       1110     1112       +2
Impacted Files Coverage Δ
src/Atom.jl 100% <ø> (ø) ⬆️
src/outline.jl 75.86% <0%> (+4.89%) ⬆️
src/static/static.jl 90.12% <100%> (+0.12%) ⬆️
src/modules.jl 86.27% <100%> (-0.14%) ⬇️
src/utils.jl 86.36% <100%> (+0.15%) ⬆️
src/goto.jl 80.4% <87.71%> (-1.42%) ⬇️
src/static/toplevel.jl 96.15% <91.66%> (-3.85%) ⬇️

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 d7af2d5...e175233. Read the comment docs.

@aviatesk
Copy link
Member Author

aviatesk commented Nov 4, 2019

Updated the description.

@aviatesk
Copy link
Member Author

aviatesk commented Nov 4, 2019

[ ] fixes that it can include items outside of modules in the entry file (e.g. isdebugging in Atom module when we traverse Atom.JunoDebugger)

I first tried to fix this by introducing a flag argument for toplevelitems that indicates whether it's in a target module (and update that appropriately in each downstream), but I found that would increase code complexity somewhat much more -- and I think creating a struct ToplevelScope like LocalScope would also end up introducing the same kind of complexity.

I may try to implement that anyway, but also want to hear better ideas if there are.

- fixes two broken test cases
@aviatesk aviatesk changed the title module validation check within CSTParser-based module traverse RFC: module validation check within CSTParser-based module traverse Nov 4, 2019
@aviatesk
Copy link
Member Author

aviatesk commented Nov 4, 2019

[ ] fixes that it can include items outside of modules in the entry file (e.g. isdebugging in Atom module when we traverse Atom.JunoDebugger)

I first tried to fix this by introducing a flag argument for toplevelitems that indicates whether it's in a target module (and update that appropriately in each downstream), but I found that would increase code complexity somewhat much more -- and I think creating a struct ToplevelScope like LocalScope would also end up introducing the same kind of complexity.

I may try to implement that anyway, but also want to hear better ideas if there are.

Okay, I've implemented the fix along with this way within the latest commit.
The code comes to look more complex, but at least imo functions APIs are acceptable.

Now I've fixed all the issues that I'm trying to address within this PR, and I would welcome review or comment :)

# ignore toplevel items outside of `mod` when `path` is an entry file
entrypath, _ = moduledefinition(m)
inmod = path != entrypath
items = toplevelitems(text; mod = stripdotprefixes(mod), inmod = inmod)
Copy link
Member Author

Choose a reason for hiding this comment

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

Now there are at most 2 calls of toplevelitems in updateeditor handler, since we don't want module escaping in outline but do want in symbols update.

@pfitzseb
Copy link
Member

pfitzseb commented Nov 8, 2019

Looks good, nice work!
There will of course still be cases this doesn't handle properly (like imported bindings), but that's why we have the runtime stuff as a fallback.

@pfitzseb pfitzseb merged commit 7142894 into master Nov 8, 2019
@pfitzseb pfitzseb deleted the avi/entermodule branch November 8, 2019 10:52
@aviatesk
Copy link
Member Author

aviatesk commented Nov 8, 2019

@pfitzseb
Do you have any alternative idea instead of the new function API with those keyword arguments ? I'm still feeling like they're rather complicated, so I really want to hear if you have the other idea.

aviatesk added a commit to JunoLab/atom-julia-client that referenced this pull request Nov 8, 2019
@pfitzseb
Copy link
Member

pfitzseb commented Nov 8, 2019

No, but I'll let you know if I figure out something better ;)

aviatesk added a commit that referenced this pull request Nov 9, 2019
aviatesk added a commit that referenced this pull request Nov 22, 2019
aviatesk added a commit that referenced this pull request Nov 22, 2019
aviatesk added a commit that referenced this pull request Nov 22, 2019
aviatesk added a commit that referenced this pull request Nov 24, 2019
aviatesk added a commit that referenced this pull request Dec 1, 2019
aviatesk added a commit that referenced this pull request Dec 14, 2019
aviatesk added a commit that referenced this pull request Feb 9, 2020
aviatesk added a commit that referenced this pull request Feb 17, 2020
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