checking Eof before calling parseIdent #596

Sehun0819 · 2022-07-02T07:13:17Z

Handling #594.
Guards are added before parseIdent, to prevent it from calling Parser.next on Eof.

cristianoc · 2022-07-02T08:19:00Z

Looking good.
Code duplication is fine as it's very very little compared to having parser.next return an option and having to check every single one.

A helper function for the error message should be more then enough to reduce boilerplate.

Sehun0819 · 2022-07-02T12:54:34Z

Thank you.
At first, I was thinking of fixing only apparent bugs(that is, found by fuzzer), so that innocent codes aren't affected. But it seems like more conservative(or aggressive) fixing is needed because there are so many Parser.next in res_core.ml.

syntax/src/res_core.ml

Lines 3603 to 3617 in 08029af

    
           | Lident ident -> 
        
             Parser.next p; 
        
             let loc = mkLoc startPos p.prevEndPos in 
        
             let lident = buildLongident (ident :: acc) in 
        
             Ast_helper.Exp.ident ~loc (Location.mkloc lident loc) 
        
           | token -> 
        
             if acc = [] then ( 
        
               Parser.next p; 
        
               Parser.err p (Diagnostics.unexpected token p.breadcrumbs); 
        
               Recover.defaultExpr ()) 
        
             else 
        
               let loc = mkLoc startPos p.prevEndPos in 
        
               Parser.err p (Diagnostics.unexpected token p.breadcrumbs); 
        
               let lident = buildLongident ("_" :: acc) in 
        
               Ast_helper.Exp.ident ~loc (Location.mkloc lident loc)

Concretely, Parser.next in L3604 would be safe, while one in L3610 seems highly suspicious though I've seen no concrete example yet. Would you give me an opinion?

cristianoc · 2022-07-02T13:13:41Z

There's no harm in doing all the non-obvious cases

For comparison there's a PR showing how horrible the code would look using exception analysis

cristianoc · 2022-07-03T01:41:07Z

This PR: #543
which has over 1000 code changes, is what we're trying to avoid here.

Sehun0819 · 2022-07-03T08:28:26Z

src/res_core.ml

@@ -1906,7 +1915,7 @@ and parseFirstClassModuleExpr ~startPos p =
 and parseBracketAccess p expr startPos =
  Parser.leaveBreadcrumb p Grammar.ExprArrayAccess;
  let lbracket = p.startPos in
-  Parser.next p;
+  Parser.expect Lbracket p;


Though just using Parser.next is safe as p is always Lbracket, this change would make it more clear.

Sehun0819 · 2022-07-03T08:28:38Z

I looked over every Parser.next usages in res_core.ml.

Because it is hard to fix skipTokensAndMaybeRetry directly(like parseIdent), I added guards right before call sites.
When it seems ok, I substituted Parser.next with Parser.nextUnsafe.

cristianoc · 2022-07-03T13:23:40Z

src/res_core.ml

-      with
-      | None -> Recover.defaultPattern ()
-      | Some () -> parsePattern p)
+      if p.Parser.token = Eof then Recover.defaultPattern ()


Error message missing

Can be moved to case in pattern matching

cristianoc · 2022-07-03T13:24:18Z

src/res_core.ml

-    | Some () -> parseLident p
-    | None -> ("_", mkLoc startPos p.prevEndPos))
+  | token -> (
+    if token = Eof then (


You can move this to a case in pattern matching

cristianoc · 2022-07-03T13:25:01Z

src/res_core.ml

@@ -600,7 +604,11 @@ let parseHashIdent ~startPos p =
    in
    Parser.next p;
    (i, mkLoc startPos p.prevEndPos)
-  | _ -> parseIdent ~startPos ~msg:ErrorMessages.variantIdent p
+  | token ->
+    if token = Eof then (


You can move this to a case in the pattern matching.

cristianoc · 2022-07-03T13:25:48Z

src/res_core.ml

@@ -1090,7 +1098,12 @@ let rec parsePattern ?(alias = true) ?(or_ = true) p =
            in
            Parser.next p;
            (i, mkLoc startPos p.prevEndPos)
-          | _ -> parseIdent ~msg:ErrorMessages.variantIdent ~startPos p
+          | token ->
+            if token = Eof then (


Case in pattern matching

cristianoc · 2022-07-03T13:27:26Z

src/res_core.ml

-      with
-      | None -> Recover.defaultExpr ()
-      | Some () -> parseAtomicExpr p)
+      if p.Parser.token = Eof then Recover.defaultExpr ()


Error missing

cristianoc · 2022-07-03T13:28:30Z

src/res_core.ml

-        Parser.err ~startPos:p.prevEndPos p
-          (Diagnostics.unexpected token p.breadcrumbs);
-        Recover.defaultType ())
+      if p.Parser.token = Eof then Recover.defaultType ()


Error missing

Sehun0819 · 2022-07-04T10:27:50Z

How about now?

cristianoc · 2022-07-04T10:58:03Z

src/res_core.ml

-      match recoverLident p with
-      | Some () -> parseLident p
-      | None -> ("_", mkLoc startPos p.prevEndPos))
+  | token when token = Eof ->


simply | Eof

cristianoc

This is looking great.

Any tests missing, that trigger the new cases?

cristianoc · 2022-07-04T13:02:16Z

src/res_core.ml

@@ -556,6 +556,9 @@ let rec parseLident p =
    Parser.next p;
    let loc = mkLoc startPos p.prevEndPos in
    (ident, loc)
+  | Eof ->
+    Parser.err ~startPos p (Diagnostics.unexpected p.Parser.token p.breadcrumbs);


This looks good, perhaps put this line, which is repeated many times, in a function?

cristianoc · 2022-07-04T14:59:36Z

Merged as needed for release

checking Eof before calling parseIdent

c38341d

Sehun0819 mentioned this pull request Jul 2, 2022

Making parser consistent with #542 #594

Closed

check Eof before calling Parser.next

f01534e

add guard for Parser.next

c8608dd

Sehun0819 commented Jul 3, 2022

View reviewed changes

cristianoc reviewed Jul 3, 2022

View reviewed changes

add error handling & fix pattern matching

32387e3

cristianoc reviewed Jul 4, 2022

View reviewed changes

fix pattern match Eof

12d00eb

cristianoc approved these changes Jul 4, 2022

View reviewed changes

cristianoc merged commit 3e8bafb into rescript-lang:master Jul 4, 2022

Sehun0819 deleted the fixcrash2 branch July 7, 2022 06:48

checking Eof before calling parseIdent #596

checking Eof before calling parseIdent #596

Uh oh!

Conversation

Sehun0819 commented Jul 2, 2022

Uh oh!

cristianoc commented Jul 2, 2022

Uh oh!

Sehun0819 commented Jul 2, 2022

Uh oh!

cristianoc commented Jul 2, 2022

Uh oh!

cristianoc commented Jul 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sehun0819 commented Jul 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sehun0819 commented Jul 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristianoc commented Jul 4, 2022

Uh oh!

Uh oh!