Skip to content
This repository has been archived by the owner on Oct 5, 2024. It is now read-only.

clarify that a var name may refer to a type #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

harrysarson
Copy link
Contributor

@harrysarson harrysarson commented Aug 5, 2020

I am confident this is how we use VarName's internally already as
I found this snippet from the Elm.Data.Exposing module:

  type ExposedItem
      = ExposedValue VarName -- exposing (foo)
      | ExposedType VarName -- exposing (Foo)
      | ExposedTypeAndAllConstructors VarName -- exposing (Foo(..))

I am confident this is how we use VarName's internally already as
I found this snippet from the Elm.Data.Exposing module:

  type ExposedItem
      = ExposedValue VarName -- exposing (foo)
      | ExposedType VarName -- exposing (Foo)
      | ExposedTypeAndAllConstructors VarName -- exposing (Foo(..))
@Janiczek
Copy link
Contributor

Janiczek commented Aug 6, 2020

To be more precise, VarName is just a string, and there's eg. the distinction between VarName and ModuleName, even if both are just aliases to a string. The situation with types is that they currently don't have their own TypeName and are just strings:

type ConcreteType a
  = ...
  | UserDefinedType
        { qualifiedness : a
        , name : String <---------------------------------
        , args : List (ConcreteType a)
        }

parseTypeAnnotation "x : MyAwesomeType"
-->
{ varName = "x"
, type_ =
    UserDefinedType
        { qualifiedness = PossiblyQualified Nothing
        , name = "MyAwesomeType"
        , args = []
        }
}

So... yeah we could say "MyAwesomeType" : VarName but perhaps we could instead create

type alias TypeName = String

and use that? eg.

type ExposedItem
    = ExposedValue VarName -- exposing (foo)
    | ExposedType TypeName -- exposing (Foo)
    | ExposedTypeAndAllConstructors TypeName -- exposing (Foo(..))

This ExposedItem was done before we did custom types / type aliases, so ... VarName was probably the best thing we had around back then. And I lacked the clarity to think of anything else :)

@harrysarson
Copy link
Contributor Author

Aha that makes sense, I will update this PR to change ExposedItem instead then!

@harrysarson
Copy link
Contributor Author

Counter point in Elm.Data.Module we have

type alias Module expr annotation qualifiedness =
    -- TODO comments? doc comments?
    { -- TODO somewhere check that dependencies' exposing lists contain only what's in that module's exposing list
      imports : Dict ModuleName Import
    , name : ModuleName
    , filePath : FilePath
    , declarations : Dict VarName (Declaration expr annotation qualifiedness)
    , type_ : ModuleType
    , exposing_ : Exposing
    }

specifically declarations : Dict VarName (Declaration expr annotation qualifiedness) where Declaration can either be a value (with a lower case name) or a type (with an uppercase name).

@Janiczek
Copy link
Contributor

Janiczek commented Aug 7, 2020

Fair enough. I don't see a way from this other than VarOrTypeName which I think is pointless.
Feel free to keep the current diff and change String into VarName in any "type name" context you find!

@harrysarson
Copy link
Contributor Author

The root issue here is that we are using a type alias rather than a custom type, which means keeping these string like types consistent is tricky.

@Janiczek
Copy link
Contributor

Janiczek commented Aug 11, 2020

@harrysarson In an earlier version of this codebase I had type ModuleName = ModuleName String or some such. It worked, but I changed it to a simple type alias ModuleName = String because I felt I'm making unnecessary hoops for users of elm-in-elm/compiler library to jump through. So in the quest for a nice library API I unwrapped all these.

What's your stance on that?
Eg. if creating some of these types from scratch people would suddenly have to do

TypeAlias [ VarName "a" ] (Maybe (Var 0))

instead of

TypeAlias ["a"] (Maybe (Var 0))

(I guess it's not that bad now that I look at it 🤔 )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants