jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
252 stars 13 forks source link

Find correct way to reference function or type #30

Open MartinSStewart opened 3 years ago

MartinSStewart commented 3 years ago

Sometimes I want to generate some code containing a call to a function or type that's defined in another module. Determining how to qualify that function or type is tricky though. Is it exposed directly (no qualification is needed) or is there a import alias I should use? Maybe it's a function from an implicitly imported module like Platform.Cmd? Maybe it's not imported at all and I can't reference this function or type.

Instead elm-review could provide something like this getQualifiedReference : ModuleNameLookupTable -> ModuleName -> FunctionOrValue -> Maybe ModuleName This function takes the fully qualified module name for the function or type and then returns the version that accounts for import aliases and exposings. Nothing is returned if the function or type does not exist or there is no import for it.

jfmengels commented 3 years ago

Sounds interesting. I have a few thoughts.

  1. getQualifiedReference: It will sometime return Just [] (because of things like import A exposing (..)), meaning it is not technically "qualified". So I'm thinking we could find a better name for it.

  2. The qualified reference depends on where you are in the scope.

import Html exposing (input)

view model =
  -- Adding a reference to `input` from `Html` here would just require `input`
  viewInput model

viewInput model =
  -- Adding a reference to `input` from `Html` here would require `Html.input`
  let
    input = Debug.todo ""
  in
  input

So we can either choose to always say Html.input by having the function always return at least the module name, but that would not be ideal (potentially fixed by elm-review-imports rules?). That or we would need more information, like the position, which would make the function a lot more complex.

  1. What is FunctionOrValue in your example? A String?

  2. What happens when the function returns Nothing? I'm guessing that you then have to insert an import statement. Again, it would be pretty cool if elm-review could figure that position for you, but that's not what I want to talk about here.

I imagine that you have several options as to how to add that import

  1. Import explicitly

    import Html exposing (input)

    Problem: if input is a top-level declaration or imported from somewhere else, then you have a compiler error.

  2. Used qualified import

    import Html -- then reference as Html.input

    Problem: if there's a import Html.Styled as Html, then you again have a conflict. If your module name is very long, you might want to add a type alias, but then you have a similar problem if another import has that name or has that alias.

What I mean to say, is that I feel like having that getQualifiedReference function would be useful, but it doesn't cover the whole story that it would need to serve.

I think that an exploration into this would be really useful. It could be implemented through additional visitors, like elm-review-scope did before it was integrated into elm-review.

MartinSStewart commented 3 years ago

You have a lot more insight into the challenges of adding getQualifiedReference than I do! The impression I get is that maybe elm-review needs to move up a level in terms of abstraction before features like this can be well supported. In other words, instead of operating on an untyped AST and letting rules insert/replace raw strings. Rules would have access to type information and fixes would change the AST with elm-review not just doing an elm-format pass but also adding missing imports. For now I think it makes more sense to try implementing something "good enough" in my rule and seeing where it goes.