Wiki

Ticket #212 (closed defect: fixed)

Opened 8 years ago

Last modified 8 years ago

File, line number and 'error' repeated in compiler error message.

Reported by: nevdelap Owned by: Chuck
Priority: trivial Milestone:
Component: Cobra Compiler Version: 0.8.0
Keywords: Cc:

Description

These are the two cases I can remember seeing.

class Test

	def main is shared
		print (1 +
			  2)	# test.cobra(5): error: test.cobra(5,1): error: Cannot mix tabs and s...
		a = 1 +		# test.cobra(6): error: test.cobra(6,10): error: Expecting an expression.

Tested against 2402.

Attachments

tokenizer-error-repeat.patch Download (3.5 KB) - added by hopscc 8 years ago.
with-colnum.patch Download (1.6 KB) - added by hopscc 8 years ago.

Change History

Changed 8 years ago by hopscc

Changed 8 years ago by hopscc

  • status changed from new to assigned
  • owner set to Chuck

Occurs in all errors generated from lexing/tokenizer.

Tokenizer errors put source line and info in message.
Parser uses these messages to generate a ParserException.
Default conversion to console string prepends file, line and type info again.

patchfile has codefix, new test and rel note addendum

Changed 8 years ago by Chuck

  • The test case does appear to be useful because it does not fail before applying the rest of the patch.
  • I couldn't find anywhere that extra location information was being included just for tokenizer errors. Neither the tokenizer itself nor the TokenizerError class do this. Parser._recordError does this. Sure enough, you can get double location info with a parser error (that is not a tokenizer error) whether the patch is applied or not:
    class X
    	def main
    		x = _ y
    # 106-parser-error.cobra(4): error: 106-parser-error.cobra(4,7): error: Unexpected line continuation character.
    

Changed 8 years ago by Chuck

In the above comment, "Parser._recordError" should be "Parser._makeError"

Changed 8 years ago by Chuck

I have the fix.

Changed 8 years ago by Chuck

  • status changed from assigned to closed
  • resolution set to fixed

Fixed in changeset:2407

The fix was to simply remove any error message enhancement in Parser._makeError. We also now have a strong test for the compiler error message format in order to avoid regressions.

Changed 8 years ago by nevdelap

Hi, I tested this, and where it was giving...

test.cobra(6): error: test.cobra(6,10): error:

it's now giving...

test.cobra(6): error:

rather than...

test.cobra(6,10): error:

I thought the column number where the error is would be a useful bit of information to retain in the error message.

Changed 8 years ago by hopscc

  • status changed from closed to reopened
  • resolution fixed deleted

Yeah the column number where the error was detected is definitely necessary - needed that a couple of times.

Sorry chuck My bad
I generated code that threw all the errors via .throwError from CobraTokenizer and all of them gave the double position info.
Tracked that to passing through Parser._makeError as well as Node.consoleString so corrected that for messages coming from the Tokenizer only - didnt think about anything from the Parser also passing
through the same codepath.
Generated a different clean test for the test suite after checking only for the error message with single position info, which passed
- never tried it on a clean tree - double bad.

Thats what happens when you try and fit a quick bugfix into a spare half hour or so and it doesnt...

Reopened for column number fix.
override consoleString in Parser.cobra (?)

Changed 8 years ago by hopscc

Changed 8 years ago by hopscc

  • status changed from reopened to assigned

patch to reassert colno info on errors, mod error line format test to accomodate
No new tests(:-).

Changed 8 years ago by Chuck

Cobra doesn't generally include column number information because it does not currently correct it for TABs for editors.

Changed 8 years ago by hopscc

a) Thats only true since your fix removing the error message enhancement in _makeError
(re: including col number info).

b) Neither has any other stand alone compiler (i.e not integrated with an editor) since compilation was invented. (re: correcting for TABs).

Changed 8 years ago by Chuck

Regarding (a) that _makeError method is not invoked when errors come from outside the parser such as during .bindInh, .bindInt and .bindImp. I think it would be weird if parser errors had (incorrect) column info and other errors had none...

Changed 8 years ago by hopscc

Well thats been the case from whatever initial release of cobra displayed error info
(lines and cols) up till now and it hasnt seemed to be weird enough to cause an issue
and
Perhaps a better answer might be to provide col info for errors coming from outside the parser rather than suppressing ones from inside the parser (and yes I know theres issues with doing that as well).

How can it be an improvement to provide less info about the point of an error when the info is available?

Also Its only incorrect column info if you are saying that the position is the "editor display column number" as opposed to the line column number or text line offset or <whatever descriptions compilers usually use for column in a line an error occurred at>.

Changed 8 years ago by Chuck

  • status changed from assigned to closed
  • resolution set to fixed

Patch applied in changeset:2425

Note: See TracTickets for help on using tickets.