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

chore: bump to nightly-2023-08-19 #48

Closed

Conversation

semorrison
Copy link
Contributor

nightly-2023-08-19 is now the first official release candidate, and I would like to see if we can get most major projects to try bumping to it.

In this PR, I've had to address the unused arguments linter in various places. Please feel free to adopt other solutions!

One warning from the linter does seem worrying: genTokens discards its l argument.

@semorrison
Copy link
Contributor Author

@insightmind would you be able to help me understand the (l : List Token) argument for genTokens in LeanInk/Annotation/Alectryon.lean? It is an unused argument, yet nontrivial values are passed to it, and I'm uncertain if it is a bug.

@insightmind
Copy link
Collaborator

Hey! I think you can remove the argument there, I can't exactly remember what it was for, but it no longer seems to be used anywhere within the function.

Additionally, judging from the commit/blame of this line: 42a98bb
it seems this was an argument that was used in a previous iteration of this function, which we likely forgot to remove when we simplified it. It also seems that it was previously used as a running list of Tokens due the recursive nature of the function back then, hence it's usually initialized with an empty list.

I'm pretty sure its safe to remove the argument.

* Turn on warningAsError in CI
* Remove unused arguments flagged by the linter
* Note that the unused `l : List Token` argument for `genTokens` was suspicious,
  as nontrivial values were passed to it.
  Please see leanprover#48 (comment)
  for confirmation that removing it is correct.
@semorrison
Copy link
Contributor Author

semorrison commented Aug 23, 2023

@insightmind, all the tests were failing, and I'm not certain to what extent it is a problem. Would you be able to have a look at the diff in the last commit (in which I just copied *.leanInk over *.leanInk.expected, thereby "fixing" the tests)?

I'll mark this as a draft so there's no chance we merge with this commit in place, in case it is wrong!

@semorrison semorrison marked this pull request as draft August 23, 2023 02:27
"semanticType": null,
"raw": " + 1 => ",
"raw": "example : Nat → Nat\n | 0 => 0\n | n + 1 => ",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like lots of typeinfo is missing, but I haven't followed recent changes to repo. So it might be that something regressed or this is happening because of changes in the compiler. Hard to tell. You might have a look at recent PRs to see when they started failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted the wrong arguments when cleaning up the unused arguments, oops!

This is done properly now on #53

Unfortunately there's still a problem bumping the tool-chain. I'm going to close this PR and do it in smaller steps.

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

2 participants