-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Fix module visibility check #6685
base: master
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #6685 will not alter performanceComparing Summary
|
@@ -824,6 +830,15 @@ impl From<Module> for Root { | |||
} | |||
} | |||
|
|||
/// Iterates through two module paths and returns the number of common prefixes of src and dst, | |||
/// i.e., the number of common ancestors of the src and dst modules. | |||
fn common_ancestor_count(src: &ModulePath, dst: &ModulePath) -> usize { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, but to be clear what I had suggested originally was to make common_ancestor_count
a local function inside check_module_privacy
, unless you think it's going to be useful and called by something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I've managed to reduce it to a single line of code, I'm inclined inline it again, and not have it as a separate function.
I doubt it needs to be used elsewhere.
Description
Fixes #6673.
The module visibility check has been broken for some time.
In some cases, like the one described in #6673, this has resulted in modules being mistakenly regarded as inaccessible, even though they ought to be accessible.
In other cases, private modules were mistakenly regarded as accessible. For examples see the updated tests in the PR.
The check has now been fixed, and all tests have been updated accordingly.
This bugfix can be regarded as a breaking change, because some modules that were deemed accessible before may now be deemed inaccessible and will require the addition of a
pub
keyword in front of the module declaration.Checklist
Breaking*
orNew Feature
labels where relevant.