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

Compiler accepts deriving(Encodable, Decodable) for TreeMaps with non-str keys #8883

Closed
catamorphism opened this issue Aug 30, 2013 · 8 comments
Labels
A-syntaxext Area: Syntax extensions A-trait-system Area: Trait system A-type-system Area: Type system

Comments

@catamorphism
Copy link
Contributor

The following code should fail to compile:

extern mod extra;

use extra::json;
use extra::json::*;
use extra::serialize::{Decodable, Encodable};
use extra::treemap::TreeMap;
use std::io;

#[deriving(Encodable, Decodable)]
struct WorkMap(TreeMap<WorkKey, ~str>); //~ ERROR can't derive Encodable on this

#[deriving(Encodable, Decodable, Eq, TotalEq, TotalOrd)]
struct WorkKey { name: ~str, kind: ~str }

because there's only an Encodable/Decodable instance for TreeMaps that have ~strs as keys. However, it compiles successfully. Moreover, if I add the following main() function:

fn main() {
    let mut t = WorkMap(TreeMap::new());
    t.insert(WorkKey{ name: ~"Tim", kind: ~"person"}, ~"tjc");
    let encoded_str = do io::with_str_writer |wr| {
        let mut encoder = json::Encoder(wr);
        t.encode(&mut encoder);
    };
    let decoded = do io::with_str_reader(encoded_str) |rdr| {
        let j = json::from_reader(rdr).unwrap();
        let mut decoder = json::Decoder(j);
        Decodable::decode(&mut decoder)
    };
    assert!(t.len() == decoded.len());
}

the encode/decode round-trip fails with a JSON decoding error, because the encoder happily prints out a representation of the struct key where a string is required.

It looks like maybe a bug in trait matching where the type parameters are getting ignored? I'm not sure.

@catamorphism
Copy link
Contributor Author

Nominating for milestone 5 (production-ready)

@catamorphism
Copy link
Contributor Author

Correcting myself: I guess the first part of the code (with only type definitions) should compile, so the compiler is doing the right thing there. However, the main function, which calls json methods, shouldn't, since there is no json impl for this particular instance of TreeMap.

@bluss
Copy link
Member

bluss commented Aug 31, 2013

Ah, I know.

You're mixing the Encodable/Decodable traits with json's different "ToJson" encoder. Json can do both (to some extent??)

(I think this is a mundane issue of extra::json being incorrect, not about deriving and type inference. It's too late for me to disentangle serialize and json at the moment).

@bluss
Copy link
Member

bluss commented Sep 6, 2013

I think the real issue is #9028

@brandonson
Copy link
Contributor

I think that what we're actually seeing here has something to do with extra::json's implementation of Encodable assuming (incorrectly) that it'll only ever get maps with ~str keys. Given the implementations of Encodable in extra::serialize for HashMap and TreeMap, this is evidently not true. So it outputs keys assuming that they're valid json keys, but unfortunately no such restriction exists for Encodable maps.

It seems to me that any map with keys which implement Encodable should be valid to encode using Encodable::encode, which is why this compiles and runs. Maps with non-~str keys should don't work with ToJson::to_json, but that should have no effect on the example code here.

It still doesn't look like a deriving vs type inference, but neither does it seem like a problem with different encoding between ToJson and Encoder. I think it's just incorrect implementation details in the emit_map_XXX methods for json::Encoder.

@catamorphism
Copy link
Contributor Author

Just a bug

@erickt
Copy link
Contributor

erickt commented Sep 12, 2013

The underlying issue here is that even though the JSON spec says objects cannot have non-key strings, but the way serialize::Encodable is written json::Encoder cannot validate that the map key type is a string. Once we have generators (#7746), I have a plan to rewrite the encoders to generate tagged values, which should allow an Encoder to inspect the type of the key of a Map.

@steveklabnik
Copy link
Member

Encodable has been moved out into its own crate, and so if this issue is still relevant, it belongs there. however, so much has changed since this was originally posted that I'm not even sure if it is. If anyone thinks it is, please open something at https://github.com/rust-lang/rustc-serialize/issues

flip1995 pushed a commit to flip1995/rust that referenced this issue Jun 30, 2022
Add [`manual_rem_euclid`] lint

Closes rust-lang#8883

Adds a lint for checking manual use of `rem_euclid(n)`

changelog: Add [`manual_rem_euclid`] lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-syntaxext Area: Syntax extensions A-trait-system Area: Trait system A-type-system Area: Type system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants