Opened 12 years ago

Closed 10 months ago

#95 closed feature request (fixed)

GHCi :edit command should jump to the the last error

Reported by: martijnislief Owned by: lortabac
Priority: normal Milestone:
Component: GHCi Version: None
Keywords: Cc: hvr
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description (last modified by simonmar)

GHCi has a :edit command for editing the current source file. Hugs's command of the same name automatically jumps the editor to the location of the last error; it would be nice if GHCi's did the same.

Attachments (4)

le.ghci (1.3 KB) - added by claus 8 years ago.
example .ghci script implementing :le (load/reload and edit)
0001-Bug-95-edit-command-should-jump-to-the-last-error.patch (4.9 KB) - added by lortabac 11 months ago.
0001-Fixes-95-edit-command-should-jump-to-the-last-error.patch (7.6 KB) - added by lortabac 10 months ago.
0001-Fixes-95-edit-command-should-jump-to-the-last-error.2.patch (10.5 KB) - added by lortabac 10 months ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 9 years ago by simonmar

  • Architecture set to Unknown
  • Description modified (diff)
  • difficulty set to Easy (1 hr)
  • Operating System set to Unknown

I implemented :edit, but without the line-number-jumping facility for now.

comment:2 Changed 9 years ago by igloo

  • Component changed from None to GHCi
  • Milestone set to 6.6.1

comment:3 Changed 8 years ago by simonmar

  • Priority changed from normal to low

comment:4 Changed 8 years ago by igloo

  • Milestone changed from 6.6.1 to 6.6.2
  • Priority changed from low to normal

Punting on this.

The only problem with fixing it is that we've forgotten where the first error was by the time :e is given; shouldn't be too hard to fix, though, by updating a mutable variable when appropriate.

comment:5 Changed 8 years ago by simonmar

  • Milestone changed from 6.6.2 to 6.8

comment:6 Changed 8 years ago by claus

starting somewhere in 6.7, you can define something like that yourself. the trick is to have a :redirErr command that allows you to capture ghci command error output. then you do a :l or :r, capture the errors, and use :e to edit the first one.

a slight complication is that :l/:r invalidate all variable bindings, so we need to define a temporary :afterCmd, in order to restore stderr and to read the errors after the :l/:r.

please note that many editors support quick compile/edit cycles (in vim, try :help quickfix; emacs should also support something similar), which will do this kind of thing more easily in most cases.

add the attached le.ghci to your .ghci, or source it with :cmd readFile "le.ghci". then, :le <mod> will :load and :edit, and :le on its own will :reload and :edit. adapt the code to your needs.

there is a patch pending for ghc head that will allow us to avoid having to compress ghci definitions into single lines, which will make things like this a lot more readable and maintainable..

Changed 8 years ago by claus

example .ghci script implementing :le (load/reload and edit)

comment:7 Changed 8 years ago by guest

actually, it turns out you can define all this even back in 6.4.1 (had i only known back then!-). you need to define your own :e command (:def is available), and ghci 6.4.1 is somewhat annoying about command names that are prefixes of each other. have a look at

http://www.cs.kent.ac.uk/people/staff/cr3/toolbox/haskell/dot-squashed.ghci641

http://www.cs.kent.ac.uk/people/staff/cr3/toolbox/haskell/dot-squashed.ghci

since .ghci files are not very readable with definitions squashed into single lines, you might want to check the explanations of the latter file in

http://www.haskell.org/pipermail/haskell-cafe/2007-September/032260.html

(note: the names in the 6.4.1 version differ from those in the described version)

i'm not yet sure whether being able to define :e means that it shouldn't be part of ghci at all (and this ticket be closed), or whether there are still any advantages to having it built in? perhaps ghci releases should have a selection of .ghci files with useful definitions?

comment:8 Changed 7 years ago by simonmar

  • Owner nobody deleted
  • Status changed from assigned to new

comment:9 Changed 7 years ago by igloo

  • Milestone changed from 6.8 branch to 6.10 branch

comment:10 Changed 7 years ago by simonmar

  • Architecture changed from Unknown to Unknown/Multiple

comment:11 Changed 7 years ago by simonmar

  • Operating System changed from Unknown to Unknown/Multiple

comment:12 Changed 6 years ago by igloo

  • Milestone changed from 6.10 branch to 6.12 branch

comment:13 Changed 5 years ago by simonmar

  • difficulty changed from Easy (1 hr) to Easy (less than 1 hour)

comment:14 Changed 5 years ago by simonmar

  • Description modified (diff)
  • Milestone changed from 6.12 branch to _|_
  • Summary changed from GHCi editor binding with ":e" to GHCi :edit command should jump to the the last error
  • Type of failure set to None/Unknown

You can define it yourself, but it's a bit hacky and it would clearly be better to have it built-in. This is also a small self-contained task (limited to the GHCi front end) that a new contributor could take on.

comment:15 Changed 11 months ago by lortabac

  • Cc hvr added
  • Status changed from new to patch

comment:16 Changed 10 months ago by nomeata

Hi lortabac. Thanks for your patch.

One thing that is definitely missing is documentation: You should explain this behaviour in the user manual for :edit.

I would name the field errors a bit more self-descriptive. Maybe lastErrorLocations?

Why do you need unsafeCoerce? Can’t you create the IORef locally when initializing GHCiState, in interactiveUI, and also pass it as a first parameter to your ghciLogAction?

Your filter could be more readable a find (from Data.List).

That’s it for now :-)

comment:17 Changed 10 months ago by nomeata

  • Owner set to lortabac

comment:18 Changed 10 months ago by lortabac

I followed your suggestions and I also fixed a small mistake.

comment:19 Changed 10 months ago by nomeata

Better! So time for the next round of suggestions.

I found this code confusing

         let ...
               setGhciErrors e = writeIORef lastErrLocations (errs ++ e) 
          setGhciErrors $ errLocation srcSpan

why not

         let ...
         writeIORef lastErrLocations (errs ++ errLocation srcSpan) 

In fact I find the whole section there a bit strange. Some suggestions:

  • Use modifyIORef
  • Write something like
    case srcSpan of
      RealSrcSpan rsp -> modifyIORef lastErrLocations (++ [srcLocFile (realSrcSpanStart x) , srcLocLine (realSrcSpanStart x) ])
      _ -> return ()
    
    so you don’t have to introduce so many local functions and do tricks with ++ [].
  • find (\(f, _) -> f == mkFastString file) errs can even simplified to lookup (mkFastSTring file)

I would put

   ghciState <- lift getGHCiState 
   liftIO $ writeIORef (lastErrorLocations ghciState) [] 

in a function resetLastErrorLocations :: GHCi (). Naming that code also serves as documentation, and you don’t clutter the local name space of doLoad with the ghciState

I wonder if things work well if the user uses a different path to the filename than GHC; maybe one wants to use a function that can check if two filenames point to the same file. But maybe that is a refinement that can be added later.

Can you come up with a way to test this? Maybe by setting the editor to use to /bin/echo, which will put +123 in the output that the test suite would check? See [Building/RunningTests/Adding] and shout if you need help creating a test case.

comment:20 Changed 10 months ago by lortabac

I added a sameFile function to check whether two paths point to the same file.
However I can't find any readable alternative to:

    curFileErrs <- filterM (\(f, _) -> unpackFS f `sameFile` file) errs
    return $ case curFileErrs of
        (_, line):_ -> " +" ++ show line
        _ -> ""

As far as the test is concerned, I just followed the enumeration in the ghci directory, so I called it prog013. It would be nice to make it a bit more self-descriptive but I have no idea how.

comment:21 Changed 10 months ago by nomeata

Don’t worry about test names, prog013 is fine; T95 would have been another possibility. Ideally, the test case is never looked at again, because nobody breaks this feature.

The patch is, IMHO, good to go. I’m validating this right now and will push if validation succeeds.

comment:22 Changed 10 months ago by Joachim Breitner <mail@…>

In ce19d5079ea85d3190e837a1fc60000fbd82134d/ghc:

Fixes #95 :edit command should jump to the last error

comment:23 Changed 10 months ago by nomeata

  • Resolution changed from None to fixed
  • Status changed from patch to closed

Thanks, Lorenzo, for your first contribution to GHC! I hope more will follow :-)

Note: See TracTickets for help on using tickets.