Skip to content
This repository has been archived by the owner on Jan 2, 2021. It is now read-only.

Typecheck entire project on Initial Load and typecheck reverse dependencies of a file on saving #688

Merged
merged 8 commits into from
Sep 2, 2020

Conversation

wz1000
Copy link
Collaborator

@wz1000 wz1000 commented Jul 12, 2020

It is useful to show diagnostics for all files in a project when it is initially loaded.

It is also useful to propagate changes we make in one file to other files which depend on the file we have changed. We only trigger this on saves to avoid annoying users with a barrage of errors, which is also helpful for IDE speed and responsiveness as we don't want to potentially recompile the entire project on every keystroke.

This reduces dependence on kick, as we can precisely target the files we need to reload.

Also fixed a bug where we would cache the iface of a file that compiled due to -fdefer-type-errors. Due to this, on subsequent runs of ghcide, we would not get diagnostics for those files. This is also essential to make the parent type-checking stuff work as expected.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good to me!

src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved
src/Development/IDE/Core/Rules.hs Show resolved Hide resolved
src/Development/IDE/Core/Rules.hs Show resolved Hide resolved
src/Development/IDE/Core/Rules.hs Outdated Show resolved Hide resolved
src/Development/IDE/Core/FileStore.hs Outdated Show resolved Hide resolved
src/Development/IDE/Core/Shake.hs Outdated Show resolved Hide resolved
Comment on lines 215 to 218
liftIO $ do
(log $ "Got Reverse dependencies for" ++ show nfp ++ ": " ++ show revs)
`catch` \(e :: SomeException) -> log (show e)
print (length revs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
liftIO $ do
(log $ "Got Reverse dependencies for" ++ show nfp ++ ": " ++ show revs)
`catch` \(e :: SomeException) -> log (show e)
print (length revs)

src/Development/IDE/Core/FileStore.hs Outdated Show resolved Hide resolved
exe/Main.hs Outdated Show resolved Hide resolved
@wz1000 wz1000 force-pushed the no-kick branch 5 times, most recently from 22f7e4b to 582247c Compare July 14, 2020 17:13
src/Development/IDE/Core/Rules.hs Show resolved Hide resolved
src/Development/IDE/Core/Rules.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cocreature cocreature left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

A few comments:

First on the UX:

  1. I personally prefer having changes propagate to other files immediately. I realize this is not everyone's preference and I’m fine with having both as an option and could be convinced as to what we want the default to be but I don’t want to lose that functionality completely.

  2. I sometimes have a large project that is known to be in a broken state but I only care about some files. By typechecking everything on startup, I get a ton of errors for files I deliberately didn’t open and on very large projects it also slows things down unnecessarily. Not quite sure how to fix that. A CLi flag for switching between the two modes doesn’t seem like a great UX.

  3. What happens if I dynamically add new files? Especially in DAML that is pretty common and something that worked fine so far. It isn’t clear to me if this still works (we should add a test for it).

As for the implementation, my primary concern is around the location of this logic.
The function for setting up sessions is now responsible for modifying knownFilesVar and there is no clear documentation around when and how it can and must modify this. Not quite sure how much we can do here API wise to make this better but least we should have some documentation on optGhcSession.

I’d also like to see some tests for this.

src/Development/IDE/Import/DependencyInformation.hs Outdated Show resolved Hide resolved
modules

Only propagate changes to parent modules when saving

Typecheck files when they are opened, don't TC FOI

Add known files rule

Don't save ifaces for files with defered errors

Co-authored-by: Zubin Duggal <[email protected]>
@wz1000
Copy link
Collaborator Author

wz1000 commented Jul 27, 2020

I personally prefer having changes propagate to other files immediately. I realize this is not everyone's preference and I’m fine with having both as an option and could be convinced as to what we want the default to be but I don’t want to lose that functionality completely.

I sometimes have a large project that is known to be in a broken state but I only care about some files. By typechecking everything on startup, I get a ton of errors for files I deliberately didn’t open and on very large projects it also slows things down unnecessarily. Not quite sure how to fix that. A CLi flag for switching between the two modes doesn’t seem like a great UX.

We can use the LSP configuration request for these

What happens if I dynamically add new files? Especially in DAML that is pretty common and something that worked fine so far. It isn’t clear to me if this still works (we should add a test for it).

Should work fine on adding files, but it will show diagnostics if you delete files.

I've modified the iface tests to check the functionality added by this module.

@wz1000
Copy link
Collaborator Author

wz1000 commented Aug 2, 2020

Added the ability to configure the behavior added by this patch, see the readme for details

@@ -500,6 +502,25 @@ typeCheckRule = define $ \TypeCheck file -> do
-- for files of interest on every keystroke
typeCheckRuleDefinition hsc pm SkipGenerationOfInterfaceFiles

data GetKnownFiles = GetKnownFiles
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to host GetKnownFiles in RuleTypes? It could be useful in plugins

* Use targets to filter located imports

* Remove import paths from the GHC session

Otherwise GHC will prioritize source files found in the import path
@pepeiborra pepeiborra merged commit b4589ae into haskell:master Sep 2, 2020
@wz1000 wz1000 mentioned this pull request Sep 4, 2020
@jneira
Copy link
Member

jneira commented Sep 15, 2020

This closes #216

pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…encies of a file on saving (haskell/ghcide#688)

* Add new command to GetModuleGraph for a session and propate changes to
modules

Only propagate changes to parent modules when saving

Typecheck files when they are opened, don't TC FOI

Add known files rule

Don't save ifaces for files with defered errors

Co-authored-by: Zubin Duggal <[email protected]>

* Add configuration for parent typechecking

* hlint ignore

* Use targets to filter located imports (haskell/ghcide#10)

* Use targets to filter located imports

* Remove import paths from the GHC session

Otherwise GHC will prioritize source files found in the import path

* Update session-loader/Development/IDE/Session.hs

Co-authored-by: Pepe Iborra <[email protected]>

* Add session-loader to hie.yaml (haskell/ghcide#714)

* move known files rule to RuleTypes

* Disable checkParents on open and close document (haskell/ghcide#12)

* Really disable expensive checkParents

* Add an option to check parents on close

Co-authored-by: Matthew Pickering <[email protected]>
Co-authored-by: Pepe Iborra <[email protected]>
Co-authored-by: Luke Lau <[email protected]>
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…encies of a file on saving (haskell/ghcide#688)

* Add new command to GetModuleGraph for a session and propate changes to
modules

Only propagate changes to parent modules when saving

Typecheck files when they are opened, don't TC FOI

Add known files rule

Don't save ifaces for files with defered errors

Co-authored-by: Zubin Duggal <[email protected]>

* Add configuration for parent typechecking

* hlint ignore

* Use targets to filter located imports (haskell/ghcide#10)

* Use targets to filter located imports

* Remove import paths from the GHC session

Otherwise GHC will prioritize source files found in the import path

* Update session-loader/Development/IDE/Session.hs

Co-authored-by: Pepe Iborra <[email protected]>

* Add session-loader to hie.yaml (haskell/ghcide#714)

* move known files rule to RuleTypes

* Disable checkParents on open and close document (haskell/ghcide#12)

* Really disable expensive checkParents

* Add an option to check parents on close

Co-authored-by: Matthew Pickering <[email protected]>
Co-authored-by: Pepe Iborra <[email protected]>
Co-authored-by: Luke Lau <[email protected]>
pepeiborra added a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020
…encies of a file on saving (haskell/ghcide#688)

* Add new command to GetModuleGraph for a session and propate changes to
modules

Only propagate changes to parent modules when saving

Typecheck files when they are opened, don't TC FOI

Add known files rule

Don't save ifaces for files with defered errors

Co-authored-by: Zubin Duggal <[email protected]>

* Add configuration for parent typechecking

* hlint ignore

* Use targets to filter located imports (haskell/ghcide#10)

* Use targets to filter located imports

* Remove import paths from the GHC session

Otherwise GHC will prioritize source files found in the import path

* Update session-loader/Development/IDE/Session.hs

Co-authored-by: Pepe Iborra <[email protected]>

* Add session-loader to hie.yaml (haskell/ghcide#714)

* move known files rule to RuleTypes

* Disable checkParents on open and close document (haskell/ghcide#12)

* Really disable expensive checkParents

* Add an option to check parents on close

Co-authored-by: Matthew Pickering <[email protected]>
Co-authored-by: Pepe Iborra <[email protected]>
Co-authored-by: Luke Lau <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants