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

Incorrect traversal for AppSpine #58

Open
purefunctor opened this issue Jun 20, 2024 · 1 comment
Open

Incorrect traversal for AppSpine #58

purefunctor opened this issue Jun 20, 2024 · 1 comment

Comments

@purefunctor
Copy link

There seems to be a bug in the traversal for AppSpine, which was introduced by #51. Here's a small reproduction:

module Main where

import Prelude

import Effect (Effect)
import Effect.Console (log)
import PureScript.CST (RecoveredParserResult(..), parseModule)
import PureScript.CST.Traversal (defaultVisitorM, topDownTraversal, traverseModule)
import PureScript.CST.Types (Module)

moduleSource  String
moduleSource =
  """
module Main where

x = f a
"""

onExpr :: Module Void -> Effect (Module Void)
onExpr = traverseModule $ topDownTraversal $ defaultVisitorM
  { onExpr = \e -> do
      log "Hey!"
      pure e
  }

main  Effect Unit
main = case parseModule moduleSource of
  ParseSucceeded m → do
    _ <- onExpr m
    pure unit
  _ →
    log "Failed."

Expected output

Hey! is printed three times, for f a, f, and then a respectively.

Likely issue

The issue seems to stem from the use of traverseExpr rather than k.onExpr, as seen on this diff

More specifically, ExprApp was originally:

  ExprApp expr args -> ExprApp <$> k.onExpr expr <*> traverse k.onExpr args

Then it was changed to:

  ExprApp expr args -> ExprApp <$> k.onExpr expr <*> traverse (traverseExprAppSpine k) args
  -- ...
 
traverseExprAppSpine
  :: forall e f r
   . Applicative f
  => { | OnBinder (Rewrite e f) + OnExpr (Rewrite e f) + OnType (Rewrite e f) + r }
  -> Rewrite e f (AppSpine Expr)
traverseExprAppSpine k = case _ of
  AppType tok ty -> AppType tok <$> traverseType k ty
  AppTerm expr -> AppTerm <$> traverseExpr k expr  -- not k.onExpr anymore

(Shouldn't traverseType be k.onType as well?)

@natefaubion
Copy link
Owner

Yeah, that seems wrong!

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

No branches or pull requests

2 participants