Looks like you failed to commit GHC.Debug.Thunks
.
Thanks for the bug rteport @treeowl, I agree that the warning should not trigger in the case you describe.
It sounds like a change localised to GHC.HsToCore.Match.warnAboutOverflowedOverLit
should be able to tackle this. I think it would be easy to make such a change; a robust test case that covers all the combinations described in the OP would suffice to validate the change.
Thanks for the bug report. I don't think we can easily change back the behaviour (in GHC 9.6 we defaulted the type variable, which we no longer do in 9.8). If you want to see the constraint, you should use an extra-constraints wildcard:
f :: _ => _
f x y = x + y
warning: [GHC-88464] [-Wpartial-type-signatures]
* Found extra-constraints wildcard standing for `Num a'
Where: `a' is a rigid type variable bound by
the inferred type of f :: Num a => a -> a -> a
at <interactive>:2:14-26
* In the type signature: f :: _ => _
warning: [GHC-88464] [-Wpartial-type-signatures]
* Found type wildcard `_' standing for `a -> a -> a'
Where: `a' is a rigid type variable bound by
the inferred type of f :: Num a => a -> a -> a
at <interactive>:2:14-26
* In the type signature: f :: _ => _
I believe this ticket is a duplicate of of #24425, so I will close.
Since ghc 9.8 the inferred types of wildcards (sometimes?) don't contains type class constraints.
Consider this code:
$ cat A.hs
f :: _
f x y = x + y
When compiled with GHC 9.6, I get this error:
A.hs:1:6: error: [GHC-88464]
• Found type wildcard ‘_’
standing for ‘Integer -> Integer -> Integer’
To use the inferred type, enable PartialTypeSignatures
• In the type signature: f :: _
|
1 | f :: _
| ^
But when compiled with GHC 9.8.2 I get this (notice the inferred type doesn't contain Num a constraint):
A.hs:1:6: error: [GHC-88464]
• Found type wildcard ‘_’ standing for ‘a -> a -> a’
Where: ‘a’ is a rigid type variable bound by
the inferred type of f :: a -> a -> a
at A.hs:2:1-13
To use the inferred type, enable PartialTypeSignatures
• In the type signature: f :: _
|
1 | f :: _
| ^
A.hs:2:11: error: [GHC-39999]
• No instance for ‘Num a’ arising from a use of ‘+’
Possible fix:
add (Num a) to the context of the inferred type of f :: a -> a -> a
• In the expression: x + y
In an equation for ‘f’: f x y = x + y
|
2 | f x y = x + y
| ^
I would expect that if I use the inferred type, I will get code that type checks (e.g. Num a => a -> a -> a
or Integer -> Integer -> Integer
).
This is breaking haskell-language-server functionality, which uses the type displayed in the warning (a -> a -> a
) in that it leads it to creation of code actions (small popups you can use to locally "fix" your code) that break user's code as in this example:
See this "broken" test case for illustration: https://github.com/haskell/haskell-language-server/blob/c50a0e118b8570b2bd405de6ff5be6a54926aa5d/plugins/hls-refactor-plugin/test/Main.hs#L665-L673
Optional:
I think the best is to have individual notes that cover certain details (such as the workaround with internal units). Then you can refer to the specific part that is relevant in the code, and you can also link to them from the main overview note Named default declarations
.
Can you include all the function type constructors in the test, namely FUN
, (=>)
, (==>)
, and (-=>)
? (That might not prove entirely straightforward given that e.g. -=>
is not exposed to users.)
That's the main thing I would be worried breaking here.
@blamario Sorry for taking so long to look at this, and thanks again for your contribution.
Please refer to a Note that explains how we handle export of named default declarations:
Good work on this MR @blamario, it's not far off at all.
I think you are not correctly dealing with imports (I suggest implementing the logic in rnImports
instead of using hptSomeThingsBelowUs
which is not right here). Other than that, my review comments are mainly suggestions to improve readability and maintainability of the code.
I don't think it's right to store cd_modules
here.
In places like the md_defaults
field of ModDetails
, the defaults are always the named defaults explicitly declared in the current module, so you should have a specific type for that (e.g. the current type minus the cd_modules
field).
When you look up the imported defaults (which I suggest to do in rnImports
), you would create some environment, e.g. a TyConEnv (ModuleEnv ClassDefaults)
which you then resolve into e.g. an overall defaulting environment (which is used in the typechecker do to defaulting in the current module). If you keep the location of the default declarations instead of discarding them, you can easily filter those that are local to the current module for the purpose of computing which named default declarations are exported without an explicit export list.
Why can't we continue to use disambigMultiGroup
as before? Now there is a fair bit of code duplication which is always a risk (e.g. fixing a bug in one code path but not the other).
These type signatures are much more complex than before.
Before, it was simple: you had a unary class cls
, an argument ty
, and you check whether you can solve cls ty
. Now we have a complicated 3-tuple with list arguments and it's hard to know what the function actually does from the type signature.
I think you should be able to continue using the old implementation (and thus type signature) of check_instance
if you simplify tcHsDefault
.
I would also hope this would make the implementation of resolveClass
more readable, as you would not have to construct all these 3-tuples.
Further to my previous comment about not changing the return type of tcRnImports
, the fact that you are discarding something here is also an indication of something being wrong in my opinion.
I don't think this is right. It will only look for imported defaults in the HPT
, which is where GHC stores information in memory when compiling multiple modules at once in --make mode. It isn't used in one-shot mode (when you are compiling individual files one by one and relying on interface files):
hptSomeThingsBelowUs extract include_hi_boot hsc_env uid mn
| isOneShot (ghcMode (hsc_dflags hsc_env)) = []
| otherwise
= ...
I believe this information should instead come from rnImports
.
Could you add an explanation of what this check is checking? It should say e.g. that the first argument consists of imported default declarations and the second of local declarations, and that you are performing the subsumption check as described in the proposal.
I don't think you should be returning anything separately from the tcg_env
; it seems wrong to change the return type of tcRnImports
just for this feature.
Can you re-structure the code (perhaps changing the type of the tcg_default
field) so that you can do the reportClashingDefaultImports
check below just using information stashed in the TcGblEnv
? Or perhaps you can move the check in order to preserve the return type of tcRnImports
?
Why do we take the first module here in cd_modules
? What if cd_modules
is empty, don't we want to warn too?
Is it possible to attach documentation to a named default declaration? In particular I think it would make sense to attach @since
annotations (but that could probably be left to a future MR).
I would have expected the built-in defaulting rules to always be included unless they are specifically overridden by a named default. However, here we only include them when there are no named defaults at all. Why does the presence of an explicit default MyClass (MyType1, MyType2)
suppress anything to do with the defaults for Num
? Is this specified in the GHC proposal?
I don't think you should re-use tcHsDeriv
, which has a fair bit of extra logic in order to handle partially applied classes, e.g. a class C a b c
with three arguments and a deriving clause of the form deriving (C Int Bool)
, where it needs to figure out the kind of the last argument.
For tcHsDefault
, you should be able to just use getClassPredTys_maybe
. Something like
case getClassPredTys_maybe ty of
Just (cls, []) ->
case classTyVars cls of
[tv] -> ... continue
_ -> addErrTc "non-unary class"
_ -> addErrTc "not a class TyCon"
Moreover, I think it would be good to avoid failing in the Tc
monad entirely. I think an error in a default declaration should not prevent GHC from attempting to typecheck the rest of the file; it's not a catastrophic error although it might lead to e.g. ambiguous type errors in the rest of the module.