Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

feat(lint): add priority test for instances that always apply#1625

Merged
mergify[bot] merged 8 commits intomasterfrom
new_lint
Nov 1, 2019
Merged

feat(lint): add priority test for instances that always apply#1625
mergify[bot] merged 8 commits intomasterfrom
new_lint

Conversation

@fpvandoorn
Copy link
Member

@fpvandoorn fpvandoorn commented Oct 28, 2019

  • Built on top of feat(tactic/lint): silent linting #1580
  • The main new feature is a new linter: it checks that instances that always apply have priority below 1000 (default). See deterministic timeout in typeclass search for has_scalar ℝ (ℝ → ℝ) #1561. This is mainly useful for performance: it is more efficient to first apply the structural instances (that match on the type), and afterwards the forgetful instances (that always apply and change the class).
    • I moved two declarations from coinductive_predicates.lean to expr.lean, and everything about binder (in expr.lean) above the section on expr.
  • There are some more minor changes, see commit messages.

@fpvandoorn fpvandoorn added awaiting-review The author would like community review of the PR blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. labels Oct 28, 2019
also move a defn from coinductive_predicates to expr
also slightly refactor incorrect_def_lemma
Now they are run in the order specified by the doc
even when they are slow and  is false
@fpvandoorn fpvandoorn removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Oct 28, 2019
remove coinductive_predicates as import from some (but not all) files
@robertylewis
Copy link
Member

Cool, this seems really useful!

Is there a reason for the default linter priorities? Or is it just the order they appear in the file? If there's a reason to order them a certain way, we should document it, but I suspect it's just arbitrary (which is fine).

Copy link
Member

@robertylewis robertylewis left a comment

Choose a reason for hiding this comment

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

Only very very minor stylistic things.

@robertylewis robertylewis 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 Oct 29, 2019
@fpvandoorn
Copy link
Member Author

Is there a reason for the default linter priorities? Or is it just the order they appear in the file? If there's a reason to order them a certain way, we should document it, but I suspect it's just arbitrary (which is fine).

No, I currently just use the order it was in before and the order of the doc. (The place of this new linter is pretty arbitrary.)

@fpvandoorn fpvandoorn 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 Oct 29, 2019
@robertylewis robertylewis added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Oct 31, 2019
@mergify mergify bot merged commit 1710fd8 into master Nov 1, 2019
@mergify mergify bot deleted the new_lint branch November 1, 2019 03:25
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 15, 2020
…over-community#1625)

* feat(lint): add priority test for instances that always apply

also move a defn from coinductive_predicates to expr
also slightly refactor incorrect_def_lemma

* update doc

* add priorities to linters

Now they are run in the order specified by the doc

* always run tests in the extra set

even when they are slow and  is false

* move some more declarations from coinductive_predicates to expr

remove coinductive_predicates as import from some (but not all) files

* reviewer comments

* remove unsafe prefixes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants