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

assumption? #281

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jalex-stark
Copy link

See this topic and this topic on zulip.

@@ -0,0 +1,3 @@
{"msgs":[{"caption":"trace output","file_name":"assumption.lean","pos_col":2,"pos_line":3,"severity":"information","text":"Try this: h1\n"}],"response":"all_messages"}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is actually garbage, I don't know how to get the output of the test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cd tests/lean
./test_single.sh ../../bin/lean assumption.lean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output is still wrong. Please fix it until the command above prints -- checked.

Copy link
Author

@jalex-stark jalex-stark Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm actually I still don't know how to build bin/lean in the first place
probably it's these instructions
https://github.com/leanprover-community/lean/blob/master/doc/make/index.md#generic-build-instructions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, where are you stuck?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I'm just waiting on the build. before I was stuck on understanding the error messages that came out of ./test_single.sh ../../bin/lean assumption.lean

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I have a real lean binary. I'm getting "error: unexpected token" at the question mark. Most of my debugging strategies rely on VSCode; is there a way to tell it to use the local lean binary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Seems like the current commit gives lots of errors like this. I thought the approach using the optional parse argument would prevent these. I think I should give up.

/Users/jstark/lean-projects/lean/library/init/meta/interactive.lean:1057:30: error: type mismatch at application
  try assumption
term
  assumption
has type
  parse (tk "?")? → tactic unit
but is expected to have type
  itactic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should just replace these references to assumption by tactic.assumption. The ? parser only works when the tactic is parsed in an begin ... end block (or by ...).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I have implemented that suggestion in the latest commit. I'm still getting the following, which seems to be the opposite problem (i want it to use the version with the parser and it uses the version without) I tried adding import tactic.interactive but i could not resolve that import.

/Users/jstark/lean-projects/lean/tests/lean/assumption.lean:3:12: error: unexpected token
/Users/jstark/lean-projects/lean/tests/lean/assumption.lean:4:0: error: invalid 'end', there is no open namespace/section

@gebner
Copy link
Member

gebner commented May 27, 2020

I feel obligated to mention that you could also implement assumption? in mathlib using the trick described here.

jalex-stark and others added 2 commits June 3, 2020 18:56
Co-authored-by: Yury G. Kudryashov <urkud@urkud.name>
@jalex-stark jalex-stark changed the title [WIP] assumption? assumption? Jun 3, 2020
@gebner
Copy link
Member

gebner commented Jun 4, 2020

bors try

bors bot added a commit that referenced this pull request Jun 4, 2020
@bors
Copy link

bors bot commented Jun 4, 2020

try

Build failed:

@gebner
Copy link
Member

gebner commented Jun 4, 2020

bors try

bors bot added a commit that referenced this pull request Jun 4, 2020
@bors
Copy link

bors bot commented Jun 4, 2020

try

Build failed:

@gebner
Copy link
Member

gebner commented Jun 4, 2020

I think for the ? notation to work, you need to add this before assumption: (copied from rcases)

precedence `?`:max

You should also add an assumption' tactic (of type tactic unit) that you can use as auto_param. That is, replace . assumption by . assumption' in two tests.

@jalex-stark
Copy link
Author

I think for the ? notation to work, you need to add this before assumption: (copied from rcases)

precedence `?`:max

You should also add an assumption' tactic (of type tactic unit) that you can use as auto_param. That is, replace . assumption by . assumption' in two tests.

There's currently an assumption' tactic that's essentially all_goals {assumption}. which one deserves the name?
(I don't know what an auto_param is, and I don't know which two tests you want replaced.)

@gebner
Copy link
Member

gebner commented Jun 4, 2020

If you write . assumption after the type of an argument, then Lean will try to automatically fill in the argument by calling the tactic assumption. This is also works for structure fields.

There are two tests that use . assumption (just search for it). The auto_param is definitely the least important, the current assumption' should stay as it is.

@jalex-stark
Copy link
Author

If you write . assumption after the type of an argument, then Lean will try to automatically fill in the argument by calling the tactic assumption. This is also works for structure fields.

There are two tests that use . assumption (just search for it). The auto_param is definitely the least important, the current assumption' should stay as it is.

so these tests need . assumption replaced by . tactic.assumption? or we should make assumption'' an alias for tactic.assumption? (I don't really know what's going on and my instinct is to blindly try to implement what you tell me.)

@jalex-stark
Copy link
Author

in the latest commit, I have
./test_single.sh ../../bin/lean assumption.lean gives output -- checked

but the . assumption tests are I think still broken, working on it.

@gebner
Copy link
Member

gebner commented Jun 4, 2020

I think you should add an assumption'' and use that in the tests.

bors try

bors bot added a commit that referenced this pull request Jun 4, 2020
@bors
Copy link

bors bot commented Jun 4, 2020

try

Build failed:

@gebner
Copy link
Member

gebner commented Jun 4, 2020

Oh wow, the precedence change broke a lot of stuff. You should post a message on Zulip about this. I'm not sure what the correct solution here is.

@jalex-stark
Copy link
Author

I feel obligated to mention that you could also implement assumption? in mathlib using the trick described here.

Maybe this approach would get around the current problem. Note that rcases is in mathlib and the precedence stuff doesn't break there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants