Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] Improve parser error handling #7912

Merged
merged 21 commits into from
Mar 6, 2019

Conversation

Simn
Copy link
Member

@Simn Simn commented Mar 4, 2019

This PR changes how we communicate with our parser. I acknowledge that it looks scary, but I think it's a necessary step and I would like to make that step now rather than later.

First of all, when we're not in display mode, the parser still raises Parser.Error early as it did before. However, when we're in display mode (including diagnostics), it returns an element of this ADT:

type 'a parse_result =
	(* Parsed display file. There can be errors. *)
	| ParseDisplayFile of 'a * parse_error list
	(* Parsed non-display-file without errors. *)
	| ParseSuccess of 'a
	(* Parsed non-display file with errors *)
	| ParseError of 'a * parse_error * parse_error list

It might still raise Parser.Error in some (rare?) cases. The endgame here is to eliminate all these cases so it always parses something. This is going to require some more refactoring though and is not something we have to do for 4.0.

The idea is to look at all occurrences of ParseDisplayFile and friends (there are only 14 at the moment) and adjust what we want to do. For instance, the compilation server naturally reacts like this:

| ParseError(_,_,_) -> "not cached, has parse error",true
| ParseDisplayFile _ -> "not cached, is display file",true
| ParseSuccess data ->"cached",false

This addresses #7793. Once we cleaned up error handling completely, we could also cache the display file if it has no errors (useful for hover-mode where the AST is often intact).

As another example --macro parsing does this:

| ParseSuccess data -> data
| ParseError(_,(msg,p),_) -> (Parser.error msg p)
| ParseDisplayFile _ -> assert false (* cannot happen *)

There are some PARSERTODOs I have to look at still, but I would like to know if people agree that this is the right way forward.

As a more or less unintended side-effect, we collect syntax errors on the diagnostics file which should allow us to address #5306.

@@ -203,16 +209,14 @@ let parse_string com s p error inlined =
display_position := old_display;
in_display_file := old_in_display_file;
end;
Lexer.restore old;
display_error := old_de
Lexer.restore old
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not save syntax_errors at beginning of parsing and restore them here as well ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, good catch

-lib utest
--cmd node test.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that seems unrelated to the PR ;)

@Simn
Copy link
Member Author

Simn commented Mar 4, 2019

Something in the grammar is broken. It currently emits this error on every file:

source/Main.hx: 64-4611686018427387903: Expected }

@Simn
Copy link
Member Author

Simn commented Mar 4, 2019

There's a travis OS X display test failure:

Command: haxe [build.hxml]
utest/ui/text/PrintReport.hx:52: 
assertations: 1030
successes: 1029
errors: 1
failures: 0
warnings: 0
execution time: 128.564
results: SOME TESTS FAILURES (success: false)
cases.IImport
  testUsing1: ERROR ..E
    HaxeInvocationException
Called from local function #4 (src/cases/IImport.hx line 41 column 2)
Called from cases.IImport.testUsing1 (src/cases/IImport.hx line 44 column 20)
Called from DisplayTestContext.fields (src/DisplayTestContext.hx line 41 column 24)
Called from DisplayTestContext.callHaxe (src/DisplayTestContext.hx line 113 column 4)

I cannot reproduce this locally though:

Josefienes-MacBook-Pro:display Simon$ haxe --version
4.0.0-rc.1+67550f526
Josefienes-MacBook-Pro:display Simon$ haxe build.hxml 
utest/ui/text/PrintReport.hx:52: 
assertations: 1033
successes: 1033
errors: 0
failures: 0
warnings: 0
execution time: 47.97

results: ALL TESTS OK (success: true)

Any idea what could cause this?

@Simn
Copy link
Member Author

Simn commented Mar 4, 2019

Oh it's this shit again...

cases.Signature
  testBadAst: ERROR .....E
    HaxeInvocationException(Error: Invalid format: (����
, testBadAst, [-cp,src,-D,display-stdin,-lib,utest,--display,src/cases/Signature.hx@63@signature], package cases;
	class Some {
		function main() {
			test("foo"
		}
		static function test(a:String, b:Int) { }
	}
	])
Called from local function #2 (src/cases/Signature.hx line 31 column 2)
Called from cases.Signature.testBadAst (src/cases/Signature.hx line 33 column 37)
Called from DisplayTestContext.signature (src/DisplayTestContext.hx line 77 column 26)
Called from DisplayTestContext.callHaxe (src/DisplayTestContext.hx line 117 column 4)

@Simn Simn force-pushed the better_parser_error_handling branch from 1f6634e to fc1b2ac Compare March 4, 2019 18:27
@Simn Simn marked this pull request as ready for review March 5, 2019 07:20
@skial skial mentioned this pull request Mar 5, 2019
1 task
@Simn Simn merged commit ef34514 into HaxeFoundation:development Mar 6, 2019
@Simn Simn deleted the better_parser_error_handling branch March 6, 2019 09:07
back2dos pushed a commit to back2dos/haxe that referenced this pull request Oct 5, 2021
* [parser] start on parser error refactoring

* [parser] remove all `in_display` checks from grammar for now

* [parser] remove Parser.display_error

* [parser] start on parse_result

* [parser] remove some hard errors

* [tests] add test for broken syntax diagnostics

see HaxeFoundation#7793

* [parser] unbreak eval-debugger completion

* [parser] adjust Context.parse(InlineString)

* [parser] adjust `parse_metadata`

* [parser] adjust type_patch parsing

* [parser] adjust import.hx parsing

* [parser] adjust module parsing

* [parser] fix special } resume case

* [parser] handle module parsing errors properly

* [macro] fix bad macro display settings handling

* [tests] add toString to HaxeInvocationException

and hope that it gets called

* [submodule] try something

* [parser] fix syntax_errors handling

* [server] remove unused binding

* [parser] don't lose exact interpolation parser message

* [tests] remove unrelated change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants