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

Provide LenTextSize for Arc<String> #36

Closed
wants to merge 1 commit into from

Conversation

CAD97
Copy link
Collaborator

@CAD97 CAD97 commented Mar 26, 2020

Unfortunately, this impl leads to infinite recursion for, say &(), or any reference type that doesn't impl LenTextSize. Cow::Owned(String::new()) falls into this trap via the type inference variable for the B in Cow<'_, B>.

I've reported the wonderful error which isn't even being caught by the "recursive requirements" folder.

I have some alternate constructions to try, but I'm putting this one in the pool before I forget that &Arc<String> should be accepted by TextSize::of.

let s = StringLike("");
let _ = TextSize::of(&s);
test! {
"",
Copy link
Member

Choose a reason for hiding this comment

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

Do we test ::of("") (without &) anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the doc-tests for TextSize::of, yes, but I'm going to add it here as well.

@matklad
Copy link
Member

matklad commented Mar 26, 2020

Hm, could you also check that this impl allows downstream crates to impl the trait? rowan's SyntaxNodeText comes into mind.

@matklad
Copy link
Member

matklad commented Mar 26, 2020

I guess, we can actually put this into an integration, test because it is a downstream crate.

@CAD97
Copy link
Collaborator Author

CAD97 commented Mar 26, 2020

For my own sanity, here's a (nonexhaustive) list of types we definitely want to be (able to be) accepted:

  • &str
  • char
  • &String
  • &Arc<str>
  • &Arc<String>
  • &Rc<str>
  • &Rc<String>
  • &smol_str::SmolStr
  • &sorbus::green::Node
  • &rowan::GreenNode
  • &rowan::SyntaxText
  • &rowan::api::SyntaxNode

And types that don't have to be accepted, but would be nice to:

  • &{above type}

I think we almost get what we want from LenTextSize::len_text_size(&self), LenTextSize for str, LenTextSize for char, and LenTextSize for D: Deref where D::Target: LenTextSize, but that fails to "upstream crates may add a new impl of trait std::ops::Deref for type char in future versions", thanks, coherence

@CAD97
Copy link
Collaborator Author

CAD97 commented Mar 26, 2020

Based on further experimentation, I think we have to just accept that this is a limitation of the Rust language that deref coersion is not occurring here, and tell users to deref their smart pointers to str. Or if they want to get deref coersion, use TextSize::of::<&str>. I've asked a question on urlo about whether the lack of deref coersion here is fundamental or just an implementation limitation.

@matklad
Copy link
Member

matklad commented Mar 26, 2020

What about removing blanket impls and just adding a tone of impls manually?

@CAD97
Copy link
Collaborator Author

CAD97 commented Mar 26, 2020

Yeah, that seems to be the best bet. I'm doing such now.

@CAD97 CAD97 closed this Mar 26, 2020
@CAD97 CAD97 deleted the recursive-lentextsize branch March 26, 2020 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants