Issue1866

classification
Title: Parser does not have mismatch token error messages caught by BaseRecognizer
Type: Severity: normal
Components: Core Versions: Jython 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: fwierzbicki Nosy List: fwierzbicki, smokxx
Priority: normal Keywords: patch

Created on 2012-03-22.10:00:02 by smokxx, last changed 2013-02-25.18:21:55 by fwierzbicki.

Files
File name Uploaded Description Edit Remove
record_mismatch_token_errors.patch smokxx, 2012-03-27.11:34:04
make_indexer_report_diagnostics_with_line_column_length_info.patch smokxx, 2012-03-27.11:35:18
unnamed smokxx, 2012-04-09.14:37:34
Messages
msg6955 (view) Author: Roman (smokxx) Date: 2012-03-22.10:00:02
Example source:
def a main(argv):
  pass
<EOF>

AnalyzingParser output (I added error reporting even if ast generation succeeded):

line 1:6 extraneous input 'main' expecting LPAREN
parse result: 
Module(body=[FunctionDef],)
errors: 


So BaseRecognizer reported error but it was not recorded.
Output with patch:

line 1:6 extraneous input 'main' expecting LPAREN
parse result: 
Module(body=[FunctionDef],)
errors: 
{ message [null], line [1], position [6] }
msg6957 (view) Author: Roman (smokxx) Date: 2012-03-23.13:03:18
Added related patch making indexer record diagnostics with line/column/length information.
msg6959 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-03-23.22:23:02
smokxx: do you use the indexer? It came from Google, and I haven't actually used it -- I'm ok with reviewing the record_mismatch_token_errors.patch without a test as I'm pretty comfortable with that code - but the indexer one will definitely need a test to go with it so I can figure out what's going on better.
msg6960 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-03-23.22:29:04
+    @Override
+    public void reportError(RecognitionException e) {
+      // Update syntax error count and output error.
+      super.reportError(e);
+      errorHandler.reportError(this, e);
-}
+    }
+}

smokxx: we won't be able to do it like you have as we will end up with ANTLR style error messages on top of/mixed in with Python style error messages - is there any way you can change your patch to extend some kind of errorHandler?
msg6961 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-03-23.22:31:42
smokxx: at least that's what it looks like to me without trying it - I'll give it a longer look this weekend
msg6962 (view) Author: Roman (smokxx) Date: 2012-03-23.22:35:30
Do you mean that you don't want ANTLR errors to be mixed with jython's
lexer/parser errors in the same container inside of RecordingErrorHandler
or you just want error messages _text style_ to be the same?

My main objective is to record ANTLR errors as well as errors coming from
jython's lexer/parser, otherwise parsing syntactically incorrect source
code ends up with AST and no error messages available for analysis (see
example), that's not good.

And I'll add a test to Indexer and update the issue with a new patch.

2012/3/24 Frank Wierzbicki <report@bugs.jython.org>

>
> Frank Wierzbicki <fwierzbicki@users.sourceforge.net> added the comment:
>
> +    @Override
> +    public void reportError(RecognitionException e) {
> +      // Update syntax error count and output error.
> +      super.reportError(e);
> +      errorHandler.reportError(this, e);
> -}
> +    }
> +}
>
> smokxx: we won't be able to do it like you have as we will end up with
> ANTLR style error messages on top of/mixed in with Python style error
> messages - is there any way you can change your patch to extend some kind
> of errorHandler?
>
> _______________________________________
> Jython tracker <report@bugs.jython.org>
> <http://bugs.jython.org/issue1866>
> _______________________________________
>
msg6963 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-03-25.22:26:31
So starting with a file with an error:

def a foo():
<end of file>

CPython gives:

  File "error.py", line 1
    def a foo():
            ^
SyntaxError: invalid syntax

Current Jython gives:


  File "error.py", line 1
    def a foo():
         ^
SyntaxError: mismatched input 'foo' expecting LPAREN

And Jython with your patch gives:

line 1:6 mismatched input 'foo' expecting LPAREN
  File "error.py", line 1
    def a foo():
         ^
SyntaxError: mismatched input 'foo' expecting LPAREN

The trouble with your patch is that it causes two error messages in the output - the first from ANTLR at the top

line 1:6 mismatched input 'foo' expecting LPAREN

and the python style at the bottom:

  File "error.py", line 1
    def a foo():
         ^
SyntaxError: mismatched input 'foo' expecting LPAREN

