Keep type lenses stable

[July541]

I got the same effect without using PositionMapping compared to instance lenses, but don't know why.

Can anyone explain this?

[michaelpj]

I think you should still need the position mapping otherwise the edit might go in the wrong place? It doesn't look like there's a usesWithStale that just gives you one PositionMapping for the lot, which feels like an oversight (probably what you want if you're getting multiple stale pieces of info). I guess otherwise you could try to use the PositionMapping for the bit you get positions from?

[July541]

Introducing useWithStale need some refactoring. Taking this opportunity, I'm wondering if it is worth extracting type lens code to a new plugin?

cc @michaelpj @pepeiborra

[michaelpj]

No particular opinion from me, I can't remember if we have any clear policy about where we're going with the ghcide plugins.

[July541]

Replace use with useWithStale without more design.

I'll open a new pr to refactor these messed code.

[July541]

@michaelpj Would you mind doing a review, I don't want mix bug fixing and refactoring

[michaelpj]

A few nits but generally looks fine.

I generally feel like we should have a usesWithStale that returns a single position mapping for all the values, so you can request many things and get a single coherent stale mapping. The current setup probably works but feels risky to me.

[July541]

Stale lens for diagnostics doesn't make sense, because we always have fresh diagnostics even if current module parsed failed( the diagnostic is parse failed).

So there is nothing to do if we generate lenses from diagnostics.

[michaelpj]

Stale lens for diagnostics doesn't make sense, because we always have fresh diagnostics even if current module parsed failed

But don't we mix together things we got from the diagnostics with things we got from the bindings? So the locations could be all messed up there? I haven't quite understood what's going on there.

[July541]

But don't we mix together things we got from the diagnostics with things we got from the bindings? So the locations could be all messed up there? I haven't quite understood what's going on there.

We don't. The secret is getDiagnostics will never fail, which means the lenses from diagnostic will flicker but others from parsed module won't.

We have 3 modes for type lenses, only lenses from generateLensForGlobal should keep stable, suggestLocalSignature relies on diagnostics. Different modes need different methods to generate lenses, so they won't be mixed.

haskell-language-server/ghcide/src/Development/IDE/Plugin/TypeLenses.hs

Lines 135 to 140 in 6640ebf

	case mode of
	Always ->
	pure (catMaybes $ generateLensForGlobal <$> gblSigs')
	<> generateLensFromDiags (suggestLocalSignature False env tmr bindings) -- we still need diagnostics for local bindings
	Exported -> pure $ catMaybes $ generateLensForGlobal <$> filter gbExported gblSigs'
	Diagnostics -> generateLensFromDiags $ suggestSignature False env gblSigs tmr bindings

[July541]

I mixed the scene if mmp is Nothing, thanks ci halt this 😅