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

Confusing goal with unfinished have #779

Open
Vtec234 opened this issue Nov 10, 2021 · 11 comments
Open

Confusing goal with unfinished have #779

Vtec234 opened this issue Nov 10, 2021 · 11 comments
Labels
enhancement New feature or request server Affects the Lean server code

Comments

@Vtec234
Copy link
Member

Vtec234 commented Nov 10, 2021

Description

In the following unfinished proof:

example : ∃n, n < 1 := by
   have h : 0 < 1 := 
                   --^ cursor here
   exact ⟨0, h⟩

the goal state shows up as

h : 0 < 1
⊢ ∃ n, n < 1

with 0 < 1 as the assumption. To see the "real" goal ⊢ 0 < 1 we have to write by. This is confusing for beginners, since really at that cursor there is no goal state at all -- one should produce a term. So I suggest we show an expected type there rather than a goal state.

Versions

8df2b07

@leodemoura
Copy link
Member

@Vtec234 Note that the example above is currently parsed as

example : ∃n, n < 1 := by
   have h : 0 < 1 := exact ⟨0, h⟩

The exact ⟨0, h⟩ is parsed as an application. The syntax tree is

(Term.app `exact [(Term.anonymousCtor "" [(numLit "0") "," `h] "")])

The info view shows the correct "Expected type" (0 < 1) when we hover over exact.

@leodemoura
Copy link
Member

@Kha Any ideas?

@Kha
Copy link
Member

Kha commented Jan 20, 2022

Good question. If we can make it show the expected type here, do we want to hide the goal by default if we're inside of a term? We definitely don't want to always do that, e.g. when typing a simp argument.

@leodemoura
Copy link
Member

I am inclined to mark this issue as "wontfix", and close it.

@Vtec234 I acknowledge the current behavior is confusing, but I don't see a good solution. Any suggestions on how to improve it?

@Vtec234
Copy link
Member Author

Vtec234 commented Jun 2, 2022

It is something that confused one of the students at the course we were teaching, and it briefly confused me as well. But I agree that changing this might be more trouble than it's worth, so closing is okay with me. The only thing I could think of would be to require colGt at have .. := (haveIdDecl). I guess it depends on whether

have h :=
term

is considered acceptable or not.

EDIT: Oh well, with colGt it parses differently but the goal state still includes h.

@gebner
Copy link
Member

gebner commented Jun 2, 2022

I'm not sure why you expect the goal state to change here. After have h := comes a term, not a tactic. (It would be great though if the expected type was shown directly after :=.)

Would it be less confusing if we had a have h by ... syntax?

@Vtec234
Copy link
Member Author

Vtec234 commented Jun 2, 2022

It would be great though if the expected type was shown directly after :=.

Yes, this is what I expected.

@leodemoura
Copy link
Member

leodemoura commented Jun 2, 2022

It would be great though if the expected type was shown directly after :=.

Yes, this is what I expected.

I agree. I did not have time to investigate this issue yet. Do you know why we are not getting the correct expected type here?

Would it be less confusing if we had a have h by ... syntax?

I like this idea.

@gebner
Copy link
Member

gebner commented Jun 2, 2022

Do you know why we are not getting the correct expected type here?

I'm pretty sure that's because we look for the smallest term node around the cursor, but there's no term node around the space after := because the space is part of :=:

import Lean
open Lean Elab Command

elab "#dump" t:term : command => do
  logInfo (repr t)

#dump by have h :=
  exact

-- ^^ newline is part of :=

@leodemoura
Copy link
Member

I'm pretty sure that's because we look for the smallest term node around the cursor, but there's no term node around the space after := because the space is part of :=:

I agree.

@Kha @gebner Any ideas on how to fix this? The first hack that comes to mind is to move the trailing spaces for the := atom to the leading spaces for the next atom/identifier. The parser is already invoking Syntax.updateLeading to make adjustments.

@leodemoura leodemoura added the enhancement New feature or request label Jun 3, 2022
@Kha
Copy link
Member

Kha commented Jun 3, 2022

I don't think that would work if what follows the := is either EOF or something that is definitely not a term (e.g. the next theorem). We could instead put an info node on :=, MVP for let only: Kha@30278e8
However, the output is still quite confusing given the unchanged outer goal
image

ChrisHughes24 pushed a commit to ChrisHughes24/lean4 that referenced this issue Dec 2, 2022
* `@[to_additive]` will now correctly guess names with `CoeTC`, `CoeT` and `CoeHTCT` in it
* rewrite function `targetName`. 
  - It will now warn the user if it gives a composite name that can be auto-generated (before `to_additive` would never warn if a composite name was given). 
  - the logic when `allowAutoName = true` now corresponds to the docstring
  - Fix a bug where declarations were silently allowed to translate to itself (maybe because the `return` statements returned a value for the whole function?)
  - Add some more tracing
  - The behavior of namespaces when giving a composite name has been changed. It will always generate a name with the same number of components. Example:
```lean
namespace MeasureTheory.MulMeasure
@[to_additive AddMeasure.myOperation] def myOperation := ...
-- before: generates `AddMeasure.myOperation` (and never gives a warning)
-- after: generates `MeasureTheory.AddMeasure.myOperation` (and probably warns that the name can be auto-generated)
end MeasureTheory.MulMeasure
```
* This should fix all problems in leanprover#659 other than leanprover#660

Minor changes:
* When applying `@[to_additive]` to a structure, add a trace message if no translation is inserted for a field.
* Define `Name.fromComponents` and `Name.splitAt`
* Reduce transitive imports of `Tactic/toAdditive`
* Move some auxiliary declarations from `Tactic/Simps` to more appropriate places 
  - swap arguments of `String.isPrefixOf?`
  - improve `Name.getString`

Co-authored-by: Scott Morrison <[email protected]>
@mhuisi mhuisi added the server Affects the Lean server code label Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server Affects the Lean server code
Projects
None yet
Development

No branches or pull requests

5 participants