parsonsmatt / intero-neovim

A neovim plugin for Intero, forked from ghcmod-vim
218 stars 28 forks source link

Ability to inspect when code doesn't type-check #84

Open unode opened 7 years ago

unode commented 7 years ago

The ability to inspect types and info about any function or variable relies on code compiling successfully. The plugin is smart enough to keep previous information about types but often it leads to misleading or incomplete information when code moves up/down in the file.

ghc includes -fdefer-type-errors and -fdefer-typed-holes (and possibly others) which allow for a successful compilation which fails type-checking.

Would it make sense to include these options by default while using Intero in order to ensure we always keep the ability to inspect code?

decentral1se commented 7 years ago

Hey @unode, a nice idea! After https://github.com/parsonsmatt/intero-neovim/pull/72, it's possible to pass these options in with g:intero_ghci_options. As for setting them as default, I'm not sure. I don't think there are any drawbacks to these options or? Perhaps a little too auto-magical?

unode commented 7 years ago

I don't know enough about those options either. I asked around for ways to have the code compile even when types don't match so one could still inspect and got those 2 recommendations from folks on #haskell@freenode. The binary produced is clearly flawed if types don't match but I guess that's the point.

There's also -fno-code which was mentioned as a way to make ghc-mod faster. It skips compilation and only type-checks. I haven't tried it but a priori I'm not sure this one is always desireable so it might be useful but perhaps not as default.

Edit: @lwm g:intero_ghci_options is undocumented. Perhaps it could benefit from a brief mention in the docs?

decentral1se commented 7 years ago

Ah yes, I've opened https://github.com/parsonsmatt/intero-neovim/issues/85 to track that. Yes, -fno-code wouldn't be a good default. I thought a little about this again and I think with intero_ghci_options available, we should probably avoid forcing a default when it is easily configurable. Perhaps, down the line, some configuration options will be incompatible or conflict or whatever.

decentral1se commented 7 years ago

I've opened up https://github.com/parsonsmatt/intero-neovim/pull/88 now. Hopefully @rdnetto or @parsonsmatt will chime in here to resolve this one.

rdnetto commented 7 years ago

IMO, by default we should match the default behaviour of stack build and upstream Intero, which is to fail compilation on type errors. There are a few reasons for this:

The real reason we want this in the first place is that we want to inspect code that doesn't compile at all, regardless of whether that's due to a type error, a syntax error, or something else entirely. One way to achieve this might be to keep the old version of a module around if the new version fails to load. I think that fixing that for all compilation errors (rather than a subset) would be a better solution, though it's going to require changes in Intero which is obviously more work.

unode commented 7 years ago

@rdnetto aren't those two approaches somewhat conflicting? I don't see how it would be possible to have inspectable incomplete/broken code if you don't allow for partial/incomplete compilation and deviate from standard build options. I think it should be safe to assume that in a development scenario you might be working with a "debug-enabled" build, rather than a "regular or release" build.

For instance:

  1. How would you handle code that has never successfully compiled or any new construct that wasn't present in previous compilation cycles?
  2. With regards to inspecting an "old version", how do you decide if the "old" is "outdated"? Getting misleading information is arguably worse than no information at all.
rdnetto commented 7 years ago

I don't see how it would be possible to have inspectable incomplete/broken code if you don't allow for partial/incomplete compilation and deviate from standard build options.

You can get there if you inspect the latest working build, instead of the current state (which may be broken).

(Deviating from the standard build options in a way not defined by the end user is the part here that bothers me, given it will almost certainly break due to some combination of incompatible options)

How would you handle code that has never successfully compiled or any new construct that wasn't present in previous compilation cycles?

If the module/function has never compiled, there's not much we can do about it. Inspection needs to operate on a best-effort basis, given that code under development is going to be broken most of the time.

With regards to inspecting an "old version", how do you decide if the "old" is "outdated"? Getting misleading information is arguably worse than no information at all.

That's a more interesting question. One approach would be to always provide whatever information we have, and leave it up to the user to decide if it made sense or not. Another would be to check for things like differing type signatures, and invalidate the cached symbols based on that.

What approach we take to invalidation will probably depend on the granularity of the cache though, which makes it hard to figure out until we've actually spiked or implemented it.

unode commented 7 years ago

You can get there if you inspect the latest working build, instead of the current state (which may be broken).

This "broken" state might be something that we can work with and avoid some of the other hurdles.

I'm now using the above mentioned options by default and it helps in some cases. I still need to get more familiar with them and test it throroughly to assess what are the downsides.

unode commented 7 years ago

I've been giving -fdefer-type-errors and -fdefer-typed-holes a more exhaustive try. The use of these options does provide additional inspection capabilities but at the moment Intero is not prepared to handle the output produced failing to parse warnings.

This leads to useless messages being displayed in the neovim status bar:

intero: [-Wunused-matches] (w)

instead of

intero: error: • Couldn't match expected type ‘IO [Annotator]’ (E)

Regardless, the interactive session window provides all the information at the cost of manual scrolling when warnings/errors are too verbose.

On the positive side, since the build succeeds, even in cases when types are incorrect, inspection of code is possible to some degree.