Fixed error messages for RecursiveDo (#8501)
ClosedPublic

Authored by ruperthorlick on Mar 3 2017, 12:43 PM.

Details

Summary

Changes in a few different places to catch several different
types of error related to RecursiveDo

Signed-off-by: Rupert Horlick <ruperthorlick@gmail.com>

Test Plan

Three test cases, with further tests in comments

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ruperthorlick created this revision.Mar 3 2017, 12:43 PM
ruperthorlick retitled this revision from Fixed error messages for ex. 1 and 3 (#8501) to Fixed error messages for RecursiveDo (#8501).Mar 3 2017, 12:54 PM
ruperthorlick edited the summary of this revision. (Show Details)
bgamari requested changes to this revision.Mar 3 2017, 4:14 PM

I like the idea, but I'm a bit concerned about the implementation. I haven't thought about what a potential refactoring would look like, but I'm happy to advise if you'd like.

compiler/parser/Lexer.x
2418

Hmm, this (and the pattern binding above) actually looks a bit dodgy. It looks like offsetBytes simply pushes a pointer into a ForeignPtr. What happens if we push it beyond the beginning of the string? It could be quite bad. It seems like this should really be refactored a bit to be more robust.

This revision now requires changes to proceed.Mar 3 2017, 4:14 PM

I like the idea, but I'm a bit concerned about the implementation. I haven't thought about what a potential refactoring would look like, but I'm happy to advise if you'd like.

I completely agree, and I was concerned about it at the time. I didn't want to do too big of a refactoring to begin with though.

So what do you think our options are?

We could remember the last n tokens for some particular value of n so that they're available at error time?

Alternatively we could have some flags for recognising particular lexemes during lexing and then just check those at error time?

I would say the second one sounds fairly reasonable, but I don't have much experience with the code base yet. What're your thoughts?

bgamari added a comment.EditedMar 4 2017, 11:46 AM

I think we probably want a function like the following in StringBuffer,

-- | Return the previous @n@ characters (or fewer if we are less than @n@
-- characters into the buffer.
decodePrevNChars :: Int -> StringBuffer -> String
decodePrevNChars n (StringBuffer buf _ cur) =
    inlinePerformIO $ withForeignPtr buf $ \p0 -> go buf0 [] (p0 `plusPtr` cur)
  where
    go :: Ptr Word8 -> String -> Ptr Word8 -> IO String
    go buf0 acc p | buf0 >= p = return acc
    go buf0 acc p = inlinePerformIO $ do
        p' <- utfPrevChar p
        let (c,_) = utf8DecodeChar p'
        go buf0 (c:acc) p'

This would allow you to safely look back at the previous contents of the buffer.

ruperthorlick edited edge metadata.
  • Refactored examining of StringBuffers (Trac #8501)
ruperthorlick marked an inline comment as done.Mar 5 2017, 7:52 AM

Okay so I've added a function like the one @bgamari suggested and tested it. Looks pretty good.

Should I ignore the linting problem with the long line, or should I change it?

bgamari accepted this revision.Mar 6 2017, 9:28 AM

Should I ignore the linting problem with the long line, or should I change it?

I think it's fine as-is.

Thanks Rupert!

This revision is now accepted and ready to land.Mar 6 2017, 9:28 AM
bgamari requested changes to this revision.Mar 6 2017, 3:15 PM

Rupert, in addition to fixing the request inline could you also update the testsuite results which have changed due to this patch? This should just be a matter of,

$ ./validate
$ make -Ctestsuite accept
$ git commit -p
  # Commit the appropriate set of changes
compiler/parser/Lexer.x
2417–2419

@ruperthorlick, can we actually avoid this partiality? There are a number of validation issues due to this being evaluated on an empty this.

This revision now requires changes to proceed.Mar 6 2017, 3:15 PM
ruperthorlick marked an inline comment as done.Mar 7 2017, 5:41 AM

I've removed the partiality and run the validation.

There are some files that have changed in the testsuite that seem unrelated to my changes. When you say commit appropriate changes, what do you mean?

I've removed the partiality and run the validation.

There are some files that have changed in the testsuite that seem unrelated to my changes. When you say commit appropriate changes, what do you mean?

Output from tests other than the ones that you add here will change due to this patch. make accept modifies the expected output files corresponding to these tests to reflect their new output. However, it is still necessary to commit these changes. git commit -p will ask you to confirm each change, allowing you to review what changed before committing.

ruperthorlick edited edge metadata.
  • Removed partiality in error checking (Trac #8501)
  • Ran validation and updated test outputs
This revision is now accepted and ready to land.Mar 12 2017, 1:36 PM
This revision was automatically updated to reflect the committed changes.