The bottom Python style is what we want, and so the top ANTLR error message is extraneous -- if you could rework the patch to avoid the extra output (basically you'd need to override ANTLR's reportError message and leave out the call to displayRecognitionError call then this  would be an acceptable patch. Thanks for working on this! I'd love to get others working in the parser -- it clearly needs more eyes :)
msg6964 (view) Author: Roman (smokxx) Date: 2012-03-26.05:22:56
Ah, I got it.

I did not run "jython" jar as is, I don't quite understand why current
jython outputs the error, because as I wrote in inital comment to the
issue, that error is not recorded (errors container is empty in
RecordingErrorHandler).

What if I just override BaseRecognizer's emitErrorMessage and make it do
nothing?

2012/3/26 Frank Wierzbicki <report@bugs.jython.org>

>
> Frank Wierzbicki <fwierzbicki@users.sourceforge.net> added the comment:
>
> So starting with a file with an error:
>
> def a foo():
> <end of file>
>
> CPython gives:
>
>  File "error.py", line 1
>    def a foo():
>            ^
> SyntaxError: invalid syntax
>
> Current Jython gives:
>
>
>  File "error.py", line 1
>    def a foo():
>         ^
> SyntaxError: mismatched input 'foo' expecting LPAREN
>
> And Jython with your patch gives:
>
> line 1:6 mismatched input 'foo' expecting LPAREN
>  File "error.py", line 1
>    def a foo():
>         ^
> SyntaxError: mismatched input 'foo' expecting LPAREN
>
> The trouble with your patch is that it causes two error messages in the
> output - the first from ANTLR at the top
>
> line 1:6 mismatched input 'foo' expecting LPAREN
>
> and the python style at the bottom:
>
>  File "error.py", line 1
>    def a foo():
>         ^
> SyntaxError: mismatched input 'foo' expecting LPAREN
>
> The bottom Python style is what we want, and so the top ANTLR error
> message is extraneous -- if you could rework the patch to avoid the extra
> output (basically you'd need to override ANTLR's reportError message and
> leave out the call to displayRecognitionError call then this  would be an
> acceptable patch. Thanks for working on this! I'd love to get others
> working in the parser -- it clearly needs more eyes :)
>
> _______________________________________
> Jython tracker <report@bugs.jython.org>
> <http://bugs.jython.org/issue1866>
> _______________________________________
>
msg6965 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-03-26.14:45:54
Making emitErrorMessage do nothing sounds like it should work, or displayRecognitionError could be made to do nothing so that it doesn't uselessly construct the error message. I don't remember if there where any other side effects (there is this: "state.errorRecovery = true;" in reportError - I don't remember if that does anything we don't want - but I doubt it.
msg6967 (view) Author: Roman (smokxx) Date: 2012-03-27.11:34:04
Made our own empty displayRecognitionError that overrides BaseRecognizer's one.
msg6968 (view) Author: Roman (smokxx) Date: 2012-03-27.11:35:18
Added a test to IndexerTest, it fails without without the previous patch. Also addded some checks to avoid NPEs during diagnostics recording.
msg6985 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-03-29.22:04:04
OK record_mismatch_token_errors.patch is pushed, thanks Roman! It may be a litte while before I can evaluate make_indexer_report_diagnostics_with_line_column_length_info.patch - so I'll leave this bug open.
msg7040 (view) Author: Roman (smokxx) Date: 2012-04-09.14:37:34
Hi Frank,

When do you think you'll be able to review
make_indexer_report_diagnostics_with_line_column_length_info.patch ?
Sorry for nagging, just want to plan my time.

2012/3/30 Frank Wierzbicki <report@bugs.jython.org>

>
> Frank Wierzbicki <fwierzbicki@users.sourceforge.net> added the comment:
>
> OK record_mismatch_token_errors.patch is pushed, thanks Roman! It may be a
> litte while before I can evaluate
> make_indexer_report_diagnostics_with_line_column_length_info.patch - so
> I'll leave this bug open.
>
> _______________________________________
> Jython tracker <report@bugs.jython.org>
> <http://bugs.jython.org/issue1866>
> _______________________________________
>
msg7041 (view) Author: Frank Wierzbicki (fwierzbicki) Date: 2012-04-09.14:52:07
Hi Roman - I should be able to do a review of the indexer patch this week. Sorry for the slowness - I haven't studied the indexer much yet :)
History
Date User Action Args
2013-02-25 18:21:55fwierzbickisetpriority: normal
versions: + Jython 2.7
2012-04-09 14:52:07fwierzbickisetmessages: + msg7041
2012-04-09 14:37:34smokxxsetfiles: + unnamed
messages: + msg7040
2012-03-29 22:04:04fwierzbickisetmessages: + msg6985
2012-03-27 11:35:19smokxxsetfiles: + make_indexer_report_diagnostics_with_line_column_length_info.patch
messages: + msg6968
2012-03-27 11:34:26smokxxsetfiles: - record_mismatch_token_errors.patch
2012-03-27 11:34:23smokxxsetfiles: - make_indexer_report_diagnostics_with_line_column_length_info.patch
2012-03-27 11:34:21smokxxsetfiles: - unnamed
2012-03-27 11:34:19smokxxsetfiles: - unnamed
2012-03-27 11:34:05smokxxsetfiles: + record_mismatch_token_errors.patch
messages: + msg6967
2012-03-26 14:45:54fwierzbickisetmessages: + msg6965
2012-03-26 05:22:56smokxxsetfiles: + unnamed
messages: + msg6964
2012-03-25 22:26:31fwierzbickisetmessages: + msg6963
2012-03-23 22:35:30smokxxsetfiles: + unnamed
messages: + msg6962
2012-03-23 22:31:43fwierzbickisetmessages: + msg6961
2012-03-23 22:29:05fwierzbickisetmessages: + msg6960
2012-03-23 22:23:02fwierzbickisetmessages: + msg6959
2012-03-23 22:20:20fwierzbickisetassignee: fwierzbicki
nosy: + fwierzbicki
2012-03-23 13:03:18smokxxsetfiles: + make_indexer_report_diagnostics_with_line_column_length_info.patch
messages: + msg6957
2012-03-22 10:00:03smokxxcreate