search

nasa / fpp

F Prime Prime: A modeling language for F Prime
https://fprime.jpl.nasa.gov
Apache License 2.0
46 stars 31 forks source link

Make reserved-keyword errors more obvious/readable #390

Closed thomas-bc closed 6 months ago

thomas-bc commented 6 months ago

When running a build where a component uses a reserved-keyword such as send, record etc..., the error is really not obvious. e.g. compiling the old version of Svc/PrmDb/PrmDb.fpp with FPP v2.1.0aX, the error message is 

  /Users/chammard/Work/fp/fprime-tutorial-math-component/fprime/Svc/PrmDb/PrmDb.fpp:143.29
                              record: I32 @< The record that had the failure
                              ^
  error: ) expected

It takes an awareness of the recent FPP changes to understand what is going on: record is now a reserved keyword and should be escaped or renamed.

It would be nice to have FPP throw a more meaningful error, or at least a hint, if possible.

bocchino commented 6 months ago

I looked at the parser implementation. I'm not sure what we can do better, given the parser library that we are using. The parsing logic is here:

https://github.com/fprime-community/fpp/blob/d9fa43a01c02e030014c9bb0b5ffd3d34e119db1/compiler/lib/src/main/scala/syntax/Parser.scala#L249

It's looking for a formal parameter list followed by a right parenthesis. In your example, when it hits the record keyword, it's done with the formal parameter list; then it expects a right parenthesis, which isn't there. It reports what it expected, where it expected it, and what's there instead (via the caret ^). If syntax highlighting is on in your editor, you'll see that the highlighted thing is a keyword, in a context where it should be an identifier. This seems reasonably clear.

I think the error message could be friendlier, e.g., "found the keyword record where ) was expected" or something, but I don't know of any way to get the parser library to generate that error. It just says expected and what was expected.

thomas-bc commented 6 months ago

"found the keyword record where ) was expected"

Would definitely be much much friendlier for users. But also if it's too much trouble, I'm happy to close this issue as wontfix. It's more in the nice-to-have bucket and not worth a lot of time. As you've said syntax highlighting should prevent this from happening in the first place, this was triggered by the introduction of the record keyword.

bocchino commented 6 months ago

I think this is something we can change if and when we rewrite the parser. With the current parser combinator library, I don't think it's feasible.