Matching of qualified names

[pepeiborra]

When asked to unfold a function foo defined in module A, currently retrie will ignore qualified occurrences of foo. I would expect that these occurrences would be unfolded too. Test case:

-u Qualified.foo
===
 module Qualified where

 foo :: Int -> Int
 foo x = x - x

-quux y = Qualified.foo y
+quux y = y - y

Results in:

Test suite test: RUNNING...
retrie
  golden
    Qualified:    
============================================================
Original:
============================================================
module Qualified where

foo :: Int -> Int
foo x = x - x

quux y = Qualified.foo y

============================================================
Expected:
============================================================
module Qualified where

foo :: Int -> Int
foo x = x - x

quux y = y - y

============================================================
Got:
============================================================
module Qualified where

foo :: Int -> Int
foo x = x - x

quux y = Qualified.foo y

============================================================
Diff:
============================================================
6c6
< quux y = y - y
---
> quux y = Qualified.foo y

============================================================


FAIL
      Exception: HUnitFailure (Just (SrcLoc {srcLocPackage = "main", srcLocModule = "Golden", srcLocFile = "tests/Golden.hs", srcLocStartLine = 119, srcLocStartCol = 7, srcLocEndLine = 119, srcLocEndCol = 43})) (Reason "file contents differ")
  parseQualified: OK

[xich]

Thanks for the test case!

This is a consequence of the fact that we're working with parsed AST instead of the renamed AST (because that is all that ghc-exactprint supports). We have to match on variable names syntactically.

Typically we'd work around this by using an adhoc rewrite instead of an unfold:

retrie --adhoc "forall x. Qualified.foo x = x - x"

which should do what you want.

I suppose I could generate extra unfold variants with the qualified name in addition to the unqualified variants, but it'd be pretty brittle. If an import was ever aliased with as, the rewrite wouldn't apply. If you think that is worth doing anyway, it'd be easy enough.

In the past, I loosened variable matching so unqualified stuff could match qualified stuff (basically, just ignore the qualification), but that led to lots of bad behavior, and generally confused everyone, so I took it out.

Medium term, I guess I could look at the imports of each module before rewriting it and elaborate the rewrite set with qualifications and aliases, etc. Not sure if the effort/payoff is worth it though.

Longer term, I'd love to get ghc-exactprint working over renamed source. Not sure if that is even possible though. Maybe @alanz has an opinion on that idea.

[alanz]

@xich FYI in HaRe we make use of a namemap from the renamed source back to the parsed source.

Every name is Located, specifically so its SrcSpan can be used for this purpose.

And to be honest some names are only resolved after type checking (such as overloaded record fields). But the same lookup process holds.

This map is also constructed in haskell-ide-engine, as so likely in ghcide/hls too.

[xich]

Ah! Of course. That's a great idea. I'll see about incorporating that.

[pepeiborra]

Thanks for the explanation and workaround suggestions.

I think that this limitation should be mentioned clearly in the README. It's pretty common to import modules qualified, and surprising when rewrites don't happen.

I would actually find your short term solution (ignoring the qualifications) more consistent. Perhaps it should be a user option.

[jrp2014]

Longer term, I'd love to get ghc-exactprint working over renamed source. Not sure if that is even possible though. Maybe @alanz has an opinion on that idea.

@xich This would be a great leap forward.

Currently, when writing a typecheckerd plugin, you seem to need to reparse the source to get the Anns, which is a bottleneck.

I much prefer your API to exactprint, keeping tha Anns and AST together (end would welcome its integration into exactprint). But it is not so straightforward to use if you are obliged to work with a both the source and typechecked source ASTs to do what needs to be done. I'll have a look at @alanz s namemap to see what it offers.

[alanz]

Longer term, I'd love to get ghc-exactprint working over renamed source. Not sure if that is even possible though. Maybe @alanz has an opinion on that idea.

I initially aimed to do this, in HaRe, and various approaches to managing the reproduction of the AST.

Unfortunately there is too much information loss between the parsed and renamed AST that makes it impossible. The fallback was to "index" each name location with a SrcSpan to allow the name matchup, and to make the ParsedSource a 100% representation of the original source (modulo tabs, trailing whitespace per line).

[xich]

@pepeiborra Does haskell-language-server already have the renamed source on hand at the point it calls retrie?

To actually rename/typecheck a module, we need to rename/typecheck the transitive imports, right? This will negate a lot of the speed advantages retrie currently has, so I'd prefer to not make this the default. But, if the caller already has the renamed/typechecked source on hand, we could just use it as an oracle.

Also, I think the precise name matching makes sense when the source of the rewrite is a decl (unfold/fold/type/rule), but not adhoc rewrites, because there is no real notion of what is in-scope for the adhoc rewrites. Sound reasonable?

[alanz]

Just remember, overloaded record fields only get resolved by the type checker, so to be sure you have to have that. And or not hit them, either way it is something to keep in mind.

[pepeiborra]

Haskell-language-server does have the renamed source on hand only for modules open in the editor. But the plugin can compute the renamed source for all the other modules as needed, using the correct package flags etc.

To rename/typecheck a module, you would only need the interface files of the transitive imports. The ghc api can take care of this automatically if used in --make mode. Allowing the caller to provide an oracle would still be desirable for integrating with other tools of course.

I agree that for ad-hoc rewrites you wouldn't want precise name matching. What do you want to use ad-hoc rewrites for? I'm having trouble coming up with a use case.

[alanz]

transitive imports

i.e. the full module graph? you need modules that are importing the one with the rename, if it is exported.

[xich]

What do you want to use ad-hoc rewrites for?

Partially it's just convenience. The adhoc rewrites are just RULES pragmas under the hood, and this saves you from having to add them to a file. The biggest use for adhoc and rule-forward/backward is for rewriting things where you don't have the definition handy (say, because it is defined in another package).