fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Refactoring: splitting a chunk from CheckExpressions

Open psfinaki opened this issue 3 years ago • 2 comments

psfinaki avatar Aug 09 '22 15:08 psfinaki

@psfinaki Basically this looks good, though I'd like to do some more on it next week to reap more benefit from the changes.

I pushed some adjustments

  • Rename CheckTypes.fs/fsi to CheckBasics.fs/fsi. We will use "CheckTypes.fs/fsi" for TcType and friends.

  • I moved one thing back to CheckExpressions.fs since it wasn't specifically needed in CheckBasics.fs (the purpose of CheckBasics should be to establish TcEnv and TcFileState and nothing else).

  • I'd imagine we will move most of the helpers in CheckExpressions.fs which only relate to TcEnv and TcFileState and the other such types into CheckBasics.fs. I pushed one example of this - often this will involve making things into members according to the types they operate on. This is part of the benefit we can gain by this - more of the logic of CheckExpressions.fs will migrate out and be organized differently.

  • I guess we may eventually split CheckBasics itself into TcEnv.fs and TcFileState.fs though no need to do that for now.

We can merge this for now I think and get it in the bank, then iterate next week.

dsyme avatar Aug 11 '22 17:08 dsyme

Just to note that, once we iterate, this lays the foundation for further splitting, e.g.

  • CheckAttributes.fs/fsi for TcAttribute* and friends
  • CheckTypes.fs/fsi for TcType*
  • GeneralizationHelpers.fs/fsi (helpers related to generalization)
  • EliminateInitializationGraphs.fs/fsi (EliminateInitializationGraphs and friends)
  • CheckIndexAndSlice.fs/fsi (TcIndexerThen* - quite a large chunk)
  • CheckObjectExpressions.fs/fsi (TcObJExpr* - quite a large chunk)
  • CheckBindings.fs/fsi (normalizing bindings, TcLet and incremental generalization of groups of recursive bindings)
  • CheckStrings.fs/fsi (checking format strings, interpolated strings)

dsyme avatar Aug 11 '22 17:08 dsyme

@dsyme thanks for the review and the adjustments. Yeah here we already have quite a lot of changes, so merging this one - and hope we'll address your points above in the near future :)

psfinaki avatar Aug 12 '22 11:08 psfinaki