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(tactic/lint): add linter that type-checks statements #7694

Closed
wants to merge 4 commits into from

Conversation

fpvandoorn
Copy link
Member

@fpvandoorn fpvandoorn commented May 22, 2021

  • Add linter that type-checks the statements of all declarations with the default reducibility settings. A statement might not type-check if locally a declaration was made not @[irreducible] while globally it is.
  • Fix an issue where with_one.monoid.to_mul_one_class did not unify with with_one.mul_one_class.
  • Fix some proofs in category_theory.opposites so that they work while keeping quiver.opposite irreducible.

Zulip

Open in Gitpod

@fpvandoorn fpvandoorn added the WIP Work in progress label May 22, 2021
@sgouezel
Copy link
Collaborator

I thought that making things irreducible after the fact was now forbidden because it is not supported by Lean 4. Are the issues in this PR only due to examples in mathlib that do not conform yet to this rule? If this is the case, a better fix would be to make them irreducible from the start and adjust the proofs, no ?

@fpvandoorn
Copy link
Member Author

fpvandoorn commented May 22, 2021

Yes, all these examples are problems caused by locally making definitions not @[irreducible] (or not yet having them made @[irreducible]). The library is making some types @[irreducible] at the end of the file where it is defined, while keeping it @[semireducible] to develop some basic definitions/results about them.

Making all definitions immediately irreducible is a little painful, because that means that e.g. the option API is not available for with_zero/with_one.

Here is a gist of all automatically generated declarations that are ill-typed. That should catch all definitions (but not all lemmas) that only work because some other declaration is locally not irreducible.

@fpvandoorn fpvandoorn added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels May 27, 2021
@[linter]
meta def linter.check_type : linter :=
{ test := check_type,
auto_decls := ff,
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
auto_decls := ff,
auto_decls := tt,

Constructors are also auto decls and should be checked as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do that, we cannot define structure on a new irreducible type by using the corresponding structure on the old type.
In particular: all of the following instances have equational lemmas that are ill-typed once with_one is irreducible.

@[to_additive] instance : monad with_one := option.monad
@[to_additive] instance : has_one (with_one α) := ⟨none⟩
@[to_additive] instance [has_mul α] : has_mul (with_one α) := ⟨option.lift_or_get (*)⟩
@[to_additive] instance : inhabited (with_one α) := ⟨1@[to_additive] instance [nonempty α] : nontrivial (with_one α) := option.nontrivial
@[to_additive] instance : has_coe_t α (with_one α) := ⟨some⟩

More failures are in this Gist: https://gist.github.com/fpvandoorn/015ce16dbd42f3ec33f26f44ef031c3a

Copy link
Member

Choose a reason for hiding this comment

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

All of the errors here are equation lemmas and abstracted proofs terms. (And linarith.config.mk.inj* which is weird, but also a meta definition.)

It's unfortunate that auto_decls covers so much. My preference would be to set auto_decls := tt for the linter and exclude abstracted proofs and equation lemmas manually. But I'm also happy if you merge the linter as it is now before it sits another week.

bors d+

Copy link
Member Author

Choose a reason for hiding this comment

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

That is a good idea. I might do that in a follow-up PR.

bors merge

@bors
Copy link

bors bot commented Jun 2, 2021

✌️ fpvandoorn can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

bors bot pushed a commit that referenced this pull request Jun 2, 2021
* Add linter that type-checks the statements of all declarations with the default reducibility settings. A statement might not type-check if locally a declaration was made not `@[irreducible]` while globally it is.
* Fix an issue where  `with_one.monoid.to_mul_one_class` did not unify with `with_one.mul_one_class`.
* Fix some proofs in `category_theory.opposites` so that they work while keeping `quiver.opposite` irreducible.
@bors
Copy link

bors bot commented Jun 2, 2021

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Jun 2, 2021
* Add linter that type-checks the statements of all declarations with the default reducibility settings. A statement might not type-check if locally a declaration was made not `@[irreducible]` while globally it is.
* Fix an issue where  `with_one.monoid.to_mul_one_class` did not unify with `with_one.mul_one_class`.
* Fix some proofs in `category_theory.opposites` so that they work while keeping `quiver.opposite` irreducible.
@bors
Copy link

bors bot commented Jun 3, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(tactic/lint): add linter that type-checks statements [Merged by Bors] - feat(tactic/lint): add linter that type-checks statements Jun 3, 2021
@bors bors bot closed this Jun 3, 2021
@bors bors bot deleted the check-type branch June 3, 2021 04:58
@YaelDillies YaelDillies removed the awaiting-review The author would like community review of the PR label Nov 17, 2021
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

4 participants