Favor `lookupPathToId` over `pathToId` #926

samuela · 2020-11-28T02:45:29Z

Fixes #921.

Done: transitiveReverseDependencies and immediateReverseDependencies have been refactored to return Maybe [...].

I haven't changed reportImportCyclesRule or transitiveDeps as they seem to be acquitted in https://github.com/haskell/ghcide/issues/921#issue-749520646, although I can't say that I personally understand the logic there.

TODO: Predictably these types changing has stirred up type errors elsewhere. Although there are obvious workarounds, I'm not familiar with the desired behavior for these call sites to propose just converting Nothing -> [].

ghcide/src/Development/IDE/Core/FileStore.hs

Line 245 in 30a46e8

() <$ uses GetModIface revs

I imagine this should error out if we fail to get the reverse dependencies? If so, what's the appropriate way to error out of the Action monad?

ghcide/src/Development/IDE/Core/Rules.hs

Line 913 in f4bfe9c

    
           else anyM (fmap (fromMaybe False) . use NeedsCompilation) . maybe [] (immediateReverseDependencies file)

I imagine that needsCompilationRule should never fail? I don't understand what's going on here.

pepeiborra · 2020-11-28T16:04:10Z

The call site in FileStore.hs is being asked to perform typechecking of the "parents" of the module in the given path. A return value of Nothing in transitiveReverseDependencies would mean that this file is not in the module graph, which can happen if a ModSummary could not be parsed out of it. It would be ok to do nothing in this case.

The call site in Rules.hs should never return Nothing because only a few lines above there is a successful call to GetModSummaryWithoutTimestamps, and it would be ok to handle a Nothing with an error.

src/Development/IDE/Import/DependencyInformation.hs

samuela · 2020-11-28T23:21:53Z

Ok, @pepeiborra thanks for your feedback! I believe I've implemented your suggestions in b2675d3 and d95dd1f. I'll mark this as ready for review now.

pepeiborra

Very nice, thanks for improving the code readability.

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

* Move tracing functions to own module * Bump opentelemetry to 0.6.0 * Write Values map size to OpenTelemetry metric * Trace all requests and notifications Instead of doing it in `HoverDefinition`, do it in with{Response,Notification,...}. These wrap all handlers, so this should cover everything. It also means that the span covers the entire processing time for the request, where before we missed the setup happening in the with* functions. * Add flag for OpenTelemetry profiling Run GC regularly with --ot-profiling * Add flag to enable OT profiling in benchmark * Use heapsize instead of ghc-datasize I renamed the fork to distringuish from the original. It is still being pulled from git using stack. This will be addressed once I can push the fork to hackage. * Bump opentelemetry to 0.6.1 - fixes 8.6 build * Use heapsize from hackage * Address HLint messages * Record size of each key independently * Refactor `startTelemetry` function * Remove delay between measuring memory loops * Each key in values map gets own OT instrument * Measure values map length more rarely * Rename --ot-profiling to --ot-memory-profiling * Add docs for how to use the opentelemetry output * Add instructions to build release version of tracy * Clarify dependencies in opentelemetry instructions * Fix LSP traces * otTraced: delete unused * Extract types out of D.IDE.Core.Shake to avoid circular module dependencies * Extract startTelemetry out of D.IDE.Shake and upgrade to 0.2 No more segfaults * [nix] install opentelemetry * [nix] install tracy * Fix merge wibble * Measure recursive sizes with sharing * Sort keys for cost attribution * Remove debug traces * Allocate less, group keys, clean up hlints * Add -A4G to the flags used for --ot-memory-profiling * Modularize D.IDE.Core.Tracing I want to reuse this code more directly in the non lsp driver * Direct driver: report closure sizes when --ot-memory-profiling An eventlog memory analysis doesnt' seem so relevant since this mode is not interactive, but we could easily produce both if wanted to * Everything is reachable from GhcSessionIO, so compute it last I suspect the ShakeExtras record is reachable from GhcSessionIO * bound recursion and use logger * hlint suggestions * Fix 8.6 build * Format imports * Do the memory analysis with full sharing. GhcSessionIO last * Fail fast in the memory analysis * error handling * runHeapsize now takes initSize as an input argument * Trace Shake sessions * Reduced frequency for sampling values length * Drop the -fexternal-interpreter flag in the Windows stack build * Produce more benchmark artifacts * Fix stack descriptors to use heapsize-0.2 from Hackage * Bump to heapsize-0.3.0 * Record completions snippets (#900) * Add field for RecordSnippets to CachcedCompletion * Initial version of local record snippets * Supprt record snippet completion for non local declarations. * Better integration of local completions with current implementation * Clean up non-local completions. * Remove commented code. * Switch from String to Text * Remove ununsed definition * Treat only Records and leave other defintions as is. * Differentiate Records from Data constructors for external declaration * Update test to include snippet in local record completions expected list. * Update completionTest to also compare insertText. * Add test for record snippet completion for imported records. * Hlint fixes * Hlint fixes * Hlint suggestions. * Update type. * Consolidate imports * Unpack tuple with explicit names * Idiomatic changes * Remove unused variable * Better variable name * Hlint suggestions * Handle exhaustive pattern warning * Add _ to snippet field name suggestions * Remove type information passed around but not used * Update to list comprehension style * Eliminate intermediate function * HLint suggestions. * Idiomatic list comprehension Co-authored-by: Pepe Iborra <pepeiborra@me.com> * [nix] use gitignore.nix (#920) * Ignore import list while producing completions (#919) * Drop any items in explicit import list * Test if imports not included in explicit list show up in completions * Update README.md (#924) * Custom cradle loading (#928) When using ghcide as a library, it may be desirable to host the hie.yaml file in a location other than the project root, or even avoid the file system altogether * Favor `lookupPathToId` over `pathToId` (#926) * Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule` * Return completion snippets only when client supports it (#929) * Use the real client capabilities on completions * Return completion snippets only when supported by the client Restored from #900 * Redundant import * Fix stack windows build Co-authored-by: Michalis Pardalos <m.pardalos@gmail.com> Co-authored-by: Michalis Pardalos <mpardalos@gmail.com> Co-authored-by: Guru Devanla <gdevanla@users.noreply.github.com> Co-authored-by: Samuel Ainsworth <skainsworth@gmail.com>

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

* Move tracing functions to own module * Bump opentelemetry to 0.6.0 * Write Values map size to OpenTelemetry metric * Trace all requests and notifications Instead of doing it in `HoverDefinition`, do it in with{Response,Notification,...}. These wrap all handlers, so this should cover everything. It also means that the span covers the entire processing time for the request, where before we missed the setup happening in the with* functions. * Add flag for OpenTelemetry profiling Run GC regularly with --ot-profiling * Add flag to enable OT profiling in benchmark * Use heapsize instead of ghc-datasize I renamed the fork to distringuish from the original. It is still being pulled from git using stack. This will be addressed once I can push the fork to hackage. * Bump opentelemetry to 0.6.1 - fixes 8.6 build * Use heapsize from hackage * Address HLint messages * Record size of each key independently * Refactor `startTelemetry` function * Remove delay between measuring memory loops * Each key in values map gets own OT instrument * Measure values map length more rarely * Rename --ot-profiling to --ot-memory-profiling * Add docs for how to use the opentelemetry output * Add instructions to build release version of tracy * Clarify dependencies in opentelemetry instructions * Fix LSP traces * otTraced: delete unused * Extract types out of D.IDE.Core.Shake to avoid circular module dependencies * Extract startTelemetry out of D.IDE.Shake and upgrade to 0.2 No more segfaults * [nix] install opentelemetry * [nix] install tracy * Fix merge wibble * Measure recursive sizes with sharing * Sort keys for cost attribution * Remove debug traces * Allocate less, group keys, clean up hlints * Add -A4G to the flags used for --ot-memory-profiling * Modularize D.IDE.Core.Tracing I want to reuse this code more directly in the non lsp driver * Direct driver: report closure sizes when --ot-memory-profiling An eventlog memory analysis doesnt' seem so relevant since this mode is not interactive, but we could easily produce both if wanted to * Everything is reachable from GhcSessionIO, so compute it last I suspect the ShakeExtras record is reachable from GhcSessionIO * bound recursion and use logger * hlint suggestions * Fix 8.6 build * Format imports * Do the memory analysis with full sharing. GhcSessionIO last * Fail fast in the memory analysis * error handling * runHeapsize now takes initSize as an input argument * Trace Shake sessions * Reduced frequency for sampling values length * Drop the -fexternal-interpreter flag in the Windows stack build * Produce more benchmark artifacts * Fix stack descriptors to use heapsize-0.2 from Hackage * Bump to heapsize-0.3.0 * Record completions snippets (haskell/ghcide#900) * Add field for RecordSnippets to CachcedCompletion * Initial version of local record snippets * Supprt record snippet completion for non local declarations. * Better integration of local completions with current implementation * Clean up non-local completions. * Remove commented code. * Switch from String to Text * Remove ununsed definition * Treat only Records and leave other defintions as is. * Differentiate Records from Data constructors for external declaration * Update test to include snippet in local record completions expected list. * Update completionTest to also compare insertText. * Add test for record snippet completion for imported records. * Hlint fixes * Hlint fixes * Hlint suggestions. * Update type. * Consolidate imports * Unpack tuple with explicit names * Idiomatic changes * Remove unused variable * Better variable name * Hlint suggestions * Handle exhaustive pattern warning * Add _ to snippet field name suggestions * Remove type information passed around but not used * Update to list comprehension style * Eliminate intermediate function * HLint suggestions. * Idiomatic list comprehension Co-authored-by: Pepe Iborra <pepeiborra@me.com> * [nix] use gitignore.nix (haskell/ghcide#920) * Ignore import list while producing completions (haskell/ghcide#919) * Drop any items in explicit import list * Test if imports not included in explicit list show up in completions * Update README.md (haskell/ghcide#924) * Custom cradle loading (haskell/ghcide#928) When using ghcide as a library, it may be desirable to host the hie.yaml file in a location other than the project root, or even avoid the file system altogether * Favor `lookupPathToId` over `pathToId` (haskell/ghcide#926) * Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule` * Return completion snippets only when client supports it (haskell/ghcide#929) * Use the real client capabilities on completions * Return completion snippets only when supported by the client Restored from haskell/ghcide#900 * Redundant import * Fix stack windows build Co-authored-by: Michalis Pardalos <m.pardalos@gmail.com> Co-authored-by: Michalis Pardalos <mpardalos@gmail.com> Co-authored-by: Guru Devanla <gdevanla@users.noreply.github.com> Co-authored-by: Samuel Ainsworth <skainsworth@gmail.com>

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

* Move tracing functions to own module * Bump opentelemetry to 0.6.0 * Write Values map size to OpenTelemetry metric * Trace all requests and notifications Instead of doing it in `HoverDefinition`, do it in with{Response,Notification,...}. These wrap all handlers, so this should cover everything. It also means that the span covers the entire processing time for the request, where before we missed the setup happening in the with* functions. * Add flag for OpenTelemetry profiling Run GC regularly with --ot-profiling * Add flag to enable OT profiling in benchmark * Use heapsize instead of ghc-datasize I renamed the fork to distringuish from the original. It is still being pulled from git using stack. This will be addressed once I can push the fork to hackage. * Bump opentelemetry to 0.6.1 - fixes 8.6 build * Use heapsize from hackage * Address HLint messages * Record size of each key independently * Refactor `startTelemetry` function * Remove delay between measuring memory loops * Each key in values map gets own OT instrument * Measure values map length more rarely * Rename --ot-profiling to --ot-memory-profiling * Add docs for how to use the opentelemetry output * Add instructions to build release version of tracy * Clarify dependencies in opentelemetry instructions * Fix LSP traces * otTraced: delete unused * Extract types out of D.IDE.Core.Shake to avoid circular module dependencies * Extract startTelemetry out of D.IDE.Shake and upgrade to 0.2 No more segfaults * [nix] install opentelemetry * [nix] install tracy * Fix merge wibble * Measure recursive sizes with sharing * Sort keys for cost attribution * Remove debug traces * Allocate less, group keys, clean up hlints * Add -A4G to the flags used for --ot-memory-profiling * Modularize D.IDE.Core.Tracing I want to reuse this code more directly in the non lsp driver * Direct driver: report closure sizes when --ot-memory-profiling An eventlog memory analysis doesnt' seem so relevant since this mode is not interactive, but we could easily produce both if wanted to * Everything is reachable from GhcSessionIO, so compute it last I suspect the ShakeExtras record is reachable from GhcSessionIO * bound recursion and use logger * hlint suggestions * Fix 8.6 build * Format imports * Do the memory analysis with full sharing. GhcSessionIO last * Fail fast in the memory analysis * error handling * runHeapsize now takes initSize as an input argument * Trace Shake sessions * Reduced frequency for sampling values length * Drop the -fexternal-interpreter flag in the Windows stack build * Produce more benchmark artifacts * Fix stack descriptors to use heapsize-0.2 from Hackage * Bump to heapsize-0.3.0 * Record completions snippets (haskell/ghcide#900) * Add field for RecordSnippets to CachcedCompletion * Initial version of local record snippets * Supprt record snippet completion for non local declarations. * Better integration of local completions with current implementation * Clean up non-local completions. * Remove commented code. * Switch from String to Text * Remove ununsed definition * Treat only Records and leave other defintions as is. * Differentiate Records from Data constructors for external declaration * Update test to include snippet in local record completions expected list. * Update completionTest to also compare insertText. * Add test for record snippet completion for imported records. * Hlint fixes * Hlint fixes * Hlint suggestions. * Update type. * Consolidate imports * Unpack tuple with explicit names * Idiomatic changes * Remove unused variable * Better variable name * Hlint suggestions * Handle exhaustive pattern warning * Add _ to snippet field name suggestions * Remove type information passed around but not used * Update to list comprehension style * Eliminate intermediate function * HLint suggestions. * Idiomatic list comprehension Co-authored-by: Pepe Iborra <pepeiborra@me.com> * [nix] use gitignore.nix (haskell/ghcide#920) * Ignore import list while producing completions (haskell/ghcide#919) * Drop any items in explicit import list * Test if imports not included in explicit list show up in completions * Update README.md (haskell/ghcide#924) * Custom cradle loading (haskell/ghcide#928) When using ghcide as a library, it may be desirable to host the hie.yaml file in a location other than the project root, or even avoid the file system altogether * Favor `lookupPathToId` over `pathToId` (haskell/ghcide#926) * Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule` * Return completion snippets only when client supports it (haskell/ghcide#929) * Use the real client capabilities on completions * Return completion snippets only when supported by the client Restored from haskell/ghcide#900 * Redundant import * Fix stack windows build Co-authored-by: Michalis Pardalos <m.pardalos@gmail.com> Co-authored-by: Michalis Pardalos <mpardalos@gmail.com> Co-authored-by: Guru Devanla <gdevanla@users.noreply.github.com> Co-authored-by: Samuel Ainsworth <skainsworth@gmail.com>

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

* Move tracing functions to own module * Bump opentelemetry to 0.6.0 * Write Values map size to OpenTelemetry metric * Trace all requests and notifications Instead of doing it in `HoverDefinition`, do it in with{Response,Notification,...}. These wrap all handlers, so this should cover everything. It also means that the span covers the entire processing time for the request, where before we missed the setup happening in the with* functions. * Add flag for OpenTelemetry profiling Run GC regularly with --ot-profiling * Add flag to enable OT profiling in benchmark * Use heapsize instead of ghc-datasize I renamed the fork to distringuish from the original. It is still being pulled from git using stack. This will be addressed once I can push the fork to hackage. * Bump opentelemetry to 0.6.1 - fixes 8.6 build * Use heapsize from hackage * Address HLint messages * Record size of each key independently * Refactor `startTelemetry` function * Remove delay between measuring memory loops * Each key in values map gets own OT instrument * Measure values map length more rarely * Rename --ot-profiling to --ot-memory-profiling * Add docs for how to use the opentelemetry output * Add instructions to build release version of tracy * Clarify dependencies in opentelemetry instructions * Fix LSP traces * otTraced: delete unused * Extract types out of D.IDE.Core.Shake to avoid circular module dependencies * Extract startTelemetry out of D.IDE.Shake and upgrade to 0.2 No more segfaults * [nix] install opentelemetry * [nix] install tracy * Fix merge wibble * Measure recursive sizes with sharing * Sort keys for cost attribution * Remove debug traces * Allocate less, group keys, clean up hlints * Add -A4G to the flags used for --ot-memory-profiling * Modularize D.IDE.Core.Tracing I want to reuse this code more directly in the non lsp driver * Direct driver: report closure sizes when --ot-memory-profiling An eventlog memory analysis doesnt' seem so relevant since this mode is not interactive, but we could easily produce both if wanted to * Everything is reachable from GhcSessionIO, so compute it last I suspect the ShakeExtras record is reachable from GhcSessionIO * bound recursion and use logger * hlint suggestions * Fix 8.6 build * Format imports * Do the memory analysis with full sharing. GhcSessionIO last * Fail fast in the memory analysis * error handling * runHeapsize now takes initSize as an input argument * Trace Shake sessions * Reduced frequency for sampling values length * Drop the -fexternal-interpreter flag in the Windows stack build * Produce more benchmark artifacts * Fix stack descriptors to use heapsize-0.2 from Hackage * Bump to heapsize-0.3.0 * Record completions snippets (haskell/ghcide#900) * Add field for RecordSnippets to CachcedCompletion * Initial version of local record snippets * Supprt record snippet completion for non local declarations. * Better integration of local completions with current implementation * Clean up non-local completions. * Remove commented code. * Switch from String to Text * Remove ununsed definition * Treat only Records and leave other defintions as is. * Differentiate Records from Data constructors for external declaration * Update test to include snippet in local record completions expected list. * Update completionTest to also compare insertText. * Add test for record snippet completion for imported records. * Hlint fixes * Hlint fixes * Hlint suggestions. * Update type. * Consolidate imports * Unpack tuple with explicit names * Idiomatic changes * Remove unused variable * Better variable name * Hlint suggestions * Handle exhaustive pattern warning * Add _ to snippet field name suggestions * Remove type information passed around but not used * Update to list comprehension style * Eliminate intermediate function * HLint suggestions. * Idiomatic list comprehension Co-authored-by: Pepe Iborra <pepeiborra@me.com> * [nix] use gitignore.nix (haskell/ghcide#920) * Ignore import list while producing completions (haskell/ghcide#919) * Drop any items in explicit import list * Test if imports not included in explicit list show up in completions * Update README.md (haskell/ghcide#924) * Custom cradle loading (haskell/ghcide#928) When using ghcide as a library, it may be desirable to host the hie.yaml file in a location other than the project root, or even avoid the file system altogether * Favor `lookupPathToId` over `pathToId` (haskell/ghcide#926) * Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule` * Return completion snippets only when client supports it (haskell/ghcide#929) * Use the real client capabilities on completions * Return completion snippets only when supported by the client Restored from haskell/ghcide#900 * Redundant import * Fix stack windows build Co-authored-by: Michalis Pardalos <m.pardalos@gmail.com> Co-authored-by: Michalis Pardalos <mpardalos@gmail.com> Co-authored-by: Guru Devanla <gdevanla@users.noreply.github.com> Co-authored-by: Samuel Ainsworth <skainsworth@gmail.com>

Favor lookupPathToId over pathToId

7a0f599

wz1000 reviewed Nov 28, 2020

View reviewed changes

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

samuela added 2 commits November 28, 2020 15:18

Fix typecheckParentsAction

b2675d3

Fix needsCompilationRule

d95dd1f

samuela marked this pull request as ready for review November 28, 2020 23:21

pepeiborra approved these changes Nov 28, 2020

View reviewed changes

pepeiborra merged commit 54f17d1 into haskell:master Nov 29, 2020

pepeiborra pushed a commit to pepeiborra/ghcide that referenced this pull request Nov 29, 2020

Favor lookupPathToId over pathToId (haskell#926)

57d2a9b

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020

Favor lookupPathToId over pathToId (haskell/ghcide#926)

aed7830

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020

Favor lookupPathToId over pathToId (haskell/ghcide#926)

db793d6

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

pepeiborra pushed a commit to pepeiborra/ide that referenced this pull request Dec 29, 2020

Favor lookupPathToId over pathToId (haskell/ghcide#926)

5905aa8

* Favor `lookupPathToId` over `pathToId` * Fix `typecheckParentsAction` * Fix `needsCompilationRule`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Favor `lookupPathToId` over `pathToId` #926

Favor `lookupPathToId` over `pathToId` #926

samuela commented Nov 28, 2020 •

edited

Loading

pepeiborra commented Nov 28, 2020

samuela commented Nov 28, 2020

pepeiborra left a comment

Favor lookupPathToId over pathToId #926

Favor lookupPathToId over pathToId #926

Conversation

samuela commented Nov 28, 2020 • edited Loading

pepeiborra commented Nov 28, 2020

samuela commented Nov 28, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

Favor `lookupPathToId` over `pathToId` #926

Favor `lookupPathToId` over `pathToId` #926

samuela commented Nov 28, 2020 •

edited

Loading