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

feat(tactic/lint): support @[nolint unused_arguments] #2041

Merged
merged 5 commits into from Feb 24, 2020
Merged

Conversation

gebner
Copy link
Member

@gebner gebner commented Feb 22, 2020

We have so many linters now and @[nolint] disables all of them. This PR adds parameters to the nolint attribute so that you can selectively disable some lints only:

@[nolint def_lemma] -- TODO: This should be a theorem but Lean fails to synthesize the placeholder
def div_def (a) {b : ordinal} (h : b ≠ 0) :

TO CONTRIBUTORS:

Make sure you have:

  • reviewed and applied the coding style: coding, naming
  • reviewed and applied the documentation requirements
  • for tactics:
  • make sure definitions and lemmas are put in the right files
  • make sure definitions and lemmas are not redundant

If this PR is related to a discussion on Zulip, please include a link in the discussion.

For reviewers: code review check list

@bryangingechen
Copy link
Collaborator

Could you update docs/commands.md?

@jcommelin
Copy link
Member

Should it even be the default that nolint disables nothing, and the linters that are to be disabled MUST be listed explicitly?

@urkud
Copy link
Member

urkud commented Feb 22, 2020

Should it even be the default that nolint disables nothing, and the linters that are to be disabled MUST be listed explicitly?

This behavior is not the "least surprising". If we want to force users to specify disabled linters explicitly, then @[nolint] with no parameters should fail.

@gebner gebner added the WIP Work in progress label Feb 22, 2020
@gebner
Copy link
Member Author

gebner commented Feb 22, 2020

I think I introduced a performance issue when refactoring the linting code. The linters now take 30 minutes instead of 12. I'm debugging it now.

Could you update docs/commands.md?

Done.

[...] the linters that are to be disabled MUST be listed explicitly?
[...] then @[nolint] with no parameters should fail.

I agree. The only reason it did not fail was because of the nolints.txt file, which added nolint without specifying the linter. I have now updated the mk_nolints.lean file, and I'll then make @[nolints] an error.

@gebner gebner removed the WIP Work in progress label Feb 22, 2020
@gebner
Copy link
Member Author

gebner commented Feb 23, 2020

I think I introduced a performance issue when refactoring the linting code.

The performance issue was due to using user attributes with parameters. I'm using the obvious fix now:

/--
Computes the declaration name used to store the nolint attribute data.

Retrieving the parameters for user attributes is *extremely* slow.
Hence we store the parameters of the nolint attribute as declarations
in the environment.  E.g. for `@[nolint foo] def bar := _` we add the
following declaration:

meta def bar._nolint.foo : unit := ()
-/
private meta def mk_nolint_decl_name (decl : name) (linter : name) : name :=
(decl ++ nolint_infix) ++ linter

As requested, @[nolint] is now an error. You always need to specify the linters you want to disable. I've also refactored the mk_nolint.lean script; the nolints.txt file is now grouped by filename. Maybe this makes it more attractive for people to just pick a file and fix all the linting errors there?

-- data/dfinsupp.lean
apply_nolint decidable_zero_symm doc_blame
apply_nolint dfinsupp doc_blame
apply_nolint dfinsupp.decidable_eq unused_arguments
apply_nolint dfinsupp.erase doc_blame
apply_nolint dfinsupp.lmk doc_blame
apply_nolint dfinsupp.lsingle doc_blame
apply_nolint dfinsupp.map_range_def unused_arguments
apply_nolint dfinsupp.map_range_single unused_arguments
apply_nolint dfinsupp.mk doc_blame
apply_nolint dfinsupp.pre doc_blame has_inhabited_instance
apply_nolint dfinsupp.single doc_blame
apply_nolint dfinsupp.subtype_domain_sum unused_arguments
apply_nolint dfinsupp.sum_apply unused_arguments
apply_nolint dfinsupp.support doc_blame
apply_nolint dfinsupp.to_has_scalar doc_blame
apply_nolint dfinsupp.to_module doc_blame
apply_nolint dfinsupp.zip_with doc_blame
apply_nolint dfinsupp.zip_with_def unused_arguments

-- data/dlist/basic.lean
apply_nolint dlist.join doc_blame

@gebner gebner added the awaiting-review The author would like community review of the PR label Feb 23, 2020
@@ -42,7 +42,8 @@ def id.mk {α : Sort u} : α → id α := id

namespace functor

@[nolint] def const (α : Type*) (β : Type*) := α
@[nolint unused_arguments doc_blame]
Copy link
Member

Choose a reason for hiding this comment

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

@[nolint doc_blame] is saying this def is permanently allowed to not have a doc string. Could you either add a doc string, or add it to nolints.txt (temporary permission to not have a doc string)? Same for map below. The examples in ordinal are a little different since morally they're lemmas, not defs.

I'd suggest a doc string but honestly I'm not sure what this is for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I've added a docstring for const, but the rest of the file is a bit of a mystery to me as well. I've got no idea what add_const is for.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @cipher1024 could help here.

@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 Feb 24, 2020
@mergify mergify bot merged commit 0fc45dc into master Feb 24, 2020
@mergify mergify bot deleted the nolint-param branch February 24, 2020 15:43
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…munity#2041)

* feat(tactic/lint): support @[nolint unused_arguments]

* refactor(scripts/mk_nolint): include failing linter name in nolints.txt

* chore(scripts/nolints): update nolints.txt

* doc(category/functor): add docstrings

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 16, 2020
…munity#2041)

* feat(tactic/lint): support @[nolint unused_arguments]

* refactor(scripts/mk_nolint): include failing linter name in nolints.txt

* chore(scripts/nolints): update nolints.txt

* doc(category/functor): add docstrings

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

6 participants