Smart cst

[srghma]

what this pr does:

• it calculated imports based on SmartQualifiedName

this is how it is defined https://github.com/srghma/purescript-ps-cst/blob/master/src/Language/PS/SmartCST/Types/SmartQualifiedName.purs

---

• the function moduleToCstModule is an entry point

• mostly all logic is in https://github.com/srghma/purescript-ps-cst/blob/master/src/Language/PS/SmartCST/ProcessSmartDeclaration.purs#L27

this method populates Array ImportDecl IIF it is not current module SmartQualifiedName

• there is a https://github.com/srghma/purescript-ps-cst/blob/master/sync-cst-declaration-to-smart-cst.sh script, for easier syncing

[srghma]

@paluh @flip111 @jvliwanag could you review it please

[paluh]

Yeah, sure. But please be aware that there is not much value in my quick review because I can only comment on styling / naming issues as I'm not familiar yet with the overall structure of the lib :-(

[paluh]

@srghma before I submit anything could you please shortly describe what is the main idea behind SmartCST*? Are you going to introduce this new namespace (beside just CST)? What are the main differences between these ASTs? Would it be possible to achieve the same by annotating "stupid" cst nodes or is it required to introduce separate type set? Sorry if the above are stupid questions but this diff is really huge...

[srghma]

np)

what is the main idea behind SmartCST*?

what this pr does:

it calculated imports based on SmartQualifiedName

i.e. there are Declaration with QualifiedName inside, but I also added Declaration with SmartQualifiedName (and SmartQualifiedNameConstructor)

Are you going to introduce this new namespace (beside just CST)?

yes

What are the main differences between these ASTs?

the diff is in https://github.com/srghma/purescript-ps-cst/blob/master/sync-cst-declaration-to-smart-cst.sh

I basically replace

sed -i 's/QualifiedName (ProperName ProperNameType_ConstructorName)/SmartQualifiedNameConstructor/g' src/Language/PS/SmartCST/Types/Declaration.purs
sed -i 's/QualifiedName/SmartQualifiedName/g' src/Language/PS/SmartCST/Types/Declaration.purs

and add ExprVar Ident

Would it be possible to achieve the same by annotating "stupid" cst nodes or is it required to introduce separate type set?

I have tried to move QualifiedName as argument, but then noticied that I need to move QualifiedName (ProperName ProperNameType_ConstructorName) as an argument too. I became messy, that started to use https://github.com/srghma/purescript-ps-cst/blob/master/sync-cst-declaration-to-smart-cst.sh

[flip111]

Hi @srghma thanks for your contribution once again !

I have tried to move QualifiedName as argument, but then noticied that I need to move QualifiedName (ProperName ProperNameType_ConstructorName) as an argument too.

Can you tell me more about how the option of a single namespace becomes messy in practice? I would prefer if we had only 1 CST that contains "features" that either be used or not used.

How does one go from converting CST to SmartCST and SmartCST to CST ?

[srghma]

How does one go from converting CST to SmartCST

by hand one should replace imports of import Language.PS.CST with Language.PS.SmartCST

and then QualifiedName with constructors of SmartQualifiedName

here is an example https://github.com/purescript-graphql-client/purescript-graphql-client/blob/5fe4e7022833f5d091a8b3107b9e72630e0f64b2/generator/GraphqlClientGenerator/MakeModule/Enum.purs#L29

and SmartCST to CST ?

one can use this function https://github.com/srghma/purescript-ps-cst/blob/b968f77058fa8fcb14907e90eb1fe25fd47478b4/src/Language/PS/SmartCST/ProcessSmartDeclaration.purs#L293

or this https://github.com/srghma/purescript-ps-cst/blob/b968f77058fa8fcb14907e90eb1fe25fd47478b4/src/Language/PS/SmartCST/ProcessModule.purs#L21

which is using the first, but with additional logic - removes current module import from import list, so purescript won't throw an error

I could do that in https://github.com/srghma/purescript-ps-cst/blob/b968f77058fa8fcb14907e90eb1fe25fd47478b4/src/Language/PS/SmartCST/ProcessSmartDeclaration.purs#L27 and https://github.com/srghma/purescript-ps-cst/blob/b968f77058fa8fcb14907e90eb1fe25fd47478b4/src/Language/PS/SmartCST/ProcessSmartDeclaration.purs#L194 modify_ function - but I decided to do that on the last stage

P.S. there are speed improvements possible

in https://github.com/srghma/purescript-ps-cst/blob/b968f77058fa8fcb14907e90eb1fe25fd47478b4/src/Language/PS/SmartCST/ProcessSmartDeclaration.purs#L27 and https://github.com/srghma/purescript-ps-cst/blob/b968f77058fa8fcb14907e90eb1fe25fd47478b4/src/Language/PS/SmartCST/ProcessSmartDeclaration.purs#L194 modify_ function an Array is used, it's possible to replace it with Map and transform Map to Array ImportDecl on a later stage

Can you tell me more about how the option of a single namespace becomes messy in practice?

yes, we could remove CST and rename SmartCST to CST and not use CST as an intermediate layer, but I decided that CST is a purescript internal representation and is something that won't go away

but there can be many SmartCST implementations

e.g. alternative implementations where

  | DeclSignature
    { comments :: Maybe Comments
    , ident :: Ident
    , type_ :: Type
    }
  | DeclValue
    { comments :: Maybe Comments
    , valueBindingFields :: ValueBindingFields
    }

are joined together to make the purescript error value should be followed after signature impossible

[flip111]

by hand one should replace imports of import Language.PS.CST with Language.PS.SmartCST

I meant if it's possible to have them operate on runtime or if there now two incompatible CST in the same codebase. I guess that since there are converting functions that it works together in runtime.

yes, we could remove CST and rename SmartCST to CST and not use CST as an intermediate layer, but I decided that CST is a purescript internal representation and is something that won't go away

This is interesting and something i didn't understood from your previous posts. I thought we had now two competing CST. But actually SmartCST would be the public facing API and CST for internal as i understand it now? Maybe put CST in Language.PS.Internal.CST and SmartCST in Language.PS.CST??

[srghma]

if there now two incompatible CST in the same codebase

why they are incompatible? SmartCST = CST + ExprVar - QualifiedName + SmartQualifiedName

Maybe put CST in Language.PS.Internal.CST and SmartCST in Language.PS.CST??

is this really needed? I'm not against, but if we want people to make their own variants of SmartCST that use CST internally, then maybe it's not good idea

[srghma]

I'll merge since I think all is right

[paluh]

@srghma - My review was really cosmetic so it is good that you have merged it as it is ;-)

[srghma]

@paluh tnx anyway)

[flip111]

Thanks @srghma :)