erlfmt icon indicating copy to clipboard operation
erlfmt copied to clipboard

Ensure parser always returns `erl_anno:location()` in errors

Open gomoripeti opened this issue 2 years ago • 2 comments

This way the return type matches error_info()

This is an alternative fix for https://github.com/WhatsApp/erlfmt/pull/324

gomoripeti avatar Nov 26 '21 12:11 gomoripeti

Hey, sorry for taking so long to reply on this

I think it would be better to do the reverse - to make sure, if possible, we always return the richer types on errors. If anything the errors are more informative with potential start & end location of the error, and not just start one.

michalmuskala avatar Dec 10 '21 14:12 michalmuskala

thanks for the feedback

some pros for the simple location

  • consistency with erlfmt_scan which also returns a location only https://github.com/WhatsApp/erlfmt/blob/main/src/erlfmt_scan.erl#L73 (I don't know if an end_location makes sense at all in case of tokenizing)
  • consistency with OTP libs like https://www.erlang.org/doc/man/compile.html#error_information, https://www.erlang.org/doc/man/yecc.html#data-types etc (ofc erlfmt forked the scanner/parser to get rid of the limitations of OTP)
  • consistent response in error_info() of erlfmt:read_nodes[_string])

Which one would you prefer?

  • just allow both location and anno in the type of erlfmt:error_info() (as in #324)?
  • modify erlfmt_scan to also return an anno (end_location is a mandatory field of erlfmt_scan:anno so then end_location = start_location?). also modify the formatting code (which I am absolutely note familiar with)
  • modify erlfmt module only to unify the error_infos coming from different components

gomoripeti avatar Dec 10 '21 15:12 gomoripeti