Skip to content

Update withError associativity to match Control.Alt#92

Merged
JordanMartinez merged 9 commits intomainfrom
trh/fix-infix
Mar 25, 2022
Merged

Update withError associativity to match Control.Alt#92
JordanMartinez merged 9 commits intomainfrom
trh/fix-infix

Conversation

@thomashoneyman
Copy link
Copy Markdown
Contributor

@thomashoneyman thomashoneyman commented Mar 23, 2022 •
edited
Loading

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
Copy Markdown
Contributor Author

thomashoneyman commented Mar 23, 2022

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

@JordanMartinez
Copy link
Copy Markdown
Contributor

JordanMartinez commented Mar 23, 2022

@thomashoneyman Looks like you need to bust the cache.

@JordanMartinez
Copy link
Copy Markdown
Contributor

JordanMartinez commented Mar 23, 2022

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

@JordanMartinez
Copy link
Copy Markdown
Contributor

JordanMartinez commented Mar 24, 2022 •
edited
Loading

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
Copy Markdown
Contributor

JordanMartinez commented Mar 24, 2022

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
Copy Markdown
Contributor

JordanMartinez commented Mar 24, 2022

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
Copy Markdown
Contributor

JordanMartinez commented Mar 24, 2022 •
edited
Loading

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 reviewed Mar 24, 2022
View reviewed changes
Comment thread src/StringParser/Combinators.purs Outdated
JordanMartinez
JordanMartinez reviewed Mar 24, 2022
View reviewed changes
Comment thread CHANGELOG.md Outdated
@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

Reviewers

1 more reviewer

@JordanMartinez JordanMartinez JordanMartinez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix issues caused by <|> being made infixr

2 participants

@thomashoneyman @JordanMartinez