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

Don't blanket impl LenTextSize #37

Merged
merged 2 commits into from
Mar 31, 2020
Merged

Conversation

CAD97
Copy link
Collaborator

@CAD97 CAD97 commented Mar 26, 2020

Instead, just provide copy-paste impls for somewhat commonly used std string containers.

@CAD97
Copy link
Collaborator Author

CAD97 commented Mar 26, 2020

This does mean that smol_str either has to impl LenTextSize directly, or the user of both SmolStr and text-size has to use TextSize::of::<&str>(&SmolStr). IIRC, this is only one place in rowan, however, so this shouldn't be a big impact on ra.

tests/constructors.rs Outdated Show resolved Hide resolved
@matklad
Copy link
Member

matklad commented Mar 27, 2020

The irony here is that if we made extension trait, and not TextSize::of, the method to create text sizes, then we'd get deref coercions for free. I do still feel that ::of design better. I even thing that of is better than of_char / of_str

@CAD97
Copy link
Collaborator Author

CAD97 commented Mar 27, 2020

If only we could rely on the fact that char: Deref will never hold, then the following should be enough:

/// Text-like structures that have a text size.
pub trait TextLen {
    /// The size of this text-alike.
    fn text_len(&self) -> TextSize;
}

impl TextLen for str {
    #[inline]
    fn text_len(&self) -> TextSize {
        self.len().try_into().unwrap()
    }
}

impl TextLen for char {
    #[inline]
    fn text_len(&self) -> TextSize {
        (self.len_utf8() as u32).into()
    }
}

impl<D: Deref> TextLen for D
where
    D::Target: TextLen,
{
    fn text_len(&self) -> TextSize {
        self.deref().text_len()
    }
}

With this formulation the error isn't great but at least makes sense (rather than infinite recursion):

error[E0277]: the trait bound `(): std::ops::Deref` is not satisfied
  --> tests\constructors.rs:35:26
   |
35 |     let _ = TextSize::of(());
   |                          ^^ the trait `std::ops::Deref` is not implemented for `()`
   | 
  ::: D:\repos\rust-analyzer\text-size\src\size.rs:60:18
   |
60 |     pub fn of<T: TextLen>(text: T) -> TextSize {
   |                  ------- required by this bound in `text_size::size::TextSize::of`
   |
   = note: required because of the requirements on the impl of `text_size::traits::TextLen` for `()`

Though I do suppose that API doesn't help prevent TextSize::of(String).

@matklad
Copy link
Member

matklad commented Mar 28, 2020

bors r+

@matklad matklad merged commit 9161432 into rust-analyzer:master Mar 31, 2020
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