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

Conversion to user type containing a std::vector not working with documented approach #1511

Closed
payoto opened this issue Mar 11, 2019 · 6 comments · Fixed by #1555
Closed
Assignees
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@payoto
Copy link

payoto commented Mar 11, 2019

When performing an assignement from json to a user defined type, vectors inside this object get appended. I've included a working example of the bug at the end of this issue but this is a smaller summary:

class A {
public:
	std::vector<double> a;
	A(){a.push_back(0.0);}
};
void from_json(const json& j, A& p){
	j.at("a").get_to(p.a); ////// FROM THE DOCUMENTATION - does not work
}


json j1, j2;
A v1, v2;
j1=v1; // j1["a"] == [0.0]
v2=j1.get<A>(); // Unsafe
j2=v2; // j2["a"] == [0.0, 0.0]

I have read a bit about the problems with doing std::vector<int> v=j and that the get method of json should be used, however these issues seem to also affect the j.get_to(v) method. This means that when following the documentation for converting user defined types very strange run-time behaviours arise with no compile time indication that anything may go wrong.

This can be fixed by replacing the from_json function to:

void from_json(const json& j, A& p){
	p.a = j.at("a").get<std::vector<double>>();
}

Not pretty and a bit brittle but simple enough.

Possible solutions

Am I doing something silly here that is causing this?

If I am not I see 3 options:

  1. Update the documentation to suggest to users the explicit .get method.
  2. Add a compile time error/warning?
  3. Change the behaviour of the get_to method...

system

  • Windows 7 64bits
  • g++ 8.1.0 on MinGW-w64

Small Compilable Example

#include <iostream>
#include <vector>
#include "json.hpp"


using nlohmann::json;
// Class A does not work
class A {
public:
	std::vector<double> a;
	std::string name() {return("A");}
	A();
};
A::A(){
	this->a.push_back(0.0);
}
void to_json(json& j, const A& p){
	j = json{
		{"a", p.a},
	};
}
void from_json(const json& j, A& p){
	j.at("a").get_to(p.a); ////// FROM THE DOCUMENTATION - does not work
}

// Class B works as expected
class B {
public:
	std::vector<double> b;
	std::string name() {return("B");}
	B();
};
B::B(){
	this->b.push_back(0.0);
}
void to_json(json& j, const B& p){
	j = json{
		{"b", p.b},
	};
}
void from_json(const json& j, B& p){
	p.b=j.at("b").get<std::vector<double>>(); ////// THE DIFFERENCE - works
}

template<class T>
int test(){
	
	json j1, j2;
	T v1, v2;

	j1=v1; // Safe
	v2=j1.get<T>(); // Unsafe
	j2=v2; // Safe
	std::cout << "Testing class " << v1.name() << std::endl;
	std::cout << j1 << std::endl;
	std::cout << j2 << std::endl;

	if (j1!=j2){
		std::cerr << "Error: Parameter conversion to JSON "
			<<" is not symmetrical" << std::endl;
		std::cerr << "In: " << __PRETTY_FUNCTION__ << std::endl;
		return (1);
	};

	return(0);
	
}

int main(){
	test<A>(); 
        // This test shows that the member vector .a == {0.0, 0.0}
	test<B>();
        // This test shows that the member vector .b == {0.0}
	// as it should be
        return(0);
}

Output

Testing class A
{"a":[0.0]}
{"a":[0.0,0.0]}
Error: Parameter conversion to JSON  is not symmetrical
In: int test() [with T = A]
Testing class B
{"b":[0.0]}
{"b":[0.0]}
@theodelrieu
Copy link
Contributor

I think I understand the problem you're having.

The main difference between get and get_to is that get constructs a new object, whereas get_to does not.

Since you default constructs some A objects, the default constructor does a a.push_back, so the underlying vector has a size of 1.

When using get, you use the copy assignment operator, thus erasing the previous vector.

@payoto
Copy link
Author

payoto commented Mar 12, 2019

You're right, it has to do with the default constructor populating the array.

What does get_to do? Why is it default constructing and appending rather than copying the data? Is this something I can fix in class A (while keeping a default constructor)?

Thanks for your help.

@theodelrieu
Copy link
Contributor

get_to does not construct anything, it just takes a reference to a constructed object and calls from_json directly.

@payoto
Copy link
Author

payoto commented Mar 12, 2019

ok, true it's not constructing, I am confused by the prepending of precisely one 0.0 every time it goes through the statements a=j.get<A>(); or a=j;, guess I'm still confused of where it comes from. What does from_json do in the case of dynamically allocated memory that causes it to grow the std::vector?

At the moment doing any of the following pieces of code will lead to a.a increasing in size at every step. Doesn't seem like it is an intended behaviour...

json j;
A a;
a.a.push_back(1.0); // a.a == [0.0, 1.0]
for(int i=0; i<10; i++){
  j=a;
  a=j;
}
// a.a == [0.0, 0.0, 0.0, 0.0, 0.0 .... , 1.0]

or

for(int i=0; i<10; i++){
  j=a;
  a=j.get<A>();
}
// a.a == [0.0, 0.0, 0.0, 0.0, 0.0 .... , 1.0]

or

for(int i=0; i<10; i++){
  j=a;
  j.get_to(a);
}
// a.a == [0.0, 1.0, 0.0, 1.0, 0.0, 1.0, 0.0, 1.0, 0.0, 1.0, 0.0, 1.0, ....]

While all these behaviours result in b.b == [0.0, 1.0] for class B.

@theodelrieu
Copy link
Contributor

I agree this is very confusing.

As of now, from_json(std::vector) do not call clear() on the vector. Other overloads have similar behavior.

I think this is a bug, we should treat the second parameter as an output parameter, therefore overwriting the previous value like we do for to_json.

@theodelrieu theodelrieu mentioned this issue Apr 1, 2019
4 tasks
@nlohmann nlohmann added confirmed kind: bug solution: proposed fix a fix for the issue has been proposed and waits for confirmation labels Apr 3, 2019
@nlohmann nlohmann self-assigned this Apr 3, 2019
@nlohmann nlohmann added this to the Release 3.6.2 milestone Apr 3, 2019
@nlohmann
Copy link
Owner

nlohmann commented Apr 3, 2019

🔖 release item in #1555

@nlohmann nlohmann modified the milestones: Release 3.6.2, Release 3.7.0 Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed kind: bug release item: 🐛 bug fix solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants