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

"Bad plan description"? #65

Open
rpgoldman opened this issue Mar 6, 2025 · 7 comments
Open

"Bad plan description"? #65

rpgoldman opened this issue Mar 6, 2025 · 7 comments

Comments

@rpgoldman
Copy link

Any suggestions for debugging plans rejected by Validate with "Bad plan description!"

Looking at the source code, it looks like this could be either a failure in type checking or because the system has not been able to read the plan:

(!the_plan || !tc.typecheckPlan(the_plan))

the first disjunct comes from:

the_plan = dynamic_cast< plan * >(top_thing);

Which I'm afraid I don't follow. Am I right in assuming that this means that the format of the plan is incorrect?

If so, any reason why these two failures are conflated?

Also, looking at the type-checking, I see this:

  bool TypeChecker::typecheckPlan(const plan *p) {
    if (!isTyped) return true;
    return p->end() == std::find_if(p->begin(), p->end(), badchecker(this));
    };

Does the above miss an opportunity to tell the user what step of the plan is ill-typed?

@DerekLong101
Copy link
Contributor

DerekLong101 commented Mar 6, 2025 via email

@rpgoldman
Copy link
Author

rpgoldman commented Mar 7, 2025

The particular case I'm working with seems trivial, so I won't bother you with the example. But I was wondering: it looks like the type-checker iterates over the plan until it finds an error or reaches the end. Right now, the type checking function just checks to see if the latter case holds, returning a boolean. But if the result of the find_if is not the end of the plan, could that be returned and printed in some way for the user. I'll poke around and see if I can figure out how to do that. But I would welcome any advice!
(As I've said many times, I'm not a C++ programmer, so if you told me how to use the iterator returned by find_if to print the operator it would be lovely!)

@rpgoldman
Copy link
Author

@DerekLong101 -- As I've said, I am not a C++ programmer. By sheer bluff I got as far as the following:

   std::string TypeChecker::planTypecheckFlaw(const plan *p) {
     std::ostringstream oss;
     oss << "Type error in:\n";
     oss << *(std::find_if(p->begin(), p->end(), badchecker(this)));
     oss << "\n";
     return oss.str();
  };

Which I think almost works, but it looks like it's printing the address of the happening, instead of the happening:

Bad plan description!
Plan failed to type-check
Type error in:
0x600003ab18c0

If anyone could tell me what I'm doing wrong trying to print the value returned by find_if, I will submit a MR for this...

@rpgoldman
Copy link
Author

OK, more information: I think this is actually a problem with checking the domain. It turns out that there's a syntax error in the domain definition, but Validate does not report this. Instead, it parses only 7 of 10 action definitions in the domain, and when it tries to read a plan that contains one of the unparsed action definitions, it raises a spurious type error.

So ... does Validate not check the results of parsing its input files?

@rpgoldman
Copy link
Author

The underlying error was me treating :precondition as optional for operators whose :precondition is just "true". When I added :precondition () to the action definition, the domain parsed, and then the validate call succeeded.

@rpgoldman
Copy link
Author

See #67 for proposed fix

@rpgoldman
Copy link
Author

So ... does Validate not check the results of parsing its input files?

Here's the check in Validate's main():

    if (!an_analysis.the_domain) {
      cerr << "Problem in domain definition!\n";
      if (LaTeX)
        *report << "\\section{Error!} Problem in domain definition! \n "
                   "\\end{document}\n";
      exit(-1);
    };

Is this too permissive? Should we be checking an_analysis.error_list instead of just an_analysis.the_domain? That seems likely to me. If there are errors in the list at this point, we should stop instead of generating GIGO, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants