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

[Merged by Bors] - feat(Mathlib/Tactic/TypeCheck): add type_check tactic #318

Closed

Conversation

j-loreaux
Copy link
Collaborator

@j-loreaux j-loreaux commented Jul 18, 2022

@arthurpaulino
Copy link
Collaborator

Gotta add it to the Mathlib.lean import list 👍🏼

Comment on lines 3 to 11
example : True := by {
type_check Nat; -- Type
type_check Bool.true; -- Bool
type_check nat_lit 1; -- Nat
type_check (1 : Nat); -- Nat
type_check (True : _); -- Prop
type_check ∀ x y : Nat, x = y; -- Prop
type_check fun x : Nat => 2 * x + 1; -- Nat -> Nat
exact True.intro }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
example : True := by {
type_check Nat; -- Type
type_check Bool.true; -- Bool
type_check nat_lit 1; -- Nat
type_check (1 : Nat); -- Nat
type_check (True : _); -- Prop
type_check ∀ x y : Nat, x = y; -- Prop
type_check fun x : Nat => 2 * x + 1; -- Nat -> Nat
exact True.intro }
example : True := by
type_check Nat -- Type
type_check Bool.true -- Bool
type_check nat_lit 1 -- Nat
type_check (1 : Nat) -- Nat
type_check (True : _) -- Prop
type_check ∀ x y : Nat, x = y -- Prop
type_check fun x : Nat => 2 * x + 1 -- Nat -> Nat
trivial

Copy link
Member

Choose a reason for hiding this comment

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

This needs some type check error tests as well, using fail_if_success.

Copy link
Contributor

Choose a reason for hiding this comment

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

Marking as unresolved, as we still need to error tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand, can you explain in more detail what we need?

Copy link
Member

Choose a reason for hiding this comment

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

e.g. fail_if_success type_check 2 + "hi"

Copy link
Contributor

Choose a reason for hiding this comment

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

#334 has an alternative expect_failure tactic which should give you what you want here. We'll try to merge that soon?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh dear, type_check 2 + "hi" fails correctly with failed to synthesize instance HAdd Nat String ?m.160, but expect_failure type_check 2 + "hi" (after merging master into this PR) claims tactic sequence succeeded.

@digama0 or @gebner, could you suggest how we should be testing this?

Copy link
Member

@digama0 digama0 Jul 25, 2022

Choose a reason for hiding this comment

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

This is a bug in fail_if_success/success_if_fail/expect_failure (the synonyms keep piling up...), not type_check. As mentioned in the zulip thread, these tactics need to be wrapped in withoutRecover (or possibly we need a ! variant of the tactic that uses this).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@digama0 expect_failure already uses withoutRecover

Copy link
Member

Choose a reason for hiding this comment

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

could you suggest how we should be testing this?

I'm not 100% sure what type_check is supposed to do, but I guess it's supposed to find bugs in proofs produced by tactics. So I would suggest to test a tactic or elaborator that produces a term which does not type check. Such as this one:

/-- A term where `inferType` returns `Prop`, but which does not type check. -/
elab "wrong" : term =>
  return Lean.mkApp2 (.const ``id [.zero]) (.sort .zero) (.app (.sort .zero) (.sort .zero))

I have committed this elaborator and a failing test case.

Co-authored-by: Mario Carneiro <di.gama@gmail.com>
@semorrison semorrison added the awaiting-author A reviewer has asked the author a question or requested changes label Jul 19, 2022
Co-authored-by: Scott Morrison <scott@tqft.net>
@semorrison semorrison added blocked-by-other-PR This PR depends on another PR which is still in the queue. awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes blocked-by-other-PR This PR depends on another PR which is still in the queue. labels Jul 20, 2022
@gebner gebner added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Aug 24, 2022
@j-loreaux j-loreaux added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Sep 8, 2022
Mathlib/Tactic/TypeCheck.lean Outdated Show resolved Hide resolved
test/TypeCheck.lean Outdated Show resolved Hide resolved
@gebner
Copy link
Member

gebner commented Sep 8, 2022

Thank!

bors r+

bors bot pushed a commit that referenced this pull request Sep 8, 2022
@bors
Copy link

bors bot commented Sep 8, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(Mathlib/Tactic/TypeCheck): add type_check tactic [Merged by Bors] - feat(Mathlib/Tactic/TypeCheck): add type_check tactic Sep 8, 2022
@bors bors bot closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review The author would like community review of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants