API: Make item `Visibility` visible in the API #294

xFrednet · 2023-10-18T20:28:32Z

Designing a sensible Visibility representation is surprisingly hard. I had to update the function names several times, and still feel like they can be improved. There is also still one util function missing, which requires the implementation of #242 first. I've left a corresponding comment in the PR.

I think this implementation will cover 99% of the current use cases. This PR also updates the not_using_has_span_trait lint and enables it by default inside the marker_api crate

Closes: #26

Veetaha

I'll take a closer look on weekends, didn't review everything yet.

marker_api/src/ast/item.rs

Veetaha · 2023-10-21T13:55:06Z

marker_api/src/ast/item.rs

+ /// Returns `true` if the item is declared as `pub(...)` with a path in brackets
+ /// that defines the scope, where the item is visible.
+ pub fn is_pub_with_path(&self) -> bool {
+ matches!(self.kind, VisibilityKind::Path(_) | VisibilityKind::Crate(_))
+ }


I'd expect that pub_with_path wouldn't include pub(crate) and only check for pub(in ...), so the name is a bit confusing. I'm not sure wether this method would be useful. Maybe if there was pub_in_path that checked only for Path visibility kind, that would be more obvious

marker_api/src/ast/item.rs

Veetaha · 2023-10-21T14:38:42Z

marker_api/src/ast/item.rs

+ ///
+ /// See [`Visibility::module_id`] to get the `ItemId`, even if the item or
+ /// field uses the default visibility.
+ pub fn pub_with_path_module_id(&self) -> Option<ItemId> {


This again looks weird due to the mixture of syntactic and semantic properties of the visibility.

I think if we split the representation into ast and sem there would be only a method on sem type:

sem::Visibility::visibility_root() -> Option<ItemId>: returns the ItemId of the root module this visibility level allows access in; returns None if the visibility is pub

And there wouldn't be such a method for ast::Visibility. I think it would be more straightforward to just expose the syntactic VisibilityKind to them, and the users would then handle all possible syntactic flavors of the visibility they want to check for.

The VisibilityKind in the ast module would look like this:

enum VisibilityKind { Pub, PubIn(Path), PubCrate, /// There is no distinction between `Default/DefautPub` because in the synatctic representation /// we only care if the visibility was explicitly specified or not Default, }

Btw. syn uses the term "inherited" visibility instead of "default". In general, it would be cool if marker's AST representation followed syn's naming and concepts if possible, because it would make it easier to adapt to marker for anyone who already wrote proc macros. But this is not strictly required (subjectively "default" visibility reads more intuitive to me than "inherited").

Maybe I'm dreaming here, and rustc doesn't provide the original syntax form of the visibility, in which case it would be a bummer.

UPD: I noticed the module_id method only after writing this comment.

marker_rustc_driver/src/conversion/marker/ast/item.rs

marker_uilints/src/lib.rs

xFrednet · 2023-10-23T22:09:42Z

This is still WIP, so far I've only applied the easy changes and skimmed over your suggestions. I'll answer properly soon. Thank you for the review!

xFrednet · 2023-11-11T17:35:38Z

Okay, I've rebased. Only the last two commits are new. I've now split the visibility into a semantic and ast representation, as discussed previously. Sorry that it took so long, to get this fixed 😅 Hope you have a beautiful weekend :D

I say as the CI fails... Oh what fun, let's hope it's fixed now :)

Veetaha

Consider making the design a bit more minimalistic but obvious as suggested in my comment #294 (comment).

marker_api/src/sem/item.rs

Veetaha · 2023-11-12T15:49:34Z

marker_api/src/sem/item.rs

+ /// // Returns `false` since the visibility is not restricted
+ /// fn example_in_root() {}
+ /// ```
+ pub fn is_pub_crate(&self) -> bool {


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.

Suggested change

pub fn is_pub_crate(&self) -> bool {

pub fn is_crate_scoped(&self) -> bool {

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 if scope resolves to the current crate no matter what syntax was used)

is_pub() -> bool { self.scope().is_none() }

is_default() -> bool

scope() -> Option<ItemId> (renamed module_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 just vis.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 exclude pub they will use !is_default && !is_pub() which would leave them with pub(crate), pub(super)andpub(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 on ast::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 👍

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 😅

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?

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. HirIds 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.

xFrednet · 2023-11-13T22:50:57Z

Okay, this should hopefully be it. Thank you for the excellent review. I think this very is way better, than my initial proposal :)

marker_api/src/sem/item.rs

Veetaha · 2023-11-13T23:52:54Z

marker_api/src/sem/item.rs

+ /// // Returns `false` since the visibility is not restricted
+ /// fn example_in_root() {}
+ /// ```
+ pub fn is_pub_crate(&self) -> bool {


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?

Veetaha

LGTM. Designing is hard, but things eventually settle 👍

xFrednet added C-enhancement Category: New feature or request A-api Area: Stable API labels Oct 18, 2023

xFrednet added this to the v0.4.0 milestone Oct 18, 2023

xFrednet mentioned this pull request Oct 19, 2023

Rustc: Librarify marker_rustc_driver #271

Merged

xFrednet force-pushed the 026-visibility-would-be-useful branch from 9843a2f to a52ab04 Compare October 19, 2023 08:51

Veetaha reviewed Oct 19, 2023

View reviewed changes

marker_api/src/ast/item.rs Outdated Show resolved Hide resolved

Veetaha reviewed Oct 21, 2023

View reviewed changes

xFrednet marked this pull request as draft October 23, 2023 22:09

xFrednet added 8 commits November 11, 2023 18:26

API: Make item Visibility visible in the API

ae52638

Lints: Test visibility in not_using_has_span_trait and update docs

93b2b22

Doc: Update changelog

adac30d

API: Update item struct size test

b8701a9

Chore: First PR comments

38e41ea

uilints: compile on stable again

cc00307

Chore: Second PR comments

b386d7b

API: Split ast::Visibility into a semantic and ast IR

26976f9

xFrednet force-pushed the 026-visibility-would-be-useful branch from 536b532 to 7f1f58f Compare November 11, 2023 17:34

Rustc: Fix local_crate_mod after rebase

fd7f151

xFrednet force-pushed the 026-visibility-would-be-useful branch from 7f1f58f to fd7f151 Compare November 11, 2023 17:36

Veetaha reviewed Nov 12, 2023

View reviewed changes

API: Address reivew comments <3

482447c

xFrednet marked this pull request as ready for review November 13, 2023 22:50

Veetaha reviewed Nov 13, 2023

View reviewed changes

Veetaha approved these changes Nov 13, 2023

View reviewed changes

API: Use extensive matching

eb84144

xFrednet enabled auto-merge November 14, 2023 09:10

xFrednet added this pull request to the merge queue Nov 14, 2023

Merged via the queue into rust-marker:master with commit 4cbd4fa Nov 14, 2023
23 checks passed

xFrednet deleted the 026-visibility-would-be-useful branch November 14, 2023 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Make item `Visibility` visible in the API #294

API: Make item `Visibility` visible in the API #294

xFrednet commented Oct 18, 2023 •

edited

Loading

Veetaha left a comment

Veetaha Oct 21, 2023 •

edited

Loading

Veetaha Oct 21, 2023 •

edited

Loading

xFrednet commented Oct 23, 2023

xFrednet commented Nov 11, 2023 •

edited

Loading

Veetaha left a comment •

edited

Loading

Veetaha Nov 12, 2023 •

edited

Loading

xFrednet Nov 13, 2023

Veetaha Nov 13, 2023

xFrednet Nov 14, 2023

xFrednet commented Nov 13, 2023

Veetaha Nov 13, 2023

Veetaha left a comment •

edited

Loading

	pub fn is_pub_crate(&self) -> bool {
	pub fn is_crate_scoped(&self) -> bool {

API: Make item Visibility visible in the API #294

API: Make item Visibility visible in the API #294

Conversation

xFrednet commented Oct 18, 2023 • edited Loading

Veetaha left a comment

Choose a reason for hiding this comment

Veetaha Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

Veetaha Oct 21, 2023 • edited Loading

Choose a reason for hiding this comment

xFrednet commented Oct 23, 2023

xFrednet commented Nov 11, 2023 • edited Loading

Veetaha left a comment • edited Loading

Choose a reason for hiding this comment

Veetaha Nov 12, 2023 • edited Loading

Choose a reason for hiding this comment

xFrednet Nov 13, 2023

Choose a reason for hiding this comment

Veetaha Nov 13, 2023

Choose a reason for hiding this comment

xFrednet Nov 14, 2023

Choose a reason for hiding this comment

xFrednet commented Nov 13, 2023

Veetaha Nov 13, 2023

Choose a reason for hiding this comment

Veetaha left a comment • edited Loading

Choose a reason for hiding this comment

API: Make item `Visibility` visible in the API #294

API: Make item `Visibility` visible in the API #294

xFrednet commented Oct 18, 2023 •

edited

Loading

Veetaha Oct 21, 2023 •

edited

Loading

Veetaha Oct 21, 2023 •

edited

Loading

xFrednet commented Nov 11, 2023 •

edited

Loading

Veetaha left a comment •

edited

Loading

Veetaha Nov 12, 2023 •

edited

Loading

Veetaha left a comment •

edited

Loading