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

Converting between dictionary types #265

Merged
merged 6 commits into from Oct 29, 2018
Merged

Conversation

@faern
Copy link
Contributor

faern commented Oct 22, 2018

I'm trying to use the CFDictionary and CFMutableDictionary types more extensively. But run into problems. They are not very compatible with each other. This crate for example does not expose any clean way of using CFDictionaryCreateMutableCopy to create a CFMutableDictionary from any CFDictionary. I added a From implementation for this.

It was not possible to, in some safe/simple way, cast a CFDictionary<K, V> into the untyped CFDictionary<*const c_void, *const c_void> something I added to_untyped to be able to do.

When it comes to downcasting, it was not possible to downcast a CFType into a dictionary, since they don't implement ConcreteCFType. Something they probably should, for the case where the keys and values are just void pointers. CFArray does exactly this.

Lastly, what I needed the most was to be able to convert a CFMutableDictionary into a CFDictionary. This allows me to borrow an immutable CFDictionary via the ItemRef type basically. It's possible to work around this and actually have owned instances of a CFDictionary and a CFMutableDictionary pointing to the same actual dictionary by doing this:

let mut_dict = CFMutableDictionary::new();
let mut_dict2 = mut_dict.clone();
let dict = mut_dict.as_immutable();
// Here one can mutate the dict via `mut_dict2` while `dict` is still in scope/alive.

But since we already allow cloning a CFMutableDictionary to have two mutable references to the same dictionary I guess someone decided this should be viewed as safe. Both CFDictionary and CFMutableDictionary does not implement Send so we should not end up in any actual thread inconsistencies here.

If the above is indeed considered safe, I could get rid of the ItemRef juggling and just make it fn as_immutable(&self) -> CFDictionary directly maybe? Would simplify usage.


This change is Reviewable

impl CFPropertyListSubClass for ::dictionary::CFDictionary {}
impl<T> CFPropertyListSubClass for ::array::CFArray<T> {}
impl<K, V> CFPropertyListSubClass for ::dictionary::CFDictionary<K, V> {}
impl<K, V> CFPropertyListSubClass for ::dictionary::CFMutableDictionary<K, V> {}

This comment has been minimized.

@faern

faern Oct 22, 2018

Author Contributor

I'm not completely sure this is correct or safe, but I think it is. The docs say that a mutable dictionary can be viewed as an immutable one, from my understanding. But not the other way around. So looking at a mutable dictionary as a propertylist should just be an extension of viewing it as an immutable dictionary then?


/// Borrow self as an immutable dictionary.
#[inline]
pub fn as_immutable(&self) -> ItemRef<CFDictionary<K, V>> {

This comment has been minimized.

@faern

faern Oct 22, 2018

Author Contributor

As I wrote in the initial description above, trying to limit this usage so the mutable dictionary acts as borrowed (thus immutable) while the returned value exists might be overkill/false claims. Since it's so easy to go around it anyway. Anyone can just clone their mutable dictionary and make one of them immutable and still have a mutable version.

With the added addition of

 impl<K, V> CFPropertyListSubClass for ::dictionary::CFMutableDictionary<K, V> {} 

below (Which we have not yet debated if it's correct or not), they could also just do

let mut_dict = CFMutableDictionary::new();
let dict = mut_dict.to_CFPropertyList().downcast::<CFDictionary>().unwrap();

and that way also have an owned immutable dictionary be live at the same time as a mutable version pointing to the same underlying dictionary.

/// Returns a `CFDictionary` pointing to the same underlying dictionary as this mutable one.
#[inline]
pub fn to_immutable(&self) -> CFDictionary<K, V> {
unsafe { CFDictionary::wrap_under_get_rule(self.0) }

This comment has been minimized.

@faern

faern Oct 22, 2018

Author Contributor

Since it was so easy to work around the borrowing here anyway I decided to revert that in the last commit. If we allow cloning of CFMutableDictionary I don't see why we would not allow this.

impl CFPropertyListSubClass for ::array::CFArray {}
impl CFPropertyListSubClass for ::dictionary::CFDictionary {}
impl<T> CFPropertyListSubClass for ::array::CFArray<T> {}
impl<K, V> CFPropertyListSubClass for ::dictionary::CFDictionary<K, V> {}

This comment has been minimized.

@faern

faern Oct 23, 2018

Author Contributor

I reverted the commit where I implemented CFPropertyListSubClass for CFMutableDictionary. I also removed the ConcreteCFType implementation for CFMutableDictionary. I realized these traits are not only for casting up to the more generic parent type, they are also for downcasting from them to the given subclass. Since mutable dictionaries have the same type ID as their immutable counterparts, it became possible to cast any CFType or CFPropertyList that was of CFDictionary type down to a CFMutableDictionary and then modify it, thus causing a segfault without any unsafe code.

This comment has been minimized.

@faern

faern Oct 23, 2018

Author Contributor

How stupid of me.. This of course applies to for example impl<T> CFPropertyListSubClass for ::array::CFArray<T> {} as well. That would allow downcasting to a typed array directly, which would not be safe. I removed the generics on these CFPropertyListSubClass implementations again..

@faern faern force-pushed the faern:dictionary-conversion branch from 6f23a1b to 1cb7f08 Oct 23, 2018
@jdm
jdm approved these changes Oct 29, 2018
Copy link
Member

jdm left a comment

I agree with your reasoning on all points. I don't think it makes sense to try to enforce Rust's views of immutable/mutable when binding to APIs that can work around it easily.

@jdm
Copy link
Member

jdm commented Oct 29, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2018

📌 Commit 1cb7f08 has been approved by jdm

bors-servo added a commit that referenced this pull request Oct 29, 2018
Converting between dictionary types

I'm trying to use the `CFDictionary` and `CFMutableDictionary` types more extensively. But run into problems. They are not very compatible with each other. This crate for example does not expose any clean way of using `CFDictionaryCreateMutableCopy` to create a `CFMutableDictionary` from any `CFDictionary`. I added a `From` implementation for this.

It was not possible to, in some safe/simple way, cast a `CFDictionary<K, V>` into the untyped `CFDictionary<*const c_void, *const c_void>` something I added `to_untyped` to be able to do.

When it comes to downcasting, it was not possible to downcast a `CFType` into a dictionary, since they don't implement `ConcreteCFType`. Something they probably should, for the case where the keys and values are just void pointers. `CFArray` does exactly this.

Lastly, what I needed the most was to be able to convert a `CFMutableDictionary` into a `CFDictionary`. This allows me to borrow an immutable `CFDictionary` via the `ItemRef` type basically. It's possible to work around this and actually have owned instances of a `CFDictionary` and a `CFMutableDictionary` pointing to the same actual dictionary by doing this:
```rust
let mut_dict = CFMutableDictionary::new();
let mut_dict2 = mut_dict.clone();
let dict = mut_dict.as_immutable();
// Here one can mutate the dict via `mut_dict2` while `dict` is still in scope/alive.
```
But since we already allow cloning a `CFMutableDictionary` to have two mutable references to the same dictionary I guess someone decided this should be viewed as safe. Both `CFDictionary` and `CFMutableDictionary` does not implement `Send` so we should not end up in any actual thread inconsistencies here.

If the above is indeed considered safe, I could get rid of the `ItemRef` juggling and just make it `fn as_immutable(&self) -> CFDictionary` directly maybe? Would simplify usage.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-foundation-rs/265)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2018

Testing commit 1cb7f08 with merge b937267...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2018

☀️ Test successful - status-travis
Approved by: jdm
Pushing b937267 to master...

@bors-servo bors-servo merged commit 1cb7f08 into servo:master Oct 29, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.