aeternity / aesophia

Stand alone compiler for the Sophia smart contract language
https://docs.aeternity.com/aesophia
ISC License
51 stars 19 forks source link

Simplify error messages reported by the compiler #362

Closed ghallak closed 2 years ago

ghallak commented 2 years ago

Fixes: #354

This PR changes most error messages to:

  1. Remove formatting newlines (between sentences).
  2. Remove trailing newlines (they will only show when print the error message using aeso_errors:pp but not in #err.msg).
  3. Remove position info (e.g. at line x, col y) when it's already available from #err.pos.
  4. Surround code with ` ` to make it distinguishable from the rest of the error message ("Unbound variable x", instead of "Unbound variable x").
dincho commented 2 years ago

IMO the compiler lib should not do any formatting at all but provide a list of structured errors downstream, that is to the HTTP and CLI compilers.

As an example structure I've in mind would be: type, line, column, message. hint

dincho commented 2 years ago

As an example of very hard to parse error message is:

➜  code_errors git:(master) aesophia_cli polymorphic_query_type.aes
Code generation error in 'polymorphic_query_type.aes' at line 3, col 5:
Invalid oracle type
  oracle('a, 'b)
The query type must not be polymorphic (contain type variables).

A lot of newlines.

Another example duplicating position information and non-sense newlines:

➜  contracts git:(master) aesophia_cli 05_greeter.aes 
Type error in '05_greeter.aes' at line 48, col 10:
The contract Greeter (at line 48, column 10) has no entrypoints. Since Sophia version 3.2, public
contract functions must be declared with the 'entrypoint' keyword instead of
'function'.

Type error in '05_greeter.aes' at line 53, col 5:
Use 'entrypoint' for declaration of blockheight (at line 53, column 5):
  entrypoint blockheight : (unit) => uint
hanssv commented 2 years ago

@dincho I think this is a good compromise, you can get a raw(ish) form of the error from the compilier, and you can ask the compiler to format it further. Or you can get the information in JSON. The reason for keeping it all in aesophia is to make updates/additions less cumbersome - otherwise you'd have to remember there is a new error when updating aesophia_cli/http, etc.

I don't see any major issues with the provided examples (they should no longer have duplicated position information!?) - separating different errors with a newline makes sense I think?

ghallak commented 2 years ago

@dincho Could you go over the changes to the files test/aeso_compiler_tests.erl and src/aeso_ast_infer_types.erl, to see if there are any error (or warning) messages that you think could be changed to be better?

dincho commented 2 years ago

separating different errors with a newline makes sense I think?

that's for sure, but I still see multiple newlines in a "single" error messages in the tests cc @ghallak

that's my main issue, for a practical example I'm building a sublime plugin that parses the CLI output, however using \n for error messages separator (which is fine, and is used in all the compilers I'm aware of), the usage of \n in the error message itself makes it un-parsable.

I've seen related issues for other languages reported elsewhere, and it seems to be e common (seen) practice in functional language compilers to use newlines in the error messages itself. Still, it does not makes much sense to me.

hanssv commented 2 years ago

The primary purpose of the errors output by aesophia_cli is to be read by a human rather than parsed by a tool! Perhaps we could add a switch (--raw_errors or something similar) to aesophia_cli?

And no, putting \n into a string doesn't make it unparseable but that discussion we've had before 😅

dincho commented 2 years ago

The primary purpose of the errors output by aesophia_cli is to be read by a human rather than parsed by a tool! Perhaps we could add a switch (--raw_errors or something similar) to aesophia_cli?

And no, putting \n into a string doesn't make it unparseable but that discussion we've had before 😅

Humans are lazy and use tools .. 😅

dincho commented 2 years ago

And no, putting \n into a string doesn't make it unparseable but that discussion we've had before

Well, define that in regex, then we talk again :)

hanssv commented 2 years ago

Well... parsing != regex 😈

Normally you use /s or ?s as modifier to do multiline regex?

hanssv commented 2 years ago

Let's merge this, and I'll tackle @dincho's issue in a follow up PR (and aesophia_cli).