Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Write hls unit tests for get references #1

Merged
merged 11 commits into from
Jan 4, 2021
Merged

Write hls unit tests for get references #1

merged 11 commits into from
Jan 4, 2021

Conversation

peterwicksstringfield
Copy link

wz1000 and others added 4 commits December 29, 2020 19:11
@wz1000
References Use db for findDef
14b206b 
save source file location to db
Find source for boot files

Use DynFlags from HieDb instead of unsafeGlobalDynFlags

Return multiple definitions

don't typecheck files on load

Add support for persistent stale values
Add persistent hie file rule

docs wip

better typedef

defs for deps

update hiedb

Fix for files with errors

Fix build

integrate hiedb commands and set dynflags on boot

workspace symbol tweaks, cabal.project

Write ifaces on save

use real mtime for saved files

safe indexing

bump hiedb

Proper refs for FOIs

hlint

Update exe/Main.hs

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

Review comments

update hiedb

Update src/Development/IDE/Core/Shake.hs

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

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

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

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

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

Update src/Development/IDE/Spans/AtPoint.hs

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

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

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

Apply suggestions from code review

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

more careful re-indexing

update for hiedb-0.1.0.0

Remove cached-deps stuff for now

explicit showSDoc

docs in AtPoint

add doc comment about database consistency

add TODO for better position mapping from diff
@peterwicksstringfield
Make HLS compile with (GHC 8.10.2) and ghcide+hiedb.
f9a4ec7 
Hacks to tactics plugic, class plugin, cabal file, and main.
@peterwicksstringfield
HLS unit tests for the find references functionality.
d441cbe
@peterwicksstringfield
Mark failing test.
cc8a582
@peterwicksstringfield peterwicksstringfield mentioned this pull request Jan 1, 2021
Merged
2 tasks
@peterwicksstringfield
Hlint fromRight hint.
b31e69a
@peterwicksstringfield
Hlint nubOrd. Update ignore annotation to point to correct function.
fd9faec 
This block of code that uses nub used to be part of main. But it got split off
into a new function called runIde, and the hlint annotation was not updated to
match.
@peterwicksstringfield
Update tests to document that we now have references and workspace sy…
fc761a6 
…mbol providers.
@peterwicksstringfield
Fix line number in broken test.
604b746 
We have this code:
20: ddd :: Num a => a -> a -> a
21: ddd vv ww = vv +! ww

The intention was to ask for the type definition of symbol "a" in line 20, and
then assert that no type definitions were found.

The reality is that the test was asking for the definition of the symbol at (20,
15) in 0-based indexing, which is the "!" in "+!". Until recently, ghcide could
not find type definitions for "+!", so no type definitions were
found and the test passed. But now, ghcide can find type definitions for "+!",
and this test has begun to fail.

The solution is to change (20, 15) to (19, 15), so that we ask for the type
definitions of the symbol "a", which will not be found.
@peterwicksstringfield peterwicksstringfield force-pushed the write_hls_unit_tests_for_get_references branch from a7d52c5 to 604b746 Compare January 1, 2021 19:38
@wz1000
Copy link
Owner

wz1000 commented Jan 3, 2021

Very nice, this looks quite helpful.

I think we will need a few test messages to indicate that indexing has completed and synchronize to avoid race condition style failures in tests(i.e. we ask for references too early and get an incomplete result), so this is what I have been working on. This might be the cause of some of the failures you are seeing.

Sorry for the late response, github didn't notify me for either this PR or the comment on the draft HLS PR for some reason. It's very late tonight, so I will take a proper look tomorrow.

@peterwicksstringfield
Copy link
Author

peterwicksstringfield commented Jan 3, 2021 •
edited
Loading

I figured you were busy, it's not a problem. :) Dunno why it didn't notify you.

The nix builds are failing with something dependency related:

building '/nix/store/bnb2xhjh7dz1pgsld2rd3l60mxx8k8h2-cabal2nix-haskell-language-server.drv'...
installing
error: anonymous function at /nix/store/lh6n286kd27aczmqi6jcik37v0ssc91n-cabal2nix-haskell-language-server/default.nix:1:1 called without required argument 'hiedb', at /nix/store/51gm3gjl7i2n3f7pamrbsckm4h0fmhy2-nixpkgs-src/pkgs/development/haskell-modules/make-package-set.nix:87:27
(use '--show-trace' to show detailed location information)
Error: Process completed with exit code 1.

And the pre 8.10 builds are failing like this:

src/Development/IDE/Spans/AtPoint.hs:260:13: error:
    • The constructor ‘FunTy’ should have 2 arguments, but has been given 3
    • In the pattern: FunTy _ a b
      In a case alternative: FunTy _ a b -> getTypes [a, b]
      In the second argument of ‘($)’, namely
        ‘\case
           TyVarTy n -> [Var.varName n]
           AppTy a b -> getTypes [a, b]
           TyConApp tc ts -> tyConName tc : getTypes ts
           ForAllTy _ t -> getTypes [t]
           FunTy _ a b -> getTypes [a, b]
           CastTy t _ -> getTypes [t]
           _ -> []’
    |
260 |             FunTy _ a b -> getTypes [a,b]
    |             ^^^^^^^^^^^
cabal: Failed to build ghcide-0.6.0.2 (which is required by bench:benchHist
from ghcide-0.6.0.2).

So it's not race condition related, the build is simply broken.

Note the review comment below though.

peterwicksstringfield
peterwicksstringfield commented Jan 3, 2021
View reviewed changes
test/functional/Reference.hs
_ <- openDoc "dependencyfoo/src/ModuleInDependency.hs" "haskell"
_ <- openDoc "dependencyfoo/src/OtherModuleInDependency.hs" "haskell"
liftIO $ sleep 2
f
Copy link
Author

Choose a reason for hiding this comment

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

This is a kludge to make sure references are ready before we ask for them.

@peterwicksstringfield
Update testcase.
e75a69e 
Getting a type definition can produce more than one result. E.g. the type of the
symbol "pid" in:

1: data Parameter a = Parameter a
2: f :: forall a. Parameter a -> Parameter a
3: f pid = pid

is (Parameter a), and the definition of this type is two part: the definition of
Parameter on line 1, and the definition of a on line 2.
@peterwicksstringfield
Copy link
Author

8.8.3
https://hackage.haskell.org/package/ghc-8.8.3/docs/src/TyCoRep.html#Type

| FunTy Type Type     -- ^ t1 -> t2   Very common, so an important special case

vs

8.10.2
https://hackage.haskell.org/package/ghc-8.10.2/docs/src/TyCoRep.html#Type

  | FunTy      -- ^ t1 -> t2   Very common, so an important special case
                -- See Note [Function types]
     { ft_af  :: AnonArgFlag  -- Is this (->) or (=>)?
     , ft_arg :: Type           -- Argument type
     , ft_res :: Type }         -- Result type

@peterwicksstringfield
Kludge compilation for <=8.8.3.
5e5d637 
FunTy takes a third argument in 8.10.1 and on.
@peterwicksstringfield peterwicksstringfield force-pushed the write_hls_unit_tests_for_get_references branch from 2654bae to 5e5d637 Compare January 4, 2021 00:48
@wz1000 wz1000 merged commit 50b3b5f into wz1000:hiedb Jan 4, 2021
@peterwicksstringfield peterwicksstringfield deleted the write_hls_unit_tests_for_get_references branch January 10, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peterwicksstringfield @wz1000