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

Convert to ghc-9.2.1 #1247

Merged
merged 1 commit into from
Dec 12, 2021
Merged

Conversation

shayne-fletcher
Copy link
Collaborator

This is a complete port of HLint to the ghc-9.2.1 parse tree. All tests pass.

@shayne-fletcher
Copy link
Collaborator Author

Cc @alanz All done mate.

@shayne-fletcher shayne-fletcher force-pushed the wip/ghc-9.2.1 branch 2 times, most recently from f34254c to 918a74e Compare May 14, 2021 09:35
@ndmitchell
Copy link
Owner

Awesome work!

@ndmitchell
Copy link
Owner

GHC 9.2.1 is now out. Thoughts on this?

@shayne-fletcher
Copy link
Collaborator Author

GHC 9.2.1 is now out. Thoughts on this?

Got it. Time to move on it then. Leave it with me for now and I'll keep you posted.

@alanz
Copy link
Contributor

alanz commented Oct 30, 2021

For the apply-refact part it will need me to complete my current ghc-exactprint update. Which is getting close.

@ndmitchell
Copy link
Owner

I'm in no tearing rush - a leisurely upgrade is fine :)

@shayne-fletcher
Copy link
Collaborator Author

i rebased this branch on master today (oct 30, 2021).

the tests all pass but this one in 'Extensions.hs':

{-# LANGUAGE DeriveGeneric, TypeFamilies #-} \
data family Bar a; data instance Bar Foo = Foo deriving Generic

which produces the apparently unexpected output

Note: Extension DeriveGeneric is not used

(using the 9.2.1 release candidate ghc-lib-parser-9.2.1.20210509.tar.gz that the stack.yaml on this branch arranges to use)

this test was introduced only recently in #1277. @zliu41 as the author of that test might i trouble you to take a look? i removed the test for the moment.

this puts us in a good place for the next step: upgrade this branch to the ghc-lib-parser and ghc-lib-parser-ex newly released 9.2.1 official versions.

@zliu41
Copy link
Collaborator

zliu41 commented Nov 4, 2021

@shayne-fletcher Removing that test for now is good. Once you make cabal build work for this patch, I can look into it - otherwise it's difficult for me to investigate.

@LeventErkok
Copy link

@shayne-fletcher Maybe it's good to go now for GHC 9.2.1?

@shayne-fletcher
Copy link
Collaborator Author

@shayne-fletcher Maybe it's good to go now for GHC 9.2.1?

yep, it's high priority with us too. don't worry @LeventErkok , we got you! little patience please 😉

@shayne-fletcher shayne-fletcher force-pushed the wip/ghc-9.2.1 branch 2 times, most recently from a077cc4 to cc81186 Compare November 15, 2021 18:15
@shayne-fletcher shayne-fletcher marked this pull request as ready for review November 15, 2021 18:18
@shayne-fletcher
Copy link
Collaborator Author

@ndmitchell i believe this is ready to merge. all 813 tests pass locally. please note that minimum build compiler is now 8.10.1.

@shayne-fletcher
Copy link
Collaborator Author

shayne-fletcher commented Nov 15, 2021

p.s. locally my testing was with refactor-0.9.2.0 cc @alanz

@shayne-fletcher shayne-fletcher force-pushed the wip/ghc-9.2.1 branch 2 times, most recently from b11504e to a9617d0 Compare November 15, 2021 18:56
@shayne-fletcher
Copy link
Collaborator Author

@shayne-fletcher Removing that test for now is good. Once you make cabal build work for this patch, I can look into it - otherwise it's difficult for me to investigate.

@zliu41 cabal v2-build on this branch works as you'd hope for now!

src/Hint/Extensions.hs Outdated Show resolved Hide resolved
@ndmitchell
Copy link
Owner

Thanks! @shayne-fletcher - can you add a CI for GHC 9.2 and remove it for GHC 8.8 (see the .github/workflows).

It seems the Windows and Mac CI are both bust installing apply-refact. Any idea why?

I'll take a peek through the code now

Copy link
Owner

@ndmitchell ndmitchell left a comment

Choose a reason for hiding this comment

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

Nice work!

src/GHC/Util/SrcLoc.hs Outdated Show resolved Hide resolved
@shayne-fletcher
Copy link
Collaborator Author

Thanks! @shayne-fletcher - can you add a CI for GHC 9.2 and remove it for GHC 8.8 (see the .github/workflows).

pushed

It seems the Windows and Mac CI are both bust installing apply-refact. Any idea why?

no idea about that sorry.

I'll take a peek through the code now

@alanz
Copy link
Contributor

alanz commented Nov 22, 2021

It seems the Windows and Mac CI are both bust installing apply-refact. Any idea why?

It depends on ghc-exactprint, the release on hackage does not compile with GHC 9.2.1 as released.

I will be making a new release tomorrow, just need to clean up a few tests

@ndmitchell
Copy link
Owner

Thanks @alanz - my plan is to hold this PR in limbo a few days, merge nothing else to the repo in the intervening time, and wait for ghc-exactprint, then pull the trigger on this.

@alanz
Copy link
Contributor

alanz commented Nov 23, 2021

I just published ghc-exactprint-1.3.0

@arjunkathuria
Copy link

arjunkathuria commented Nov 25, 2021

hi, if all's good can we merge and publish this ? looking forward : )

Comment on lines +19 to +23
# hlint:
# ghc-lib: true
# ghc-lib-parser-ex:
# auto: false
# no-ghc-lib: false
Copy link

@unclechu unclechu Dec 8, 2021

Choose a reason for hiding this comment

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

This re-indentation doesn’t seem like a reasonable change. There must be at least 1 space after # (see flags). But after hlint there are two, which means the indentation is 1 but ghc-lib has 2 for indentation.

Suggested change
# hlint:
# ghc-lib: true
# ghc-lib-parser-ex:
# auto: false
# no-ghc-lib: false
# hlint:
# ghc-lib: true
# ghc-lib-parser-ex:
# auto: false
# no-ghc-lib: false

Comment on lines +11 to +14
args <- getArgs
errs <- hlint args
unless (null errs) $
exitWith $ ExitFailure 1
Copy link

Choose a reason for hiding this comment

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

5 spaces for indentation? But still the exitWith has 4.

Suggested change
args <- getArgs
errs <- hlint args
unless (null errs) $
exitWith $ ExitFailure 1
args <- getArgs
errs <- hlint args
unless (null errs) $
exitWith $ ExitFailure 1

Comment on lines +189 to +191
IEName n -> fromId (unLoc n)
IEPattern _ n -> ("pattern " ++) <$> fromId (unLoc n)
IEType _ n-> ("type " ++) <$> fromId (unLoc n)
Copy link

Choose a reason for hiding this comment

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

Broken alignment:

Suggested change
IEName n -> fromId (unLoc n)
IEPattern _ n -> ("pattern " ++) <$> fromId (unLoc n)
IEType _ n-> ("type " ++) <$> fromId (unLoc n)
IEName n -> fromId (unLoc n)
IEPattern _ n -> ("pattern " ++) <$> fromId (unLoc n)
IEType _ n -> ("type " ++) <$> fromId (unLoc n)

Comment on lines +41 to +43
res = [x | x <- hsmodImports xs
, pkg x /= Just (StringLiteral NoSourceText (fsLit "hint") Nothing)
]
Copy link

Choose a reason for hiding this comment

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

Suggested change
res = [x | x <- hsmodImports xs
, pkg x /= Just (StringLiteral NoSourceText (fsLit "hint") Nothing)
]
res = [x | x <- hsmodImports xs, pkg x /= Just (StringLiteral NoSourceText (fsLit "hint") Nothing)]

Comment on lines +97 to +104
adjustMessage x =
"Parse error: " ++
dropBrackets (
case stripInfix "parse error " x of
Nothing -> x
Just (prefix, _) ->
dropPrefix (prefix ++ "parse error ") x
)
Copy link

Choose a reason for hiding this comment

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

Suggested change
adjustMessage x =
"Parse error: " ++
dropBrackets (
case stripInfix "parse error " x of
Nothing -> x
Just (prefix, _) ->
dropPrefix (prefix ++ "parse error ") x
)
adjustMessage x =
"Parse error: " ++ dropBrackets errMsg
where
errMsg = maybe x (\(pfx, _) -> dropPrefix (pfx ++ subStr) x) (stripInfix subStr x)
subStr = "parse error "

@ndmitchell ndmitchell merged commit e6a6df4 into ndmitchell:master Dec 12, 2021
@ndmitchell
Copy link
Owner

It's got to the stage where we need to move, so landing, and will fix forward if we need to.

@shayne-fletcher shayne-fletcher deleted the wip/ghc-9.2.1 branch December 12, 2021 18:55
@ndmitchell
Copy link
Owner

Merged, but all the CI is red on GHC 9.2 because apply-refact is not yet GHC 9.2 compatiblehttps://github.com/mpickering/apply-refact/issues/114

@alanz
Copy link
Contributor

alanz commented Dec 23, 2021

I am working on the failing tests for apply-refact, and came across Bracket7, which has

yes = if (f x) then y else z

and a corresponding apply-refact refactoring of

[("tests/examples/Bracket7.hs:1:10-14: Suggestion: Redundant bracket\nFound:\n  if (f x) then y else z\nPerhaps:\n  if f x then y else z\n"
,[Replace
  { rtype = Expr
  , pos = SrcSpan {startLine = 1, startCol = 10, endLine = 1, endCol = 15}
  , subts = [("x",SrcSpan {startLine = 1, startCol = 11, endLine = 1, endCol = 14})]
  , orig = "x"}])]

I presume that only the positions are meaningful, as in "substitute 1:10-15 with 1:11-14", and the reference to "x" can be ignored?

@ndmitchell
Copy link
Owner

@alanz - @zliu41 is the expert here. I think (but am most definitely not sure) that the code is saying:

  • Due to pos Operate on 1:10-15, i.e. (f x)
  • Due to subts bind $x to 1:11-14, i.e. f x
  • Replace the whole thing with orig definition of $x so replace (f x) with f x

But that's just a guess on what I think it's doing.

@alanz
Copy link
Contributor

alanz commented Dec 24, 2021

That seems to make sense. So the "x" is a "hlint variable", rather than a quote of existing source code.

@zliu41
Copy link
Collaborator

zliu41 commented Jan 2, 2022

@alanz Yeah "x" is a template variable. In this case it happens to be the entire template (orig), but in general that's not the case.

@alanz
Copy link
Contributor

alanz commented Feb 1, 2022

mpickering/apply-refact#116 is ready for review, should hopefully unblock this

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.

7 participants