-
-
Notifications
You must be signed in to change notification settings - Fork 370
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 sig lens for where clauses #2966
base: master
Are you sure you want to change the base?
Conversation
-- | Get the type string of a binding id | ||
bindToSig :: Id -> HscEnv -> GlobalRdrEnv -> IOEnv (Env TcGblEnv TcLclEnv) String | ||
bindToSig id hsc rdrEnv = do | ||
env <- tcInitTidyEnv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this expensive? It seems like we might do it a lot...
| WhereBindings{..} <- localBindings | ||
, let sigSpans = getSrcSpan <$> existedSigNames | ||
, WhereBinding{..} <- bindings | ||
, let idsWithoutSig = filter (\x -> getSrcSpan (idName x) `notElem` sigSpans) bindingId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit odd to me: we're checking that a binding doesn't have a signature by comparing source spans. Why not just compare the ids directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two names are not exactly equal, one name is for the signature and another is for the binding, but they have the same SrcSpan
. I've commented it in the definition of WhereBinidng
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems surprising? I would really have expected that the names of the identifiers would be the same? Or maybe there's a different comparison method you're supposed to use there?
I rewrite the test to make it clear. I increasingly find that I know little about type information in Haskell, I'm confused by the concept of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marked two known failed tests which I can't tackle.
f aa bb = (aa, ida bb) | ||
where | ||
ida :: b -> b | ||
ida = id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails as you may guess...
f = undefined | ||
where | ||
g :: Num a => a -> a -> a | ||
g a b = a + b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Num a
lost here, the same method can show constraints for top-level bindings...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that the lens cannot deduce Num a =>
here if the signature is not present?
I find that ok, constraints belong at the top level, not in local type signatures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for ignoring this...
Are you saying that the lens cannot deduce Num a => here if the signature is not present?
Yes.
I find that ok, constraints belong at the top level, not in local type signatures
But it's not correct without constraints.
Yeah, it's very complicated and I don't understand it either. Worth talking to the GHC devs in #ghc, though, they're very helpful. |
Some notes from what I observed:
|
47f2b3b
to
c9daf86
Compare
Close #2943.