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

Fix potentially ambiguous .as_ref() #1062

Merged
merged 1 commit into from
Jul 30, 2023
Merged

Fix potentially ambiguous .as_ref() #1062

merged 1 commit into from
Jul 30, 2023

Conversation

robjtede
Copy link
Contributor

@robjtede robjtede commented Jul 30, 2023

#1017 was reported as a breaking change made to rustls. This isn't really the case. The true culprit is this vague .as_ref() call.

In general, it is allowed to change a fn that used to take T to now take impl Trait where T: Trait. AsRef is an unfortunate example of a trait that is often misused like this.

There may be more of these cases, if possible they should be switched so that a new AsRef impl or change such as was made in rustls doesn't break downstreams again.

@robjtede robjtede marked this pull request as ready for review July 30, 2023 17:08
@Jarema Jarema self-requested a review July 30, 2023 17:58
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

Hey!

Thanks for the PR!
As I agree with the general approach to what is breaking and what is not, I think it's a little different story with such an important and impactful library as rustls, where breaking should probably be considered anything that is actually breaking anything, with plethora of use cases and different API usageges. Of course, the situation was handled nicely and swiftly by rustls team.

To the point of PR:
Indeed, such as_ref() can be unfortunate and lead to ambiguity. We will definitely scan for any other similar occurrences.

Once again, thanks for the contribution.
LGTM!

@Jarema Jarema merged commit 97a9498 into nats-io:main Jul 30, 2023
13 checks passed
@robjtede robjtede deleted the fix-asref branch July 30, 2023 20:33
@Jarema Jarema mentioned this pull request Sep 21, 2023
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.

2 participants