Skip to content

Update withError associativity to match Control.Alt #92

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

Merged
merged 9 commits into from
Mar 25, 2022

Conversation

thomashoneyman
Copy link
Contributor

@thomashoneyman thomashoneyman commented Mar 23, 2022

Description of the change

Fixes #91, which resulted from purescript/purescript-control#79.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a link to this PR and your username
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation in the README and/or documentation directory
  • Added a test for the contribution (if applicable)

@thomashoneyman
Copy link
Contributor Author

@JordanMartinez do I need to do anything so that the master branch of control is picked up in CI?

@JordanMartinez
Copy link
Contributor

@thomashoneyman Looks like you need to bust the cache.

@JordanMartinez
Copy link
Contributor

Hm.... Thoughts on how to design around the CI error?

@JordanMartinez
Copy link
Contributor

JordanMartinez commented Mar 24, 2022

I tried out Nate's suggestion of bumping prec to 2 for <?>. Here's what I get as I change that value:

prec set to 3 (current)
[1/1 TypesDoNotUnify] test/Examples.purs:94:26

  94          ( string " " <?> "Failed to match space as a separator"
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  
  Could not match type
  
    String
  
  with type
  
    t0 t1
  
  while checking that type String
    is at least as general as type t0 t1
  while checking that expression "Failed to match space as a separator"
    has type t0 t1
  in value declaration extractWords
  
  where t0 is an unknown type
        t1 is an unknown type
prec set to 2 (Nate's idea): `string (" " "msg")`, not `(string " ") "msg"`
[1/1 TypesDoNotUnify] test/Examples.purs:94:26

  94          ( string " " <?> "Failed to match space as a separator"
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  
  Could not match type
  
    String
  
  with type
  
    t0 t1
  
  while checking that type String
    is at least as general as type t0 t1
  while checking that expression "Failed to match space as a separator"
    has type t0 t1
  in value declaration extractWords
  
  where t0 is an unknown type
        t1 is an unknown type
prec set to 4 or 5
[1/1 TypesDoNotUnify] test/Examples.purs:102:19

  102                <?> "Failed to match period as a separator"
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  
  Could not match type
  
    String
  
  with type
  
    Parser t0
  
  while checking that type String
    is at least as general as type Parser t0
  while checking that expression "Failed to match period as a separator"
    has type Parser t0
  in value declaration extractWords
  
  where t0 is an unknown type
prec set to 6
[1/1 TypesDoNotUnify] src/StringParser/CodeUnits.purs:80:63

  80  char c = satisfy (_ == c) <?> "Could not match character " <> show c
                                                                    ^^^^^^
  
  Could not match type
  
    String
  
  with type
  
    Parser Char
  
  while checking that type String
    is at least as general as type Parser Char
  while checking that expression show c
    has type Parser Char
  in value declaration char

@JordanMartinez
Copy link
Contributor

Since <|> is right associative now, it may be better to resolve this situation by dropping <?> entirely:

foo <|> fail "failed due to bar"

@JordanMartinez
Copy link
Contributor

Actually, the error is caused by what is likely just weird code string " " <?> "foo" <|> string "other". <?> is expected to go at the end of a chain. Here, it goes in front. Moreover, string " " <?> "foo" <|> string "bar" is the same as string " " <|> string "bar" since the error message will be ignored by the <|>:

extractWords :: Parser (NonEmptyList String)
extractWords = do
  endBy1 (regex "[a-zA-Z]+")
    -- try commenting out one of the "<|> string ..." lines and see what happens
    ( many1
        ( string " " <?> "Failed to match space as a separator"
                  -- ^^^
            <|> string "'"
              <?> "Failed to match single-quote char as a separator"
            <|> string ","
              <?> "Failed to match comma as a separator"
            <|> string "?"
              <?> "Failed to match question mark as a separator"
            <|> string "."
              <?> "Failed to match period as a separator"
              <?> "Could not find a character that separated the content..."
        )
    )

@JordanMartinez
Copy link
Contributor

JordanMartinez commented Mar 24, 2022

See purescript-contrib/purescript-parsing#164 (comment) and that thread's previous comments for context, but we need to keep <?> as infixl but drop its precedent to 4.

@JordanMartinez JordanMartinez merged commit fbd1380 into main Mar 25, 2022
@JordanMartinez JordanMartinez deleted the trh/fix-infix branch March 25, 2022 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix issues caused by <|> being made infixr
2 participants