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.

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

Add pragma after she-bang #1340

Merged
merged 2 commits into from
Feb 24, 2021

Conversation

ishmum123
Copy link
Contributor

Draft PR for #555

plugins/default/src/Ide/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
plugins/default/src/Ide/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
plugins/default/src/Ide/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
plugins/default/src/Ide/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
plugins/default/src/Ide/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
@ishmum123 ishmum123 marked this pull request as draft February 10, 2021 11:07
@ishmum123
Copy link
Contributor Author

Hi @Ailrun, sorry for the late reply. I have been trying to get parsedModule required for the endOfModuleHeader but I keep running into this-

Exception: BadDependency "GetParsedModuleWithComments"

Any Ideas?

@Ailrun
Copy link
Member

Ailrun commented Feb 13, 2021

What do you mean by "trying to get"? The code to retrieve ParsedModule was already there, right?

@ishmum123
Copy link
Contributor Author

ishmum123 commented Feb 13, 2021

Yes, the value of pm is Nothing from the printed output I got

@Ailrun
Copy link
Member

Ailrun commented Feb 13, 2021

You mean even before your code? Your code does not change anything related with pm loading, so I don't get what is the problem....

@ishmum123
Copy link
Contributor Author

ishmum123 commented Feb 13, 2021

As far as I could check that happened before my code as well. Also, I couldn't figure out what problems a Nothing module would have seems like -

genPragma mDynflags target = [ r | r <- findPragma target, rnotElemdisabled] where disabled | Just dynFlags <- mDynflags -- GHC does not export 'OnOff', so we have to view it as string = [ e | Just e <- T.stripPrefix "Off " . T.pack . prettyPrint <$> extensions dynFlags] | otherwise -- When the module failed to parse, we don't have access to its -- dynFlags. In that case, simply don't disable any pragmas. = []

this is the only place Maybe gets checked and not disabling any pragmas would not have been noticed till now. I am really sorry for the noob questions

@Ailrun
Copy link
Member

Ailrun commented Feb 13, 2021

As I believe it surely works and disables some, so I just checked, and it properly disables No- pragmas. I think that problem is more like an issue occurring with some specific settings.

P.S. There's nothing wrong with asking questions :) If my answers sound too hostile, I'm sorry. It's probably because I'm too busy right now...

@ishmum123 ishmum123 marked this pull request as ready for review February 13, 2021 08:11
@ishmum123
Copy link
Contributor Author

@Ailrun this is most of what I could conjure up. In theory, if pm is not Nothing it should work. Would be grateful if you could check when you are free

plugins/default/src/Ide/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
plugins/default/src/Ide/Plugin/Pragmas.hs Outdated Show resolved Hide resolved
@ishmum123
Copy link
Contributor Author

@Ailrun what about now? Also, if this is done, please suggest me some other issues...

then do
contents <- LSP.getVirtualFileFunc lsp . toNormalizedUri $ docId ^. J.uri
text <- pure $ fmap VFS.virtualFileText contents
line <- pure . fromMaybe 0 $ findIndex (not . T.null) . take somh . T.lines =<< text
Copy link
Member

Choose a reason for hiding this comment

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

The intention is when we have

#! /usr/bin/env nix-shell
#! nix-shell --pure -i runghc -p "haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])"

{-# LANGUAGE ScopedTypeVariables #-}
module AfterShebang where

foo :: forall a. a -> a
foo = id @a

we get

#! /usr/bin/env nix-shell
#! nix-shell --pure -i runghc -p "haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])"

{-# LANGUAGE ScopedTypeVariables #-}
{-# LANGUAGE TypeApplications #-}
module AfterShebang where

foo :: forall a. a -> a
foo = id @a

right? Then I have one question: Why do we need to check contents? I notice that somh itself is good enough...
Do you have any other cases that are not covered by somh?

Copy link
Member

@Ailrun Ailrun Feb 13, 2021

Choose a reason for hiding this comment

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

BTW, this is why the tests fail. Current code actually gives

{-# LANGUAGE TypeApplications #-}
#! /usr/bin/env nix-shell
#! nix-shell --pure -i runghc -p "haskellPackages.ghcWithPackages (hp: with hp; [ turtle ])"

{-# LANGUAGE ScopedTypeVariables #-}
module AfterShebang where

foo :: forall a. a -> a
foo = id @a

as it checks from the first line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not figure out the case for contents as well. I mostly copied from CodeAction.hs -> endOfModuleHeader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the somh only but can't figure out why it still adds pragma to the first line. Sorry about that

Copy link
Member

Choose a reason for hiding this comment

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

Does it work in your local machine? GetParsedModuleWithComments errors probably come frome other tests, so could you check addPragma tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for being so patient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ailrun please let me know if I can help in anything

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm really busy for this week or two... maybe @jneira can also help? I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

will try

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jneira ... means a lot

test/functional/FunctionalCodeAction.hs Outdated Show resolved Hide resolved
@ishmum123 ishmum123 force-pushed the bug-fix/pragma-before-shebang branch from 83fab8a to 833c6a8 Compare February 14, 2021 22:43
@jneira
Copy link
Member

jneira commented Feb 19, 2021

The pr needs a rebase, i've try to do it here: ishmum123/haskell-language-server@bug-fix/pragma-before-shebang...jneira:pr1340
Not sure if it is correct though, it only compiles

@ishmum123 ishmum123 force-pushed the bug-fix/pragma-before-shebang branch from fa1af55 to 50bd268 Compare February 23, 2021 19:06
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

@ishmum123 thanks for the patience and your work, here and indirectly in #1417

@jneira jneira added merge me Label to trigger pull request merge and removed pr: needs rebase labels Feb 24, 2021
@ishmum123
Copy link
Contributor Author

@jneira my pleasure... would be grateful if you could point to another issue

@jneira
Copy link
Member

jneira commented Feb 26, 2021

@ishmum123 nice! there are some issues labeled with good first issue (well they are good as second or third too 😄 )

For example one that should be quite straightforward would be #1341

hololeap added a commit to hololeap/haskell-language-server that referenced this pull request Sep 23, 2022
Remove warning about hlint restrictions, as haskell#1340 has been merged.
michaelpj added a commit that referenced this pull request Sep 26, 2022
Remove warning about hlint restrictions, as #1340 has been merged.

Co-authored-by: Michael Peyton Jones <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language pragmas are inserted incorrectly at the top of file (before shebangs)
3 participants