Skip to content

Warn if unresolved disallowed types/macros/methods are used in clippy.toml for disallowed_* macros #10090

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

Closed
wants to merge 1 commit into from

Conversation

i509VCB
Copy link
Contributor

@i509VCB i509VCB commented Dec 16, 2022

I am unsure if this is the right level of abstraction to work at for this feature. There doesn't seem to be an easy way to get a span of the config file to point to what directly was unresolved. This does note what entry is unresolved though so the user can see what may have been removed or is invalid for disallowed macros, types and methods.

changelog: [disallowed_macros]: warn if unresolved macro is disallowed, [disallowed_types]: warn if unresolved type is disallowed, [disallowed_methods]: warn if unresolved method is disallowed

….toml

I am unsure if this is the right level of abstraction to work at. There doesn't seem to be an easy way to get a span of the config file to point to what directly was unresolved. This does note what entry is unresolved though so the user can see what may have been removed or is invalid for disallowed macros, types and methods.
@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2022

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 16, 2022
@i509VCB i509VCB changed the title Warn if unresolved disallowed types/macros/methods are used in clippy.toml Warn if unresolved disallowed types/macros/methods are used in clippy.toml for disallowed_* macros Dec 16, 2022
@xFrednet
Copy link
Member

Hey, I would alter the changelog entry, to something like this:

changelog: [disallowed_macros], [disallowed_types], [disallowed_methods]: Now emit a warning if the given path couldn't be resolved

Does this capture the change correctly? 🙃

@Alexendoo
Copy link
Member

This may not mesh well with workspaces, when multiple crates share a clippy.toml some of the paths may be reachable from some but not other crates, e.g. if some crates depend on a certain dependency but others do not

Not sure on the best thing to do here

@xFrednet
Copy link
Member

We could have the warning, be optional, by another config value, lint or other kind of verification, but that doesn't feel quite right. Ideally, it would emit a warning, if the path couldn't be found in any workspace member. However, this is not viable with the current setup of Clippy AFAIK, as each crate is compiled individually.

We could add some magic to cargo-clippy, but that sounds very hacky 🤔

@bors
Copy link
Contributor

bors commented Apr 20, 2023

☔ The latest upstream changes (presumably #10669) made this pull request unmergeable. Please resolve the merge conflicts.

@xFrednet
Copy link
Member

As previously stated in the comments, I don't believe that this can be nicely implemented for workspaces with Clippy's current setup. If someone is as a suggestion, we'll be happy to discuss it, as the idea is good IMO.

For now, I'll close this PR, as there hasn't been any activity in the last year.

@i509VCB thank you for the suggestion and the time you put into this!

@xFrednet xFrednet closed this Mar 21, 2024
@xFrednet xFrednet added E-hard Call for participation: This a hard problem and requires more experience or effort to work on S-needs-discussion Status: Needs further discussion before merging or work can be started S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard Call for participation: This a hard problem and requires more experience or effort to work on S-inactive-closed Status: Closed due to inactivity S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants