Insert pragmas after shebang or to existing pragma list

[OliverMadine]

Fix: #1726

Pragmas are now inserted before comments.

Updated the behaviour to insert directly after existing pragmas, else directly after shebangs, else at the top of the file.

Refactored to use the file contents rather than a parsed module so that missing pragmas, which cause parse errors, are still inserted into the correct position.

[jneira]

I commented too late 😟 : #1726 (comment)
Not sure if your final solution is affected by though

I will add test cases but in either case

Tests are definitely needed, to ensure there is no regression and to see examples about how it works

[OliverMadine]

I commented too late 😟 : #1726 (comment)
Not sure if your final solution is affected by though

I will add test cases but in either case

Tests are definitely needed, to ensure there is no regression and to see examples about how it works

If we were to enable the option to insert at the top of the file, I suppose we would just have #555 again so perhaps I should just close this PR.
Also since we are working with a ParsedModule, it would be difficult to get the span of existing pragma blocks?
But then I'm not sure how else we could solve this issue?

I suspected this may be the case, hence I waited before writing the tests 😔

[berberman]

I would say adding an option is definitely not ideal. Perhaps we could fix the real issue by finding the first non-empty line after a possible shebang, using file contents rather than parsed module.

[jneira]

Also since we are working with a ParsedModule, it would be difficult to get the span of existing pragma blocks?

No an expert in the ghc parse ast but i think they would be present as annotations. We have to use the version of the parsed module that includes such annotations (and no the default one)

@emilypi solution could be a little bit more adhoc but could work too. In fact i think #1340 tried to do it but was superseded by #1417 which established the actual behaviour.

//cc @berberman

[jneira]

#1340 added a regression test over the shebang case:

haskell-language-server/test/functional/FunctionalCodeAction.hs

Lines 558 to 588 in c117657

	, testCase "After Shebang" $ do
	runSession hlsCommand fullCaps "test/testdata/addPragmas" $ do
	doc <- openDoc "AfterShebang.hs" "haskell"
	_ <- waitForDiagnosticsFrom doc
	cas <- map fromAction <$> getAllCodeActions doc
	liftIO $ "Add \"NamedFieldPuns\"" `elem` map (^. L.title) cas @? "Contains NamedFieldPuns code action"
	executeCodeAction $ head cas
	contents <- documentContents doc
	let expected =
	[ "#! /usr/bin/env nix-shell"
	, "#! nix-shell --pure -i runghc -p \"haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])\""
	, ""
	, "{-# LANGUAGE NamedFieldPuns #-}"
	, "module AfterShebang where"
	, ""
	, "data Record = Record"
	, " { a :: Int,"
	, " b :: Double,"
	, " c :: String"
	, " }"
	, ""
	, "f Record{a, b} = a"
	]
	liftIO $ T.lines contents @?= expected
	]

And this pr didnt break it (and should afaiu), i suppose the option is disabled by defaultThe option is true by default so not sure what is going on

[michaelpj]

Perhaps I missed some discussion, but the issue was that we were inserting pragmas after module haddock, right? So could we just extend the property to be "insert just before the first declaration, import, module header, or haddock comment"?

[OliverMadine]

Perhaps I missed some discussion, but the issue was that we were inserting pragmas after module haddock, right? So could we just extend the property to be "insert just before the first declaration, import, module header, or haddock comment"?

Is it necessary to insert before haddock comments or could we just insert before any comments? I've just pushed a commit to do the latter but I can change it to specifically haddock comments if necessary.

[jneira]

before any comments sound good to me too 🤔

[berberman]

Looks great, thanks!

[michaelpj]

Let me be annoying, what about this?

{-# LANGUAGE ... -#}
-- this is here for reason X
{-# LANGUAGE ... -#}

I think it's pretty common for people to put comments inside their pragma blocks, which will result in slightly weird behaviour. But nobody puts haddock comments in their pragma blocks!

[pepeiborra]

Can we also consider inserting below copyright comments? E.g.

-- Copyright (c) 2019 The DAML Authors. All rights reserved.
-- SPDX-License-Identifier: Apache-2.0
{-# OPTIONS_GHC -Wno-dodgy-imports #-} -- GHC no longer exports def in GHC 8.6 and above
{-# LANGUAGE TemplateHaskell #-}

[pepeiborra]

Personally, I think the property should be as below, even if this doesn't deal properly with copyright headers when there is no existing pragmas:

Append at the end of the existing list of pragmas, if any, otherwise insert just before the first declaration, import, module header, or haddock comment

[OliverMadine]

Let me be annoying, what about this?

{-# LANGUAGE ... -#}
-- this is here for reason X
{-# LANGUAGE ... -#}

I think it's pretty common for people to put comments inside their pragma blocks, which will result in slightly weird behaviour. But nobody puts haddock comments in their pragma blocks!

Thanks, I hadn't thought about this.

Personally, I think the property should be as below, even if this doesn't deal properly with copyright headers when there is no existing pragmas:

Append at the end of the existing list of pragmas, if any, otherwise insert just before the first declaration, import, module header, or haddock comment

This should now be the behaviour of the code.

[berberman]

Append at the end of the existing list of pragmas, if any, otherwise insert just before the first declaration, import, module header, or haddock comment

Can we reach this by merely looking at file contents instead of parsed module? If there is an existing pragma, we insert the new one below it; otherwise we insert the new one after shebang meanwhile before the first non-empty line. Not sure if this will work or I missed something, but I believe getting rid of parsed module would avoid mysterious CPP macro in test.

[OliverMadine]

Append at the end of the existing list of pragmas, if any, otherwise insert just before the first declaration, import, module header, or haddock comment

Can we reach this by merely looking at file contents instead of parsed module? If there is an existing pragma, we insert the new one below it; otherwise we insert the new one after shebang meanwhile before the first non-empty line. Not sure if this will work or I missed something, but I believe getting rid of parsed module would avoid mysterious CPP macro in test.

I can't see any immediate reason why using file contents wouldn't work. What would be the performance difference with this solution? I might be able to refactor it to take this approach at some point, but I have exams at the moment 😔. In the meantime, since we don't guarantee any order of inserting pragmas, are there any practical issues with the current solution?

sorry for the late response

[berberman]

are there any practical issues with the current solution?

No, I think it's good enough :)

I suggest not using parsed module because missing language extensions like TypeApplications will result in a parse error, rather than a typecheck error - in such case our strategy will no loger work, and the pragma will be inserted to the top of file.

[michaelpj]

Ugh, that is a bit annoying. Maybe we should add a paragraph to the known limitations section about that, since I bet people will hit it (I think inserting TypeApplications is one of my top uses!).

But yeah, I don't think fixing it should block this PR.

[OliverMadine]

I suggest not using parsed module

Ugh, that is a bit annoying. Maybe we should add a paragraph to the known limitations section about that, since I bet people will hit it (I think inserting TypeApplications is one of my top uses!).

I decided it'd be best to just fix this issue now so I've pushed a new commit which hopefully should.

[berberman]

Thanks again!