RFC: Context creator

[jfmengels]

I have been playing (and doing a lot of refactoring to make it work) with creating rules where the context can be initialized with some pre-computed data. This is what it would look like:

rule : Rule
rule =
    Rule.newModuleRuleSchemaWithContextCreator "NoDebug.TodoOrToString" contextCreator
        |> Rule.withImportVisitor importVisitor
        |> Rule.withExpressionEnterVisitor expressionVisitor
        |> Rule.fromModuleRuleSchema

contextCreator : Rule.ContextCreator () Context
contextCreator =
    Rule.initContextCreator
        (\metadata () ->
            { moduleName = Rule.moduleNameFromMetadata metadata
            , moduleNameNode = Rule.moduleNameNodeFromMetadata metadata
            , isInSourceDirectories = Rule.isInSourceDirectories metadata
            }
        )
        |> Rule.withModuleMetadata

The API for the "context creator" is similar to the JSON decode pipeline. Every with adds an argument to the function.

Right now, the available information is:

• Metadata about the current module, containing
  • The node to the module name (just like we do in fromProjectToModule)
  • The module name (in case you don't care about the node)
  • isInSourceDirectories, a boolean that tells you whether the module is in the source-directories. This will allow giving some different behavior for tests/review code (like
• The moduleKey, like the one fromProjectToModule (I still need to forbid this when you are in the context of a module visitor)

Some others pre-computed that I am thinking of:

• A lookup table to know the "real" module name of a type/value, based on the range of the Node. Ideally, this would replace elm-review-scope.
• The modules' signatures (this might actually be passed through a visitor, like the dependencies visitor, not sure yet).
• Type inference, in a similar way to the lookup table. Not for today though :sweat_smile:

The benefit of this system is also that we can compute most of this once, before rules are run, instead of once for every rule that wants that information (and needs to collect it on their own).

Why not give all the information at once?

• That would be a lot of parameters/fields, most of which you'll ignore. And ignoring parameters at the very least is annoying
• I expect we'll add more as time goes, and adding a parameter would mean a breaking change
• Some of these might be expensive to compute (type inference, https://github.com/jfmengels/elm-review/issues/15, ...), and we could prevent computing them if we notice that none of the rules require that information (which we can with this system).

A few points:

• For project rules, you can also choose to have fromProjectToModule and fromModuleToProject to also use this context creation construct.
• I am looking to get rid of that () in the context creator. That is meant to be the projectContext, but for module rules that is simply (). It's annoying but we'll see if I can manage to get rid of it :man-shrugging:
• The names I chose are probably not great, I'd love to get input on them these.
• I wish I could break some of these functions (context creation, metadata, ...) into different modules, but unfortunately, that would mean a breaking change because of an issue in the compiler... :disappointed: So I'll leave that for later.
• Under the hood, all modules rules are transformed to project rules. I now only have one implementation of rules to maintain.
• It could be interesting to have some of these information be dynamic (in other words, you provide a hook), and that hook would only be computed once for every rule that would use the same one. I'm not sure that will turn out great though, so it's more likely than not that this won't get in.

What I am looking for

Feedback

I am looking for feedback on this feature. Do you think this is useful, great, cumbersome, overkill?

Bikeshedding

I am looking for better names to the functions I shown here. Note that I might need to have a separate context creator for module visitors than for project visitors.

---- Review.Rule - MINOR ----

    Added:
        type ContextCreator from to
        type Metadata 
        initContextCreator : (from -> to) -> Review.Rule.ContextCreator from to
        isInSourceDirectories : Review.Rule.Metadata -> Basics.Bool
        moduleNameFromMetadata :
            Review.Rule.Metadata -> Elm.Syntax.ModuleName.ModuleName
        moduleNameNodeFromMetadata :
            Review.Rule.Metadata
            -> Elm.Syntax.Node.Node Elm.Syntax.ModuleName.ModuleName
        newModuleRuleSchemaUsingContextCreator :
            String.String
            -> Review.Rule.ContextCreator () moduleContext
            -> Review.Rule.ModuleRuleSchema {} moduleContext
        withMetadata :
            Review.Rule.ContextCreator Review.Rule.Metadata (from -> to)
            -> Review.Rule.ContextCreator from to
        withModuleContextUsingContextCreator :
            { fromProjectToModule :
                  Review.Rule.ContextCreator projectContext moduleContext
            , fromModuleToProject :
                  Review.Rule.ContextCreator moduleContext projectContext
            , foldProjectContexts :
                  projectContext -> projectContext -> projectContext
            }
            -> Review.Rule.ProjectRuleSchema
                   { schemaState
                       | canAddModuleVisitor : ()
                       , withModuleContext : Review.Rule.Required
                   }
                   projectContext
                   moduleContext
            -> Review.Rule.ProjectRuleSchema
                   { schemaState
                       | hasAtLeastOneVisitor : ()
                       , withModuleContext : Review.Rule.Forbidden
                   }
                   projectContext
                   moduleContext
        withModuleKey :
            Review.Rule.ContextCreator Review.Rule.ModuleKey (from -> to)
            -> Review.Rule.ContextCreator from to

Does it even make sense to have a Metadata field, when we could do withModuleName, withModuleNameNode, withIsInSourceDirectories? I am thinking this might be easier and I am not sure what other fields I might add later to the metadata. Also, I think it's better for re-runs that rules don't store the Metadata itself inside the context (but not sure that will impact anything yet). I am leaning towards that at the moment.

Should ContextCreator be named ContextBuilder, or something else entirely?

People trying it out

Try it out on your rules, and let me know what you think. Let me know if you find nice patterns that we can suggest to use in the documentation. I think the easiest way to try it out for now is to checkout this repo and create a rule in the tests.

You can find the documentation here: https://elm-doc-preview.netlify.app/?repo=jfmengels/elm-review

[MartinSStewart]

This is a lot to think about so I don't think I have any good response to your feedback question.

A lookup table to know the "real" module name of a type/value, based on the range of the Node. Ideally, this would replace elm-review-scope.

That said, this might be a good feature request for elm-syntax?

[jfmengels]

A lookup table to know the "real" module name of a type/value, based on the range of the Node. Ideally, this would replace elm-review-scope.

That said, this might be a good feature request for elm-syntax?

It would, but I do not believe that elm-syntax can do the best job at this, especially with regards to re-computing the lookup table, at least with the current processing approach.

I guess it could do an efficient work if we add all files (with Elm.Processing.addFile) to the processing context, and then post-process them with all the knowledge (Elm.Processing.process).

But imagine module A gets modified after that initial processing (in watch or fix mode) and stops exposing something. That can have an impact on the lookup table of module B that imports module A. elm-syntax doesn't have the current capability to re-generate the lookup table for module B, whereas elm-review already does this kind of work, almost at its core.

I would be glad to be proved wrong though, if you have any ideas :blush:

(cc @stil4m)

[MartinSStewart]

Sorry, I was unclear. I'm thinking that it would be a good feature for elm-syntax to support resolving the module name for a function or type from a list of files. Caching the results from that would still be the responsibility of elm-review.

[jfmengels]

I'm thinking that it would be a good feature for elm-syntax to support resolving the module name for a function or type from a list of files

I am a bit unclear as to what you mean here. Do you mean to be able to tell things like this below?

module B exposing (..)

import Html.Styled as Html
import A exposing (..)

a =
  identity --> this comes from module `Basics`
    b      --> This is local

b =
  Html.div --> this comes from module `Html.Styled`
    attributes --> This comes from module `A`

If so, that is what I meant. In my previous comment, the problem I wanted to highlight, is if A stops exposing attributes, then we'd need to somehow have elm-syntax re-analyse (re-parse?) B to get the correct lookup table.

If you meant something else, then I didn't get it.

[MartinSStewart]

Ah, I misunderstood your question then. Yes, I think you're right then that adding such a feature to elm-syntax wouldn't help elm-review very much.

[jfmengels]

Implemented and released with 2.3.0 :tada: