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

WIP/RFC: Wildcards for banned_api #8274

Closed
wants to merge 2 commits into from
Closed

Conversation

akx
Copy link
Contributor

@akx akx commented Oct 27, 2023

Summary

This PR is a heavy WIP and RFC for adding support for wildcard paths for banned-api.

In short, the goal would be to be able to do

[tool.ruff.flake8-tidy-imports.banned-api]
"some_package.ANCIENT*".msg = "None of the ancient powers of `some_package` may be invoked"

instead of having to spell out all of the ancients' names in the config.

How?!

This PR implements this by way of a new CallPathPattern struct, which is basically a CallPath but with possible wildcard segments (glob.Patterns; I looked at wildmatch and decided not to introduce another dependency) and can be matched against a qualified name CallPath. I'm not 100% sure that's the best way here, and especially e.g. config parsing becomes a bit hairier since turning the strings in the config to CallPathPatterns could fail (e.g. some_package.ANCI[ is invalid).
(I also noticed there's a NameMatchPolicy thing in the flake8_tidy_imports linter that kind-of does the same thing.)

Then, there's a possible concern about performance, since we can no longer just use an FxHashMap for looking up bans. (Since this is very much a WIP (and who knows, maybe this is deemed a Bad Idea all in all), I haven't measured things.)

Of course we could come up with a smarter data structure that's both an FxHashMap for the exact-match happy path, and a vector of CallPathPatterns for other situations?

Test Plan

Not very well yet. It's probably buggy (but compiles!). The patch is currently also peppered with TODOs.

@akx
Copy link
Contributor Author

akx commented Oct 30, 2023

Sorry to ping, but: any thoughts, @charliermarsh et al.? I wouldn't want to continue here until the whole idea is even slightly validated :)

@charliermarsh
Copy link
Member

I think the overall idea is reasonable. I assume we'd also want to support:

"some_package.*.ANCIENT".msg = "None of the ancient powers of `some_package` may be invoked"

One idea: could we use a GlobSet? So, combine all the patterns into one, and do that single test; if it passes, then run all the individual tests to find the relevant message.

@akx
Copy link
Contributor Author

akx commented Oct 31, 2023

@charliermarsh Mm, yeah! (This would already support some_package.*.ANCIENT, but not some_package.**.ANCIENT.) Using a GlobSet would be a grand idea otherwise, but it doesn't look like you can customize which character(s) the globs think are path separators, so I don't think we can just easily match them against full CallPaths then.

We'd want these patterns to consider . path separators, not / ... unless we do spooky things on our side and turn the CallPaths into slash-separated strings (which, tbh, isn't completely unreasonable, probably...)?

@akx
Copy link
Contributor Author

akx commented Nov 1, 2023

Oh hey, huh... there's some prior art: #5024

@akx akx marked this pull request as draft November 1, 2023 12:44
@akx
Copy link
Contributor Author

akx commented Nov 1, 2023

Emdraftened pending more discussion in #8403...

@zanieb
Copy link
Member

zanieb commented Mar 12, 2024

@MichaReiser I've assigned you to this to decide what the path forward looks like. Let me know if that's not a good fit.

@MichaReiser
Copy link
Member

I only saw the ping when browsing through open Ruff PRs. I'm sorry.

I don't have a clear path forward for this, but I'm now looking into multifile analysis and plan to significantly change Ruff's semantic model. I think it does make sense to defer this work until we have a better understanding of how our semantic model looks (I don't expect CallPath to survive the refactor).

@MichaReiser MichaReiser closed this Apr 8, 2024
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.

4 participants