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

Get the ItemId based on a given ast-path #93

Closed
xFrednet opened this issue Jan 14, 2023 · 2 comments · Fixed by #140
Closed

Get the ItemId based on a given ast-path #93

xFrednet opened this issue Jan 14, 2023 · 2 comments · Fixed by #140
Assignees
Labels
A-api Area: Stable API C-enhancement Category: New feature or request
Milestone

Comments

@xFrednet
Copy link
Member

During linting, a common pattern is to check for a specific type, method, or trait. In rustc these are identified using DefId/LocalDefId's or type relative checks for methods.

Marker currently doesn't provide a way to check for a specific item, as it only provides the ItemId and no further information. Ideally, the AstContext should provide a resolve method or something similar that takes a path like std::vec::Vec and returns the ItemId for comparison.


Clippy tries to avoid card coded paths in favor of diagnostic items. This makes sense in Clippy's context, especially when Clippy can just add the IDs to rustc types. See:

For Marker, this is not a viable option, as the rustc_diagnostic_item attribute is compiler intern. Even if marker would add a similar attribute, users might not always be able to add these to items in question.

Hard-coded paths have one major problem: The user known path might only be a reexport of items from a different module. std::iter::Iterator for instance, is actually located at std::iter::traits::iterator::Iterator. If we require the user to specify the absolute path, the lints would break if the items would be moved internally. If we accept the public path, we have to resolve the item, which could potentially be expensive, especially for modules with glob imports.

Ideally, I'd like to choose the second option. Maybe we can try to reuse rustc's path resolution, to find the items in question?

@xFrednet xFrednet added C-enhancement Category: New feature or request A-api Area: Stable API labels Jan 14, 2023
@xFrednet xFrednet added this to the v0.0.1 milestone Jan 14, 2023
@xFrednet
Copy link
Member Author

xFrednet commented Jan 17, 2023

I'll start looking into this issue slowly. For reference, Clippy has clippy_utils::def_path_res. The documentation notes, that a path like std::vec could resolve to the module and macro which is a good thing to keep in mind. I'm considering, if it might be good to restrict the resolve function to TyDefIds, as that should be the main usage of it anyways.

Further things found during research:


Notes about special cases, for extra fun:

  • Crates can depend on the same crate with different versions multiple times if they rename the dependency. The name resolution will need to handle this correctly... This will be fun....
  • Symbols can life in different Namespaces. This might be a problem for types, const/static variables and macros. I hope that they're all in the same namespace, but we have to test for that at some point. See Namespaces

@xFrednet
Copy link
Member Author

I'm sadly still stuck on this issue. It seems like rustc does all path resolution at once in an AST-pass. It makes sense for rustc not to expose a simple way to resolve paths outside of that, as additional usages would be a "code smell". However, this doesn't make it easy for external tools like Clippy and Marker. I see 4 ways to solve this and will need some more time to decide:

  1. Create a PR allowing external tools to use rustc's path resolution. Here IDK, which method would be enough to expose and also how to create the state the solver would need
  2. Use Clippy's manual path resolution
  3. Write a manual path resolution for Marker, similar to Clippy's
  4. Magically find a way to resolve paths with rustc that I've overlooked so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-api Area: Stable API C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant