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] - chore: another non-class instance #7250

Closed
wants to merge 6 commits into from

Conversation

alexjbest
Copy link
Member

@alexjbest alexjbest commented Sep 19, 2023

Following #7245, https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/isClass.3F.20panic!/near/391779504.

There is one exception that it is unclear how to fix, it seems lean makes the internal declarations in a block of mutual instances also instances perhaps? Seeing as it is internal I hope it won't cause too much trouble


Open in Gitpod

@alexjbest alexjbest added WIP Work in progress t-meta Tactics, attributes or user commands awaiting-CI labels Sep 19, 2023
@alexjbest alexjbest added awaiting-review The author would like community review of the PR and removed awaiting-CI WIP Work in progress labels Sep 19, 2023
@Vierkantor Vierkantor self-assigned this Sep 19, 2023
Copy link
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Is there a reason for putting this linter in Mathlib instead of Std? I think it should be applicable to any Lean project.

@alexjbest
Copy link
Member Author

Is there a reason for putting this linter in Mathlib instead of Std? I think it should be applicable to any Lean project.

Lazyness ;) its much easier to test vs mathlib by simply PRing to mathlib.

I'm not sure where the line is here though, Mathlib/Tactic/Lint.lean already contains one linter that I would also argue is widely applicable

Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@Vierkantor
Copy link
Contributor

There's certainly value in getting this linter in quickly, and I expect/hope that ultimately it will be obsoleted anyway because Lean itself makes this situation an error so there might not be enough gained in moving this to Std.

But to ensure such a move remains possible, how about the following: ensure no non-Std imports in the linters, by adding a #assert_not_exists command. Not sure what to assert against, maybe something from either Mathlib/Lean/Expr/Basic.lean or Mathlib/Tactic/Core.lean?

@alexjbest
Copy link
Member Author

There's certainly value in getting this linter in quickly, and I expect/hope that ultimately it will be obsoleted anyway because Lean itself makes this situation an error so there might not be enough gained in moving this to Std.

But to ensure such a move remains possible, how about the following: ensure no non-Std imports in the linters, by adding a #assert_not_exists command. Not sure what to assert against, maybe something from either Mathlib/Lean/Expr/Basic.lean or Mathlib/Tactic/Core.lean?

This seems way more effort than its worth, ill just pr this to std then I think :)

@alexjbest alexjbest closed this Sep 20, 2023
@alexjbest
Copy link
Member Author

Actually we may aswell have this PR be simply the have to haveI change now

@alexjbest alexjbest reopened this Sep 20, 2023
Mathlib/Tactic/Lint.lean Outdated Show resolved Hide resolved
@alexjbest alexjbest changed the title feat: a linter for non-class instances chore: another non-class intstance Sep 20, 2023
@alexjbest alexjbest changed the title chore: another non-class intstance chore: another non-class instance Sep 20, 2023
Copy link
Contributor

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

For the lazy, like me, std4 pr: leanprover-community/batteries#268 :)

bors r+

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the ready-to-merge This PR has been sent to bors. label Sep 20, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the awaiting-review The author would like community review of the PR label Sep 20, 2023
bors bot pushed a commit that referenced this pull request Sep 20, 2023
Following #7245, https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/isClass.3F.20panic!/near/391779504.

There is one exception that it is unclear how to fix, it seems lean makes the internal declarations in a block of mutual instances also instances perhaps? Seeing as it is internal I hope it won't cause too much trouble
@bors
Copy link

bors bot commented Sep 20, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title chore: another non-class instance [Merged by Bors] - chore: another non-class instance Sep 20, 2023
@bors bors bot closed this Sep 20, 2023
@bors bors bot deleted the alexjbest/nonclassinstancelint branch September 20, 2023 10:21
kodyvajjha pushed a commit that referenced this pull request Sep 22, 2023
Following #7245, https://leanprover.zulipchat.com/#narrow/stream/270676-lean4/topic/isClass.3F.20panic!/near/391779504.

There is one exception that it is unclear how to fix, it seems lean makes the internal declarations in a block of mutual instances also instances perhaps? Seeing as it is internal I hope it won't cause too much trouble
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants