DST-ification of libraries #16918
Comments
|
Here is my very, very early WIP branch: https://github.com/nick29581/rust/tree/dstify |
|
I should almost have what is needed here. |
|
In particular, what is needed to enable unification of types. |
|
@nick29581 Are you working on this atm? I DSTified Want to coordinate the work in this area? |
|
I've compiled a table with all the traits that |
|
@japaric I was in the process of assembling a similar patch myself, but I'd be glad to work with you on this. I'm away from the office this week but should be available early next. Please ping me if you post a PR. What you have so far looks like a good start. |
|
DSTifying The reason is that, for example, The following patterns break:
I'll skip those traits for now. |
|
Some of this (e.g., |
|
@aturon I've got other branches where I've DSTified other 4 traits, all them need a newer snapshot (#18259) that includes #18121. Could you review them? Here are the branches + some remarks:
Each branch passes |
|
@nikomatsakis Care to elaborate on how we should be handling operators? I personally find it a bummer that they're one of the last places where explicit derefs of an actual reference are used, especially since they then... take a reference. |
|
@gankro According to #4920, @nikomatsakis idea at that moment was that |
|
Re: |
This PR changes the signature of several methods from `foo(self, ...)` to `foo(&self, ...)`/`foo(&mut self, ...)`, but there is no breakage of the usage of these methods due to the autoref nature of `method.call()`s. This PR also removes the lifetime parameter from some traits (`Trait<'a>` -> `Trait`). These changes break any use of the extension traits for generic programming, but those traits are not meant to be used for generic programming in the first place. In the whole rust distribution there was only one misuse of a extension trait as a bound, which got corrected (the bound was unnecessary and got removed) as part of this PR. I've kept the commits as small and self-contained as possible for reviewing sake, but I can squash them when the review is over. See this [table] to get an idea of what's left to be done. I've already DSTified [`Show`][show] and I'm working on `Hash`, but bootstrapping those changes seem to require a more recent snapshot (#18259 does the trick) r? @aturon cc #16918 [show]: https://github.com/japaric/rust/commits/show [table]: https://docs.google.com/spreadsheets/d/1MZ_iSNuzsoqeS-mtLXnj9m0hBYaH5jI8k9G_Ud8FT5g/edit?usp=sharing
|
@japaric @gankro what I'm most concerned about is that, currently, we do a kind of odd-ball search when looking up operators. e.g., if we have |
|
I got |
|
New problem, the [0u8, 1] == [0u8, 1];
//^~ error: mismatched types: expected `[u8]`, found `[u8, ..2]` (expected unsized array, found array)
[0u8, 1].eq([0u8, 1].as_slice()); // OK
[0u8, 1].eq(&[0u8, 1]); // OK@nikomatsakis I think you are working on the operator desugaring at the moment, any clue what could be happening there? I'm not quite sure how the desugaring works for fixed-sized arrays but I think that |
|
@japaric I am indeed working on cleaning up this code and I actually encountered a similar issue. I think I do know yes what the problem is, though I have to confirm. It has to do with the specifics of when coercions occur and under what conditions. Basically I think what happens (at least in my branch) is that we match the receiver as |
- The signature of the `*_equiv` methods of `HashMap` and similar structures have changed, and now require one less level of indirection. Change your code from:
``` rust
hashmap.find_equiv(&"Hello");
hashmap.find_equiv(&&[0u8, 1, 2]);
```
to:
``` rust
hashmap.find_equiv("Hello");
hashmap.find_equiv(&[0u8, 1, 2]);
```
- The generic parameter `T` of the `Hasher::hash<T>` method have become `Sized?`. Downstream code must add `Sized?` to that method in their implementations. For example:
``` rust
impl Hasher<FnvState> for FnvHasher {
fn hash<T: Hash<FnvState>>(&self, t: &T) -> u64 { /* .. */ }
}
```
must be changed to:
``` rust
impl Hasher<FnvState> for FnvHasher {
fn hash<Sized? T: Hash<FnvState>>(&self, t: &T) -> u64 { /* .. */ }
// ^^^^^^
}
```
[breaking-change]
---
After review I'll squash the commits and update the commit message with the above paragraph.
r? @aturon
cc #16918
Methods that used to take `ToCStr` implementors by value, now take them by reference. In particular, this breaks some uses of `Command`:
``` rust
Command::new("foo"); // Still works
Command::new(path) -> Command::new(&path)
cmd.arg(string) -> cmd.arg(&string) or cmd.arg(string.as_slice())
```
[breaking-change]
---
It may be sensible to remove `impl ToCstr for String` since:
- We're getting `impl Deref<str> for String`, so `string.to_cstr()` would still work
- `Command` methods would still be able to use `cmd.arg(string[..])` instead of `cmd.arg(&string)`.
But, I'm leaving that up to the library stabilization process.
r? @aturon
cc #16918
- The `BytesContainer::container_into_owned_bytes` method has been removed
- Methods that used to take `BytesContainer` implementors by value, now take them by reference. In particular, this breaks some uses of Path:
``` rust
Path::new("foo") // Still works
path.join(another_path) -> path.join(&another_path)
```
[breaking-change]
---
Re: `container_into_owned_bytes`, I've removed it because
- Nothing in the whole repository uses it
- Takes `self` by value, which is incompatible with unsized types (`str`)
The alternative to removing this method is to split `BytesContainer` into `BytesContainer for Sized?` and `SizedBytesContainer: BytesContainer + Sized`, where the second trait only contains the `container_into_owned_bytes` method. I tried this alternative [in another branch](https://github.com/japaric/rust/commits/bytes) and it works, but it seemed better not to create a new trait for an unused method.
Re: Breakage of `Path` methods
We could use the idea that @alexcrichton proposed in #18457 (add blanket `impl BytesContainer for &T where T: BytesContainer` + keep taking `T: BytesContainer` by value in `Path` methods) to avoid breaking any code.
r? @aturon
cc #16918
|
@japaric I think it's pretty close, but a few more cases to investigate. I ran The other thing, which is much harder, would be to add |
|
@aturon Here's the updated table. According to my analysis, this is what's left to be done:
Do look at the table, and let me know if you think I missed anything. |
|
@japaric Looks good to me! I think the The couple of places where you ask "shouldn't this be automatically derived?" -- the answer is yes, but not yet: that hasn't been implemented yet. I've just opened an issue about this. |
How is that possible? |
|
@japaric You're right. |
|
I'm going to close this as this was basically done for the alpha release and has been an ongoing concern during stabilization. Nice work everyone! |
fix: Don't assert paths being utf8 when filtering them in the watcher Closes rust-lang/rust-analyzer#16914
We should change our impls to a DST style where appropriate. E.g.,
impl Ord for str .... This requires changing a bunch of traits to have theSized?bound.This kind of works. The problem is that we currently do not unify
Twith unsized types ([T],str, etc.) during subtyping/type inference and thus trait matching. This means that if do the DST-ification, we will fail to type check in places with references (e.g.,a == bwhereaandbboth have type&str). However, if we do unify, we will get coherance errors where impls are defined for, e.g.,&Tand&str. Thus we have a bit of a catch 22. We could cfg our way to victory, but there are literally hundreds of places that need DST-ification, so that will be extremely painful. I'm not sure if trait reform landing can help us here, or perhaps where clauses with!=constraints or something equally exotic. Perhaps we could hack coherance with this special case temporarily. Anyway, we need a plan!cc @nikomatsakis
The text was updated successfully, but these errors were encountered: