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

audit uses of as_ref() and remove those that are ambiguous #1466

Open
BurntSushi opened this issue Jul 25, 2024 · 5 comments
Open

audit uses of as_ref() and remove those that are ambiguous #1466

BurntSushi opened this issue Jul 25, 2024 · 5 comments
Assignees

Comments

@BurntSushi
Copy link

Adding a trait impl in bstr 1.9.2 (which is yanked for now) caused gix-credentials to fail to compile.

The issue looks to be the result of using ambiguous as_ref(). That is, an as_ref() whose inference relies on the fact that there is only one trait implementation to choose from. If new trait implementations are added, then inference fails and a compile error occurs.

I think this has happened before with gix, although I don't remember the precise details. What do you think about doing an audit for as_ref() usage? In general, I'd like to be able to add trait implementations to bstr as they come up, but I also don't want to be breaking folks downstream of me. :-)

@Byron
Copy link
Owner

Byron commented Jul 25, 2024

Thanks for the heads-up! Indeed, it's not the first time and I can't promise it will be the last one after this one is fixed.

As for bstr, adding new impls for AsRef isn't considered a breaking change (despite the risk), and since I seem to be making that mistake (and so will others), maybe embracing a reactive response is the only way here.

Yanking temporarily gives some time to respond and like in this case, publish a patched release which will be picked up by the most recent gix and the one before.

I will let you know once the fix is available, so the latest version of bstr can be unyanked again.

@BurntSushi
Copy link
Author

Thanks sounds good!

Byron added a commit that referenced this issue Jul 25, 2024
…impls (#1466)

As `bstr` adds a new implementation of `AsRef`, the `as_ref()` call becomes
ambiguous which will then break `gitoxide`.

Now `as_ref()` is avoided in favor of a method that can't fail.
@Byron
Copy link
Owner

Byron commented Jul 25, 2024

It looks like gitoxide-core is also broken (my 'audit' was made by using the latest unreleased local clone so I could compile with what v1.9.2 would be), which means that only the most recent version of gitoxide will install correctly. That fortunately should be fine as this is what most people do and do by default with cargo install gitoxide.

So with git-credentials released as patch and gitoxide-core released as patch, all current users should be safe.

Once CI passes here I will publish the new versions and share them in this issue.

@Byron Byron mentioned this issue Jul 25, 2024
2 tasks
@Byron
Copy link
Owner

Byron commented Jul 25, 2024

Both gitoxide-core and gix-credentials have been published as patch releases. They should prevent damage downstream as they don't use as_ref() anymore.

@Byron
Copy link
Owner

Byron commented Jul 25, 2024

Please feel free to close the issue once bstr 1.9.2 is unyanked, and I suppose I will know if it worked by the absence of a flood of new issues 😅,

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

No branches or pull requests

2 participants