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

Join nested IO actions of the form IO (IO ()) #2459

Merged
merged 4 commits into from
Dec 10, 2021

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Dec 9, 2021

I think these are some accidental remnants of #2434.

Nothing broke for me, I just reviewed code and noticed the type of invalidateShakeCache was inferred to IO (IO ()).

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.

thanks for the clean up

@michaelpj michaelpj requested a review from pepeiborra December 9, 2021 14:26
@pepeiborra
Copy link
Collaborator

pepeiborra commented Dec 9, 2021

Are there any compiler warnings that could have prevented this?

@michaelpj
Copy link
Collaborator

Isn't there a warning saying a do-block expression is discarding a result? I'd have expected that to fire here.

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2021

Isn't there a warning saying a do-block expression is discarding a result? I'd have expected that to fire here.

Yeah, same, but it didn't. No idea why not.

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2021

I thought we are already using -Wunused-do-bind, but maybe we aren't, I'll give it a try.

@jneira
Copy link
Member

jneira commented Dec 9, 2021

I thought we are already using -Wunused-do-bind, but maybe we aren't, I'll give it a try.

In this pr? or could we merge it?

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2021

image

This is interesting. I don't know what is going on.

EDIT: this is what the ghc invocation looks like:

/home/hugin/.ghcup/bin/ghc --make -fbuilding-cabal-package -O -static -dynamic-too -dynosuf dyn_o -dynhisuf dyn_hi ... -XHaskell2010 -XApplicativeDo -XBangPatterns -XDeriveFunctor -XDeriveGeneric -XDeriveFoldable -XDeriveTraversable -XFlexibleContexts -XGeneralizedNewtypeDeriving -XLambdaCase -XNamedFieldPuns -XOverloadedStrings -XRecordWildCards -XScopedTypeVariables -XStandaloneDeriving -XTupleSections -XTypeApplications -XViewPatterns -XDataKinds -XTypeOperators -XKindSignatures Control.Concurrent.Strict ...  Development.IDE.Session ... -Wunused-do-bind -hide-all-packages -haddock -Wunused-do-bind

contains -Wunused-do-bind twice, as I supply it via cabal.project and ghcide.cabal.

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2021

Let's look into some more details before merging, imo.

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2021

Very interesting:
image

@fendor
Copy link
Collaborator Author

fendor commented Dec 9, 2021

I don't understand, copying that code to another project highlights correctly both occurrences. Even deleting everything except these two functions doesnt work for me, i.e. still shows only one warning.

@pepeiborra
Copy link
Collaborator

I found it: it's ApplicativeDo, which is enabled in ghcide.cabal. Please disable it @fendor since it doesn't make any difference with hls-graph anymore.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!

@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Dec 10, 2021
@mergify mergify bot merged commit 7794398 into haskell:master Dec 10, 2021
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.

4 participants