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

Sean/typed core fn #20

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

Sean/typed core fn #20

wants to merge 49 commits into from

Conversation

gnumonik
Copy link
Collaborator

@gnumonik gnumonik commented Feb 1, 2024

Description of the change

This is the up-to-date branch of our typed CoreFn conversion machinery. It should handle every language construct now, and appears to work for a nontrivial, but by no means exhaustive, set of examples

I still need to setup the scaffolding for a real test suite & implement most of the minor changes that came up during our live review session so this isn't really ready for final review, but I thought it'd be important to get it up before the meeting tomorrow.


Checklist:

  • Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
  • Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution or a justification for the absence of tests

gnumonik and others added 20 commits November 28, 2023 19:00
…HaP, added basic nix shell with build deps & locale config, placeholder UPLC codegen module & functions. You almost certainly need the hie.yaml and .envrc to work on this so I committed those intentionally.
…n the annotation) + reworked pretty printer to... print prettily
…e expressions, necessary for fully typing the desugared typeclass declarations
…s-wip

Sean/typed core fn.typeclasses wip
type M m = (MonadSupply m, MonadState CheckState m, MonadError MultipleErrors m, MonadWriter MultipleErrors m)

-- | Traverse a literal. Note that literals are usually have a type like `Literal (Expr a)`. That is: The `a` isn't typically an annotation, it's an expression type
traverseLit :: forall m a b. Monad m => (a -> m b) -> Literal a -> m (Literal b)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks deriveable

import Control.Monad.IO.Class (MonadIO (liftIO))

import Language.PureScript.AST qualified as AST
import Language.PureScript.CoreFn (Ann, Module(..), Expr(..), Literal(..), Meta)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use explicit exports/imports, as suggested by https://github.com/mlabs-haskell/styleguide?tab=readme-ov-file#modules

inferExprTypes = \case
_ -> undefined

{-| nil = constr 0 []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, if you leave any commented-out code, describe how it can be useful/illustrative for the readers (if it can't, please remove).

are qualified by their ModuleName. What we do here is first look for a "local" type for the identifier using the provided source position,
then, if that fails, look up the identifier in the "global" scope using a module name.

I *think* this is fine but I'm not *certain*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can't name a single reason why it may not work, it's better to omit comments about uncertainty, because it is implied anyway - everywhere in our industry. If you have known concerns, please write them down.


{- Converts declarations from their AST to CoreFn representation, deducing types when possible & inferring them when not possible.

TODO: The module name can be retrieved from the monadic context and doesn't need to be passed around
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address this TODO

bindLocalVariables [(ss,name,valDeclTy,nv)] $ do
expr <- exprToCoreFn mn ss (Just valDeclTy) e -- maybe wrong? might need to bind something here?
pure [NonRec (ssA ss) name expr]
-- Recursive binding groups. This is tricky. Calling `typedOf` saves us a lot of work, but it's hard to tell whether that's 100% safe here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any specific concerns about the correctness here?

-- NOTE: This should be OK because you can data declarations can only appear at the top-level.
declToCoreFn mn (A.DataBindingGroupDeclaration ds) = wrapTrace "declToCoreFn DATA GROUP DECL" $ concat <$> traverse (declToCoreFn mn) ds
-- Essentially a wrapper over `exprToCoreFn`. Not 100% sure if binding the type of the declaration is necessary here?
-- NOTE: Should be impossible to have a guarded expr here, make it an error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should've been a TODO, right? Please address it as well

-> m (CaseAlternative Ann)
altToCoreFn mn ss ret boundTypes (A.CaseAlternative bs vs) = wrapTrace "altToCoreFn" $ do
env <- gets checkEnv
bTypes <- M.unions <$> zipWithM inferBinder' boundTypes bs -- Inferring the types for binders requires some special machinery & knowledge of the scrutinee type. NOTE: Not sure why multiple binders?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why multiple binders?

Maybe due to case a, b, c of syntax (which is not tuple construction)

$ "untyped binding group element in mutually recursive LET binding group after initial typechecker pass: \n"
<> LT.unpack (pShow $ lefts types)
where
go :: ((SourceAnn, Ident), A.Expr) -> Either ((SourceAnn,Ident), A.Expr) ((SourceAnn, Ident), (A.Expr, SourceType))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(SourceAnn,Ident) is present in both alternatives. Consider moving it out of Either

guardToExpr [A.ConditionGuard cond] = cond
guardToExpr _ = internalError "Guard not correctly desugared"

{- Dirty hacks. If something breaks, odds are pretty good that it has something do with something here.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{- Dirty hacks. If something breaks, odds are pretty good that it has something do with something here.
{- Dirty hacks. If something breaks, odds are pretty good that it has something to do with something here.




{- "Type Constructor analysis" machinery. (This requires some explaining)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great explanation!

Comment on lines +218 to +219
in error $ "Internal error. Expected a function type, but got: " <> showty
{- This function more-or-less contains our strategy for handling polytypes (quantified or constrained types). It returns a tuple T such that:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
in error $ "Internal error. Expected a function type, but got: " <> showty
{- This function more-or-less contains our strategy for handling polytypes (quantified or constrained types). It returns a tuple T such that:
in error $ "Internal error. Expected a function type, but got: " <> showty
{- This function more-or-less contains our strategy for handling polytypes (quantified or constrained types). It returns a tuple T such that:


deriving instance Eq a => Eq (Module a)

data DiffResult a =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain the purpose of this module within the larger context

| C.I_functionApply <- fn -> App a x y
| C.I_functionApplyFlipped <- fn -> App a y x
(App a t1 (App _ t2 (Var _ t3 fn) x) y)
| C.I_functionApply <- fn -> App a t1 x y -- NOTE @klntsky not sure about the type here, needs reviewed. I *think* the type shouldn't change?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, please remove the comment

prettyTypeStr = T.unpack . smartRender . asOneLine prettyType


{- TYPES (move later) -}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{- TYPES (move later) -}
{- TOD: TYPES (move later) -}

@@ -122,12 +122,14 @@ updateCacheDb codegenTargets outputDirectory file actualFile moduleName = do
let moduleCacheInfo = (normaliseForCache cwd (fromMaybe file actualFile), (dayZero, contentHash))

foreignCacheInfo <-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a todo to make this used? If not, please remove

Comment on lines +123 to +124
-- pTrace regrouped
-- pTrace exps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncomment these and make traces visibility configurable by a global var or a flag

@@ -75,7 +78,7 @@ convertPrettyPrintType = go
-- Guard the remaining "complex" type atoms on the current depth value. The
-- prior constructors can all be printed simply so it's not really helpful to
-- truncate them.
go d _ | d < 0 = PPTruncated
-- go d _ | d < 0 = PPTruncated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit more of these in this module

=> [SourceType]
-> SourceType
-> [CaseAlternative]
=> [SourceType] -- the types of the scrutinee values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use doc-comments

Suggested change
=> [SourceType] -- the types of the scrutinee values
=> [SourceType] -- ^ the types of the scrutinee values

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

Successfully merging this pull request may close these issues.

3 participants