Require newtype constructors to be imported for coercing them#3937
Conversation
|
Is the new |
It didn’t occur to me that empty golden tests were equivalent to checking for the absence of warnings 🤦 I removed |
| let usedImports = fold $ M.lookup moduleName usedImportsByModuleName | ||
| usedImports' = foldl' (flip $ \(fromModuleName, newtypeCtorName) -> | ||
| M.alter (Just . (fmap DctorName newtypeCtorName :) . fold) fromModuleName) usedImports checkCoercedNewtypeCtorsImports | ||
| censor (addHint (ErrorInModule moduleName)) $ lintImports checked exEnv' usedImports' |
There was a problem hiding this comment.
Could you please add a comment here saying that we need to wait until after type checking to lint imports because newtype constructors may be marked as used as part of Coercible solving?
| -- since this way, we can provide good error messages | ||
| -- during instance resolution. | ||
| , checkCoercedNewtypeCtorsImports :: S.Set (ModuleName, Qualified (ProperName 'ConstructorName)) | ||
| -- ^ Newtype constructors imports required to solve Coercible constraints |
There was a problem hiding this comment.
This field is only for keeping track of newtype constructors which have been used so that we don't emit unused import warnings for them, right? If so could you please mention that here?
|
I added some comments, let me know if this is clear enough! |
src/Language/PureScript/Make.hs
Outdated
| -- Imports cannot be linted before type checking because we need to | ||
| -- known which newtype constructors are used to solve Coercible | ||
| -- constraints in order to not report them as unused. | ||
| censor (addHint (ErrorInule moduleName)) $ lintImports checked exEnv' usedImports' |
There was a problem hiding this comment.
typo here? I think this is causing CI to fail
There was a problem hiding this comment.
I guess I was too tired even for adding comments properly 🤦
|
Looks great 👍 please feel free to hit merge once CI is passing |
kl0tl
force-pushed
the
coercible/newtype-constructors-unwraping
branch
from
6551c54 to
c2da719
Compare
3 participants
As noticed by @hdgarrood in #3927 (comment), merely being exported isn’t a sufficient condition for a newtype to be unwraped when solving Coercible constraints because of internal modules.
This pull request requires newtypes constructors to be imported for them to be unwrapable and ensures
UnusedDctorImportwarnings aren’t emitted for newtype constructors imported only for the sake of solving Coercible constraints.