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

Detect and don't report non-breaking renaming re-exports. #330

Merged
merged 3 commits into from
Jan 29, 2023

Conversation

obi1kenobi
Copy link
Owner

@obi1kenobi obi1kenobi commented Jan 28, 2023

Only use importable paths to match items across versions.

Declared names (e.g. Foo in pub struct Foo;) are subject to change in renaming pub use and pub type statements (e.g. pub use Foo as Bar), so relying on them to match items across versions means we miss renames. The importable paths provided by trustfall_rustdoc v0.8.2+ account for this, and list all valid paths under which the item can be imported. These paths are the only way we can match items across versions.

The above logic change needed to be applied to all currently-written lints, and will need to also apply to all future lints as well. Heads-up for @SmolSir @tonowak @staniewzki @mgr0dzicki.

The new logic is also somewhat more expensive, since it increases the search space and eliminates some of the pruning that filtering on the name could get us. This is unfortunate, but unavoidable at the moment — we don't want to sacrifice correctness for speed. I'll focus on perf improvements next and I'm confident we can win back all the lost performance and then some.

Fixes #288, #215, #202.
Fixes #42.

Only use importable names to match items across versions.
@obi1kenobi
Copy link
Owner Author

obi1kenobi commented Jan 28, 2023

Still TODO here:

  • add all crates that had these false-positives into the GitHub Actions regression suite
  • measure the perf impact of the new lint changes

I think the impact won't be spectacular on most crates, but still better to measure and make an explicit decision based on data. Hope is not a strategy.

Copy link
Collaborator

@epage epage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't get into too much of the details but glad to see this done

@obi1kenobi
Copy link
Owner Author

An overnight perf run didn't show any significant perf changes one way or the other on even the biggest crates.

@obi1kenobi obi1kenobi merged commit 7d18362 into main Jan 29, 2023
@obi1kenobi obi1kenobi deleted the imports_revamp branch January 29, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations
Projects
None yet
2 participants