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 fix for import placement #2402

Closed
wants to merge 1 commit into from

Conversation

nini-faroux
Copy link
Contributor

@nini-faroux nini-faroux commented Nov 25, 2021

This is related to #2401 and #2392 but with imports in ghcide.

So it's only an issue when the file doesn't have any existing imports or a module declaration already - in those cases it relies on the ParsedSource type from the GHC parsing.

But in the case where neither of those are in the file it would fall back on the same logic as in the pragma plugin, and so had the same bug when there was multiline LANGUAGE or OPTS_GHC pragmas, like:

Before:
{-# LANGUAGE RecordWildCards,
OverloadedStrings #-}

After:
{-# LANGUAGE RecordWildCards,
Import Data.Monoid
OverloadedStrings #-}

This fixes that issue. It works for all the cases except for the issue raised in #2392, if there's an OPTIONS_GHC pragma later in the file the import will be incorrectly added under that.

I think it's probably better to wait for the parser to deal with that case instead of trying to do the same thing in two places, and I can adapt this solution to use that then.

@jneira
Copy link
Member

jneira commented Nov 29, 2021

thanks for the pr, I am gonna mark it as WIP, to wait to the pr using the new parser
alternatively regression tests could be extracted in another pr, marked as known broken if needed and be merged before any try to fix it

@nini-faroux
Copy link
Contributor Author

thanks that makes sense, I'll update it when the parser is ready and add the new tests then too

@jneira jneira marked this pull request as draft November 29, 2021 14:01
@jneira
Copy link
Member

jneira commented Nov 30, 2021

#2392 is already merged

@nini-faroux
Copy link
Contributor Author

hi @jneira, I'm not too sure about the best way to integrate the parser in ghcide to be honest, I don't think it makes sense to copy the whole thing into ghcide and have it in two places, or to include a different solution to it in ghcide, so I guess it might be a case of putting it in it's own module and importing it into the pragmas plugin and ghcide - but I think it would be better to leave that to you to decide on. So I might just extract the tests like you mentioned and add 'known broken'. 

The place it's needed is where the 'findNextPragmaPosition' function is now in 'ghcide/src/Development/IDE/Plugin/CodeAction.hs' - which arises when there's no module declaration or imports in the file.

I also noticed #2402 - I can include a fix for that if you want - it's just a small change and unrelated to the case that needs the parser. 

@nini-faroux
Copy link
Contributor Author

I'll close this and add the known broken tests in a separate pr now.

@nini-faroux nini-faroux closed this Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants