Persist name resolution for field names #2466
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facundominguez
force-pushed
the
fd/resolve-field-names
branch
from
da5d2a5 to
dcac999
Compare
|
I managed to remove most of the qualify passes after fixing a mistake in The only pass that remains is |
sjoerdvisscher
approved these changes
Dec 13, 2024
| (F.dummyPos "expand-iSpecs2") | ||
| [] | ||
| (M.fromList dependencySpecs) | ||
| mySpec2 = Bare.expand rtEnv l mySpec1 where l = F.dummyPos "expand-mySpec2" |
There was a problem hiding this comment.
Clean-up: I think you should inline the l here.
| @@ -497,8 +491,7 @@ makeSpecQual _cfg env tycEnv measEnv _rtEnv specs = SpQual | |||
|
|
|||
| makeQualifiers :: Bare.Env -> Bare.TycEnv -> (ModName, Ms.Spec F.Symbol ty) -> [F.Qualifier] | |||
| makeQualifiers env tycEnv (modn, spec) | |||
| = fmap (Bare.qualifyTopDummy env modn) | |||
| . Mb.mapMaybe (resolveQParams env tycEnv modn) | |||
| = Mb.mapMaybe (resolveQParams env tycEnv modn) | |||
| where | ||
| qual t es = qualifyTermExpr env name rtEnv t <$> es | ||
| qual es = expandTermExpr rtEnv <$> es |
There was a problem hiding this comment.
Clean-up: line-up equal signs or remove whitespace
| = (v, t, e) | ||
| where | ||
| t = Bare.qualifyTop env name (F.loc t0) t0 | ||
| (t0, e) = makeAssumeType allowTC embs lmap dm x mbT v def | ||
| (t, e) = makeAssumeType allowTC embs lmap dm x mbT v def |
| @@ -656,7 +656,7 @@ ofBDataCtorTc env name l l' tc αs ps πs _ctor@(DataCtor _c as _ xts res) c' = | |||
| res' = Bare.ofBareType env name l (Just ps) <$> res | |||
| t0' = dataConResultTy c' αs t0 res' | |||
| _cfg = getConfig env | |||
| (yts, ot) = qualifyDataCtor (not isGadt) name dLoc (zip xs ts', t0') | |||
| (yts, ot) = (zip xs ts', t0') | |||
| where | ||
| lookupLHName s = Mb.fromMaybe (panic (Just (GM.fSrcSpan ldcp)) $ "unexpected symbol: " ++ show s) $ lookup s lhNameMap |
| @@ -1113,10 +1099,10 @@ mkReft _ _ _ _ | |||
| ------------------------------------------------------------------------------------------- | |||
| makeSpecName :: Bare.Env -> Bare.TycEnv -> Bare.MeasEnv -> ModName -> GhcSpecNames | |||
| ------------------------------------------------------------------------------------------- | |||
| makeSpecName env tycEnv measEnv name = SpNames | |||
| makeSpecName env tycEnv measEnv _name = SpNames | |||
There was a problem hiding this comment.
Should the _name argument be removed?
| @@ -75,13 +75,13 @@ makeRTEnv env modName mySpec dependencySpecs lmap | |||
| = renameRTArgs $ makeRTAliases tAs $ makeREAliases eAs | |||
| where | |||
| tAs = [ t | (_, s) <- specs, t <- Ms.aliases s ] | |||
There was a problem hiding this comment.
Clean-up: remove whitepsace here too.
| expandMeasure :: Bare.Env -> ModName -> BareRTEnv -> BareMeasure -> BareMeasure | ||
| expandMeasure env name rtEnv m = m | ||
| expandMeasure :: BareRTEnv -> BareMeasure -> BareMeasure | ||
| expandMeasure rtEnv m = m | ||
| { msSort = RT.generalize <$> msSort m |
|
Thanks @sjoerdvisscher! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another step for #2169.
Also fixes #2412 by making the field names available as measures as the documentation says it should.
The main contribution of this PR is persisting name resolution of field names. They are treated much the same as the names of measures in #2464.
To get tests passing I had to stop LH from trying to qualify names at some places, which would interfere with name resolution of field names. This made me aware that much of the passes to qualify identifiers are now redundant, so I removed most of them.
An additional commit fixes name resolution for built-in data constructors and names in the logic map, which previously produced names with
makeLocalLHName, and now they are constructed withmakeLogicLHName. The big difference is that the later causes names to be qualified when converted to symbols. These omissions were identified when removing calls to qualifying passes.The commit that makes
showLHNameDebugthe show instance forLHNameis a follow up from #2456. A few error messages were impacted and there might be still a few places that aren't caught by tests.Given that I had to remove some qualifying passes, I updated the mechanism to resolve expression aliases, which does require occurrences of expression aliases to be qualified. The name resolution pass now makes sure they are qualified.
Lastly I removed some calls to
getLHNameSymbolfrom Bare.hs, replacing them with eitherlhNameToUnqualifiedSymbolorlhNameToResolvedSymbol, which defines what is the intended behavior.