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: to_additive adds instances #678

Closed
wants to merge 10 commits into from

Conversation

mcdoll
Copy link
Member

@mcdoll mcdoll commented Nov 22, 2022

No description provided.

@mcdoll mcdoll added the WIP Work in progress label Nov 22, 2022
/-- foo -/
def copyInstanceAttribute (src tgt : Name) : CoreM Unit := do
if (← isInstance src) then
addInstance tgt AttributeKind.global 1000 |>.run'
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would also copy the priority, can we add a todo note here for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is true and it is not totally trivial, because there are some nice features instances still missing in Core. But I will have a look at that later.

@alexjbest
Copy link
Member

Oops I still this is still WIP, so sorry if i just commented something that was already on your mind

@mcdoll
Copy link
Member Author

mcdoll commented Nov 23, 2022

It does not work yet and I don't really know why. I somewhat suspect that the instances are registered after the monad in to_additive is run.

@mcdoll mcdoll added awaiting-review and removed WIP Work in progress labels Nov 23, 2022
@mcdoll
Copy link
Member Author

mcdoll commented Nov 23, 2022

@alexjbest now your complaints are valid. I've tried and did not make any meaningful progress and I asked on zulip.

@alexjbest
Copy link
Member

Yes, I've also tried this and agree there is missing API (this is why I mentioned adding a TODO rather than implementing it here).
I'll also continue trying to see if I can understand / extract the right api functions from lean.

I think its probably preferable to merge this for now and improve it later.

@semorrison
Copy link
Contributor

Do we need to add some links in the source code to zulip discussions and/or Lean 4 issues asking for this extra API? It would be nice to leave some breadcrumbs here.

@mcdoll
Copy link
Member Author

mcdoll commented Nov 23, 2022

Added an issue in Lean 4: leanprover/lean4#1878

@mcdoll mcdoll linked an issue Nov 23, 2022 that may be closed by this pull request
@semorrison
Copy link
Contributor

bors merge

bors bot pushed a commit that referenced this pull request Nov 24, 2022
@semorrison semorrison added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Nov 24, 2022
@bors
Copy link

bors bot commented Nov 24, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore: to_additive adds instances [Merged by Bors] - chore: to_additive adds instances Nov 24, 2022
@bors bors bot closed this Nov 24, 2022
@bors bors bot deleted the mcdoll/to_additive branch November 24, 2022 02:24
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to_additive doesn't seem to translate instance-ness
3 participants