Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
API: Make item
Visibility
visible in the API #294API: Make item
Visibility
visible in the API #294Changes from 2 commits
ae52638
93b2b22
adac30d
b8701a9
38e41ea
cc00307
b386d7b
26976f9
fd7f151
482447c
eb84144
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think people will be confused if we name this method after the special
pub(crate)
syntax when this method covers more than just that.Given the amount of info we get from rustc (the lost of AST context) consider this number of methods instead:
is_crate_scoped() -> bool
(true ifscope
resolves to the current crate no matter what syntax was used)is_pub() -> bool { self.scope().is_none() }
is_default() -> bool
scope() -> Option<ItemId>
(renamedmodule_id
)In other words I'd like to make it explicit that the semantic visibility no longer represents the original syntax that was used.
You can also notice that in the list of methods I suggested there isn't
pub_with_path_module_id
. I think it's not needed because it's expressible with justvis.scope().filter(|_| !vis.is_default())
or similar.If the user wants to check if the visibility was specified (non-default), they would use
!is_default()
. If they want to excludepub
they will use!is_default && !is_pub()
which would leave them withpub(crate),
pub(super)and
pub(in ...)` syntaxes.Maybe to have checks for AST syntax like
is_pub_crate, is_pub_super, is_pub_in_path
etc. (which would be available onast::Visibility
we could just re-parse the text snippet from the span manually? However, I wouldn't do it in this PR to just land this chunk of work and have something good enough 👍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.
I really like the suggested names. I'll make the changes. We should be able to reparse at least part of the text snippet. I haven't worked with Rustc's lexer and parser yet, which is partially why I'm avoiding such solutions 😅
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.
Re-parsing the text snippet is more of a hack. I wonder how clippy fights with this? Maybe other kinds of lint passes such as
EarlyLintPass
have the necessary ast info?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.
Clippy usually just checks if things are public, and that's it. In Clippy we don't really have lints that span over more than a module and those that do don't require the visibility AFAIK.
For the
EarlyLintPass
, I've just checked and that one still has the path [1]. The big problem with this approach is that nodes from the AST, are hard to connect to the nodes from the HIR, as the AST doesn't have a unique ID for everything.HirId
s are also structured differently.The current rustc interface has covered all of Clippy's use cases so far. So I hope that this is enough for Marker.