String parsing results in FilePath as data-string when error occurs

[aaronchongth]

Environment

• OS Version: Ubuntu 20.04
• Source or binary build?
  • Source build, branch sdf11

Description

Adding this test in parser_TEST.cc,

TEST(Parser, ReadStringError)
{
  sdf::SDFPtr sdf = InitSDF();
  std::ostringstream stream;
  stream
    << "<sdf version='1.8'>"
    << "<model name=\"test\">"
    << "  <link name=\"test1\">"
    << "    <visual name=\"good\">"
    << "      <geometry>"
    << "        <box><size>1 1 1</size></box>"
    << "      </geometry>"
    << "    </visual>"
    << "  </link>"
    << "  <link>"
    << "    <visual name=\"good2\">"
    << "      <geometry>"
    << "        <box><size>1 1 1</size></box>"
    << "      </geometry>"
    << "    </visual>"
    << "  </link>"
    << "</model>"
    << "</sdf>";
  sdf::Errors errors;
  EXPECT_FALSE(sdf::readString(stream.str(), sdf, errors));
  ASSERT_NE(errors.size(), 0u);

  std::cerr << errors[0] << std::endl;

  EXPECT_EQ(errors[0].Code(), sdf::ErrorCode::ATTRIBUTE_MISSING);
  EXPECT_FALSE(errors[0].FilePath().has_value());
  EXPECT_FALSE(errors[0].LineNumber().has_value());
}

• Expected behavior:

Expected FilePath and LineNumber to be missing.

Error Code 4: Msg: Required attribute[name] in element[link] is not specified in SDF.

• Actual behavior:

However due to the source being set to data-string, we get this.

Error Code 4: [data-string:L1]:  Msg: Required attribute[name] in element[link] is not specified in SDF.

Steps to reproduce

1. Add code block into parser_TEST.cc
2. Build.
3. Run ./build/sdformat11/src/UNIT_parser_TEST

Output

Gist, https://gist.github.com/aaronchongth/74c9641202dbdc4e60142a8547449a1f

[EricCousineau-TRI]

I'd propose keeping the <fake token>:L# approach. Just change data-string to <data-string> to make it more clear that it's not a real file.

Rationale: It can confuse users if they don't see line numbers in some cases, and not in others. Additionally, the line number is still useful in the string.

Also, the current usage of "data-string" or "urdf string" may preclude an actual file by that name, leading to confusing errors. (Super edge case, but still.)

[aaronchongth]

Thanks for the input @EricCousineau-TRI!

I'd propose keeping the :L# approach. Just change data-string to to make it more clear that it's not a real file.

Yup that will make it quite clear. We can do that.

Also, the current usage of "data-string" or "urdf string" may preclude an actual file by that name, leading to confusing errors. (Super edge case, but still.)

I thought about that too, and played around with filesystem::exists(...) but figured that it was not really the job for Error to check for existence.

The solution I can think of that would taken into account the edge case where data-string or urdf string is indeed the file name, would be to have something like a filesystem::exists(...) check before creating an Error object in parser.cc. I avoided it because it would bloat up the source code a fair bit, let me know what you think.

[EricCousineau-TRI]

Sorry for late response!

The solution I can think of that would taken into account the edge case where data-string or urdf string is indeed the file name, would be to have something like a filesystem::exists(...) [...]

My suggestion would be to assume it's not the case hehe, at least to start. A concrete defect/use case can be filed as an issue later 😬
If it's possible to find a character that's invalid on all filesystems, then that'd work! Alternative is some obvious form of shell-style escaping (e.g. what bash would show if you did ls on a directory with weird characters).

Will check out your latest PR changes.

[EricCousineau-TRI]

K, checked latest PR, doesn't do any file checks - looks great!