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

Somewhat unhelpful error message "cannot use operator[] with object" #1220

Closed
rivertam opened this issue Aug 29, 2018 · 8 comments · Fixed by #1221
Closed

Somewhat unhelpful error message "cannot use operator[] with object" #1220

rivertam opened this issue Aug 29, 2018 · 8 comments · Fixed by #1221
Assignees
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@rivertam
Copy link
Contributor

rivertam commented Aug 29, 2018

  • What is the issue you have?

Indexing into an object with a string works fine, as it should. However, indexing into an object with a number does not work, which is acceptable (though perhaps unideal for ergonomics), but the error message is somewhat unhelpful. The error message is:

what():  [json.exception.type_error.305] cannot use operator[] with object

I think this should note that it is actually possible to use operator[] with object, but the argument must be a string.

  • Please describe the steps to reproduce the issue. Can you provide a small but working code example?
#include <iostream>
#include "json.hpp"

using json = nlohmann::json;

int main(int argc, char ** argv) {
  json hi = json::object();

  hi[5] = "hooo";

  std::cout << hi.dump() << std::endl;
}

g++ main.cpp --std=c++1z -Wall -o main && ./main

  • What is the expected behavior?

An error message indicating a number cannot be used to index into an object.

  • And what is the actual behavior instead?

An error message indicating one cannot use operator[] with an object at all.

(remainder of template omitted as I just really think it's noise in this case)

@gregmarr
Copy link
Contributor

Sounds like some indication of the argument type should be added to the error message.

@gregmarr
Copy link
Contributor

However, indexing into an object with a number does not work, which is acceptable (though perhaps unideal for ergonomics)

The std::map type which is used for objects doesn't support constant-time access by index, so iterating over an object by index would be really expensive for large objects. Why would you want to access by index?

@rivertam
Copy link
Contributor Author

rivertam commented Aug 30, 2018

@gregmarr First and foremost, that's just 100% not the issue.

Second, the way JavaScript and JSON work is that numeric keys are identical to string keys. So, in practice, obj[1] could be translated equivalent to obj["1"]. The time complexity of indexing via number is not really relevant. I'm just saying rather than throwing an exception here, json could just call std::to_string on the argument and have basically equivalent functionality. That's how JS works, anyways.

In this case, I'm actually making an object where the keys are numbers/numbers converted to strings. It's not an array and there's actually no iteration involved. Also performance is basically irrelevant (the object is like 10 keys big and I wouldn't care if the whole routine took like two seconds (though it currently takes < 20ms because it's just not that hard)).

@gregmarr
Copy link
Contributor

That might be how JavaScript works, but it's not how JSON works in other languages. In your case, you can just do the to_string() yourself.

@rivertam
Copy link
Contributor Author

@gregmarr All object keys in JSON (the format) must be strings, so it's really just an API/convenience thing, not a language thing. You could argue that anything that is trivially convertible to a string could be a valid argument to the operator[]. Again, I'm not advocating for or against it. All I was saying is that the choice was made and it's acceptable, but the error should at least specify what went wrong.

@gregmarr
Copy link
Contributor

Yes, anything that can be trivially converted to a string works. Calling to_string() is not a trivial conversion.

I agreed with you that the error message should say which version of operator[] was called, as there are versions that take numbers for array indices, and versions which take strings as object indices. You get the same error message if you call operator[](int) on a string or an object.

rivertam added a commit to rivertam/json that referenced this issue Aug 30, 2018
@rivertam rivertam mentioned this issue Aug 30, 2018
4 tasks
@nlohmann
Copy link
Owner

nlohmann commented Sep 9, 2018

I have one question regarding #1221: Do the explanatory strings of exceptions belong to the public API of the library? If so, wouldn't changing be only allowed with a major release?

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Sep 9, 2018
@rivertam
Copy link
Contributor Author

@nlohmann I would think the purpose of the error numbers (e.g. 305 in this case) stabilizes this and allows the strings to float. I've thought about this a good amount -- there are very few changes that one could make to a library that don't necessitate a major release if you're being extremely strict. There's almost always a way to detect any changes made in an API (an example might be a check for if a class has a method that results in different behavior), and at a certain point you need to say "If you're relying on this being true, it's your own fault."

If the errors didn't have numbers, though, I would agree with you.

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed state: please discuss please discuss the issue or vote for your favorite option labels Sep 18, 2018
@nlohmann nlohmann self-assigned this Sep 18, 2018
@nlohmann nlohmann added this to the Release 3.2.1 milestone Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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