Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Try this" replaces incorrect segment #313

Closed
grhkm21 opened this issue Oct 19, 2023 · 6 comments
Closed

"Try this" replaces incorrect segment #313

grhkm21 opened this issue Oct 19, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@grhkm21
Copy link

grhkm21 commented Oct 19, 2023

Hi. Here's code:

import Mathlib.Tactic.Basic

open Lean Elab.Tactic Std.Tactic.TryThis

syntax (name := change') "change'" (ppSpace colGt term)? : tactic

elab_rules : tactic
| `(tactic|change'%$tk $[$sop:term]?) => withMainContext do
  let dstx ← delabToRefinableSyntax (← getMainTarget)
  addSuggestion tk (← `(tactic| change $dstx)) (origSpan? := ← getRef)

def f := fun x ↦ x + 1
def g := fun x ↦ x + 1
example : f 3 = 4 := by
  change' a b c d e f g

With this code, it is supposed to change the entire line, as specified by origSpan?. Apparently it does on VSCode c.f. Mario. However, the plugin doesn't - it replaces it with f 3 = 4 a b c d e f g.
Screenshot 2023-10-19 at 21 20 20
Screenshot 2023-10-19 at 21 20 25

grhkm21 added a commit to grhkm21/lean.nvim that referenced this issue Oct 19, 2023
Fixes Julian#313. The diagnostics objects already includes `end_lnum` and
`end_col`, so we can just use them. Not sure why neither VSCode nor your
code uses it.
@Julian Julian added the bug Something isn't working label Oct 20, 2023
Julian added a commit that referenced this issue Oct 20, 2023
They're incomplete, as evidenced best by #313, so when that
is fixed we should revisit porting more of the Lean 3 tests to the file.
@Julian
Copy link
Owner

Julian commented Nov 5, 2023

This seems to work correctly here (now?). I'm on Lean 4.3.0rc1, Mathlib4 df32aa2ebe0f1ef9bce7831b1bcc0723f07a4724 and NVIM v0.9.4. Can you confirm whether it does or doesn't for you?

@grhkm21
Copy link
Author

grhkm21 commented Nov 7, 2023

Hi, sorry for the late reply. No, it doesn't work for me. I'm on NVIM v0.9.4, Lean v4.3.0-rc1, Mathlib b6ec7450650a5945bf4244751be4a5cf1fee962f. It's very interesting that it works for you, my understanding is it shouldn't...

Also, I think I have a fix idea, but I haven't gotten to implement it yet. Essentially, we should implement the .getWidgets RPC (found here) and use those directly to get the replacement range (and the js widget source code, which we ignore for now).

@Julian
Copy link
Owner

Julian commented Nov 7, 2023

This is what I see here:

Screen.Recording.2023-11-06.at.10.34.05.PM.mov

Will need some way of reproducing the broken behavior I think

@Julian
Copy link
Owner

Julian commented Nov 7, 2023

To be sure -- are you using lean.trythis.swap or the code action here? You should be using the latter on Lean 4, given it exists now.

@grhkm21
Copy link
Author

grhkm21 commented Nov 7, 2023

I was using trythis, and I have gotten it to work with code action now :) Thanks a lot!

@Julian
Copy link
Owner

Julian commented Nov 7, 2023

Cool, ok glad it works, I'll try to make this clearer -- I'd already removed lean.trythis from the readme but I think the next step would be not binding it at all in Lean 4 buffers, which should send a message at least. I'll likely do that and then close this. Thanks regardless for the issue.

@Julian Julian closed this as completed in fa25b14 Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants