-
Notifications
You must be signed in to change notification settings - Fork 448
Change PrismRuby not to depend on hack that stores module nesting information to context.parent #1580
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
base: master
Are you sure you want to change the base?
Conversation
| # Need to update module parent chain to emulate Module.nesting. | ||
| # This mechanism is inaccurate and needs to be fixed. | ||
| container.parent = old_container | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RDoc::Parser::Ruby updates parent only for NormalClass. This information is used by includes/extends name resolve and also in CrossReference resolve.
This is one of the reason that HTML generated by RDoc::Parser::PrismRuby and RDoc::Parser::Ruby has huge diffs.
And there is no way to reduce diff except re-implementing many bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this part to
container.parent = old_container if !singleton && container.is_a?(RDoc::NormalClass)will minimize crossref difference of generated HTML between two parsers.
(but this change doesn't make sense. It's just re-implementing a bug)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you this some examples of the diffs caused by this? It's not entirely clear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really complicated. In RDoc::Parser::Ruby parser, parent chian is updated like this.
class A; class B; class C; class D; class E; end; end; end; end; end
# Parent chain is:
# F → E → D → C → B → A → toplevel
class A::B::C
# Method comment A::B B::C C::D D::E
# (B::C is not linked because RDoc only search A::B::C and toplevel)
def f; end
module D::E
end
end
# Parent chain is updated. This imitates module nesting
# E → C → toplevel (changed)
# B → A → toplevel, D → C (unchanged)
# `B::C` in method comment is not linked because it can't be found from CSame situation for modules. It's strange that parent chain is not updated. Maybe bug or workaround for something.
module M; module N; module O; module P; module Q; end; end; end; end; end
# Parent chain is:
# R → Q → P → O → N → M → toplevel
module M::N::O
# Method comment M::N N::O O::P P::Q
# All linked because RDoc searches M::N::O, M::N, M and toplevel
def f; end
endParent chain is emulating module nesting, used in crossref resolve, include/extend name resolve, superclass name resolve, etc.
Parent chain update is skipped for modules, so module parent seems to represent module owner. This inconsistency causes many bugs, but this also makes crossref work better in some cases.
PrismRuby parser updated both module and class to emulate module nesting, so crossref behavior got worse as a bugfix.
In this pull request, parent will no longer updated. Parent will exactly represent module/class owner, not module nesting.
As a result, B::C in the method comment will be linked too. It means crossref doesn't follow module nesting.
But I think it's OK, some comments already don't follow module nesting, and already linked.
class RDoc::TopLevel < RDoc::Context
##
# Returns the NormalClass "Object", creating it if not found.
# (NormalClass can't be resolved in this module nesting but we often write this kind of comment)
def object_class
end
end| RDoc::Parser::Ruby | PrismRuby(master) | PrismRuby (this PR) | |
|---|---|---|---|
| Linked classes | A::B C::D D::E |
A::B C::D D::E |
A::B B::C C::D D::E |
| Linked modules | M::N N::O O::P P::Q |
M::N O::P P::Q |
M::N N::O O::P P::Q |
|
🚀 Preview deployment available at: https://4f846e5d.rdoc-6cd.pages.dev (commit: 47e5e2d) |
| # Need to update module parent chain to emulate Module.nesting. | ||
| # This mechanism is inaccurate and needs to be fixed. | ||
| container.parent = old_container | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you this some examples of the diffs caused by this? It's not entirely clear to me.
| ## | ||
| # Find a module at a higher scope | ||
| # Tries to find a module at a higher scope. | ||
| # But parent is not always a higher module nesting scope, so the result is not correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add examples here?
Looks like it's for
module Foo
module Bar; end
module Baz; end # we're at this scope and we try to find Bar?
endBut in some cases this might not work either?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added.
There are three cases I know.
- PrismRuby stopped updating parent chain in this PR.
- There's a bug updating parent chain in RDoc::Parser::Ruby, (not updated when opening module)
- Module nesting will change with opening class, but parent chain can only represent one global module nesting.
class A::B::C
class D::E
# module nesting 1
include M
end
end
class A::B
class C::D::E
# module nesting 2
include N
end
end
# Only the last opened module nesting will be used to resolve crossref and include/extend4776a83 to
2ed7fac
Compare
…ormation to context.parent All constant/module/class paths are resolved by PrismRuby parser itself. Resolves include/extend module name before adding it because `Context#parent` will not have an information for delayed full-path name resolve.
2ed7fac to
47e5e2d
Compare
Closes #1291
All constant/module/class paths are resolved by PrismRuby parser itself.
Resolves include/extend module name before adding it because
Context#parentwill not have an information for delayed full-path name resolve.