Analyze code from dependencies

[jfmengels]

The problem

By only analyzing the source code, elm-review encounters a few knowledge gaps when we need to know implementation details from code that comes from dependencies

• https://github.com/jfmengels/elm-review-rule-ideas/issues/7: Detect functions in a type to prevent crashes during Model comparisons and the Elm debugger from not working
• UnreachableCase rule elm-review-rule-ideas#8: Detect/evaluate whether the code can enter a dangerous case.
• NoUnused.CustomTypeConstructors also has a use-case for detecting the phantom types in the types from dependencies, as that information is lost when we encounter opaque types in the docs.json.
• If we want to detect that Html.lazy is used correctly, we sometimes need to know whether a function creates a new reference to a value or not. To know that, we need to look at the implementation of functions, including those from dependencies.

What would be needed

• Need to analyse the dependencies in the elm-review API
  • A simple solution would be to have a visitor that just takes all the dependencies information, and let the user use that.
  • A much better solution would be re-using the visitor pattern that we have.
    • We could use project visitors that can't return errors, but just fetch information.
    • We might need another layer of ModuleContext/ProjectContext, since you would need to analyze a dependency, store the resulting context (WorkspaceContext? ProjectContext would have fit nicely here...), then kickstart the analysis of a dependency using that WorkspaceContext with a system like withModuleContext but on a higher-level.
• We would need to handle naming collisions, since there could be several files with the same name. Make sure that project code can't import modules from indirect dependencies.
• Provide a way to give dependencies information in a Project
• Provide a way to create dependencies for tests (in a non-painful way...)
• Provide a way to test the encoding/decoding of the dependencies analysis
  • We could re-run a rule that uses the dependencies information after encoding/decoding the analysis, and check that all results are equal (and the context too?)
• Decide what to do if the decoding of the cache fails. Abort elm-review and display an error?
  • Maybe we could try to decode it before saving it to the file-system. If that fails, then... we throw an error? We could ignore the problem but then performance will be quite impacted.

Performance improvements

• If no rules in the configuration requires access to this information, don't load/fetch/access it. This would require some introspection
• Cache the result
  • Ask the rule author for a way to return a Json.Encode.Value. (Bytes would be nice too?)
  • The cache would be stored under the hash of the configuration, because it is configuration dependent. We could create a hash of the combined set of dependencies and versions + rule name, and store it in FS using that information.
  • Each dependency would probably have its own cache entry, so that we don't have to recompute every dependency if a single one is changed. We could then combine them just like we do for project contexts. It would also help prevent recomputing packages like elm/core multiple times.
• If the cache for this set of dependencies and rule exists, load it, but don't try to load/download the dependencies data.
• Discard the dependencies data once we don't need it anymore (in watch mode), so that it can be garbage-collected.
• Clean up outdated cache files after some time? I guess it depends on how much information is usually stored in the resulting context. I am guessing not much, but I can't be sure.
• As I mentioned above, the cache would be stored under the hash for the configuration. Meaning that if you change a single thing in your configuration (ReviewConfig, elm.json or custom rule), then we need to re-run this computation again. That is because we have little ways of knowing whether that rule's behaviour hasn't changed (its implementation, the implementation of one of its imports, ...). It would be nice to better finger-print this cache, so we re-compute this cache less, especially when it is non-configurable and comes from a package.

Fetching dependencies

Dependencies may not be available. That should be relatively rare though, so the happy path that we should try first is to fetch the data from the file system in the ELM_HOME.

If some dependencies are not available, we could do either of two things:

1. Warn the user that they need to compile first
2. Run elm make ourselves

Running elm make ourselves

We can simply run elm make, we don't need a target file. If the project is a package, that will be sufficient to run the compiler. If the project is an application, that is not sufficient for the compiler to run, but thankfully the Elm compiler downloads the packages beforehand (If that behaviour ever changes, we should fall back to displaying an error message). We then ignore the error.

If there is no input (for applications), then downloads were successful and we get an error with the title NO INPUT. If there is no network access, we get PROBLEM LOADING PACKAGE LIST. If the user messed the dependencies by hand, we get INCOMPATIBLE DEPENDENCIES.

Notes

This is definitely not light work :D
I think it is a lot of work for a small benefit. That said, once we have it, elm-review would potentially become much more powerful. I kind of like elm-review becoming more "omniscient".

I am worried about the performance impact this would have, as this would add a lot of additional work, especially in the initial runs and when the configuration or dependencies change.

Note that even with this, we still have a few knowledge gaps:

• What happens in non-Elm files (JS, CSS, HTML, list of images in the project, REST/GraphQL schemas, ...)
• What happens in native modules from core packages.

I don't know about the second gap, but I am interested in exploring the non-Elm files realm. Please open an issue if you want to discuss those two points, I just wanted to point out that even with this, there would still be some knowledge gaps (though a lot less!).

[MartinSStewart]

I think it's worth mentioning that while I think this would be a wonderful addition (provided all the technical hurdles can be overcome), this won't be useful when making packages since packages define a dependency range "elm-community/list-extra": "8.0.0 <= v < 9.0.0", in contrast with applications which define a specific dependency version.

Should the user try to run a rule inside a package that requires information about dependency source code, what should happen? I guess elm-review should have the rule fail and say that, that rule is only for applications?

[jfmengels]

this won't be useful when making packages since packages define a dependency range

I think it would make sense for packages, as long as there are use-cases where packages would like to look at their dependencies.

At the moment, elm-review is already getting the dependencies
packages get to see their dependencies too using withDependencies*Visitor where we take the lower-bound version (1.0.1 is the constraint is 1.0.1 <= v < 2.0.0).

Since in this case, we want to get those files using elm make, we will probably change to take the latest matching versions on disk. I just tried it out, and elm make downloads the latest version, so I'm guessing it will also use the latest version to make the project.

I am not yet convinced that this should not be available for packages. But the part where one user could have different results than another one based on what they have available on their machine does bug me. In practice, I imagine that the results will rarely be different, but it is a good point to think about...

[MartinSStewart]

True, I overstated my case against supporting dependencies for packages. There might be rules that still are useful. With https://github.com/jfmengels/elm-review-rule-ideas/issues/7 it's effectiveness is reduced but it probably can still be somewhat useful for packages (though if the user doesn't understand the implications then it might give a false sense of security). With jfmengels/elm-review-rule-ideas#8 I think it's best if the rule refuses to work with packages to avoid a pretty big footgun.