Skip to content

Conversation

@cloudRoutine
Copy link
Contributor

While working with FSharp.Compiler.Service to build features for VFPT and FSAC the lack of fieldnames has added a lot of friction to the development process. The are many cases where the type signature is not enough to understand what the field in a DU case are used for or represent, so I've added field names based on the comments in the source file.

WIP, but a review and some guidance are necessary before more progress can be made.

Naming Conventions Employed

  • range : the full range of UAST construct
  • leftAngleRange : the range of <
  • rightAngleRange : the range of >
  • id : Ident
  • longId : LongIdent
  • dotId : LongIdentWithDots
  • attributes : SynAttributes
  • xmlDoc : PreXmlDoc
  • accessibility: SynAccess option
  • constant : SynConst
  • genericName : SynTypar
  • memberSig : SynMemberSig
  • constraints : SynTypeConstraint list
  • case : SynUnionCase
  • valSig : SynValSig
  • memberFlags : MemberFlags
  • memberSigs : SynMemberSigs
  • typeParams : SynValTyparDecls, SynTyparDecls
  • bindings: SynBindingList
  • moduleDecl : SynModuleDecl
  • moduleSigDecl : SynModuleSigDecl
  • typeDefn : SynTypeDefn
  • hashDirective : ParsedHashDirective
  • exnRepr : SynExceptionRepr
  • exnSig : SynExceptionSig
  • typeDefSig : SynTypeDefnSig
  • spTarget : SequencePointInfoForTarget
  • field : SynField

Questionable Naming Conventions

  • typeName : SynType
  • members : SynMemberDefns

Changes From Inconsistent Comment Naming Conventions

  • LESSm -> leftAngleRange
  • GREATERm -> rightAngleRange
  • m, wholem, mwholeExpr, wholeRange, rangeOfWholeExpr -> range

Types Without A Naming Convention

  • SynPat
  • SynValInfo
  • SynTypar
  • SynTypeDefnKind
  • SynTypeDefnSimpleRepr
  • SynTypeDefnSigRepr
  • SynModuleOrNamespace
  • SynModuleOrNamespaceSig
  • SynComponentInfo
  • SynModuleSigDecls
  • ParsedImplFileFragment list
  • ParsedSigFileFragment list

Convention Questions :

Regarding Underlying Type Information

  • Currently optional fields have the same name as their base type (id is used for both Ident & Ident option)
    Should the field names for options have an 'Opt' suffix? (idOpt)
  • Should field names for types aliases like SynModuleSigDecls have a List suffix to indicate the underlying type?
  • Should cases with one field get fieldnames? (| Ident of Ident)
  • Should fields of type range have range in their names?

Unclear on

Once ast.fs is finished I'll add union case field names to tast.fs

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is not needed anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm saving cleaning up the comments for last 😉

@dsyme
Copy link
Contributor

dsyme commented Jan 9, 2016

After this change i accepted we will also need to cherry-pick it into Microsoft/visualfsharp to maintain consistency and alignment.

Alternatively it should be submitted there first - in some ways that would be easier since we currently integrate from Microsoft/visualfsharp --> fsharp/fsharp --> fsharp/FSharp.Compiler.Service. However I do see that the change is particularly relevant to users of FCS

@dsyme
Copy link
Contributor

dsyme commented Jan 9, 2016

It's a good change - a great contribution. I'll need to look at the specific questions above more closely - I'll try to get to that later today

@dsyme
Copy link
Contributor

dsyme commented Mar 30, 2016

@cloudRoutine I'm going to pull this as is at last. Please cherry pick it into Microsoft\visualfsharp if you have time, that would be very much appreciated.

The easiest way to document the answers to the question above is just for me or someone else who knows the compiler well to prepare a diff with names that answer the questions :)

@dsyme dsyme merged commit 6107e05 into fsharp:master Mar 30, 2016
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