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: apply’s error message should show implicit arguments as needed #3929

Merged
merged 28 commits into from
May 18, 2024

Conversation

nomeata
Copy link
Contributor

@nomeata nomeata commented Apr 16, 2024

luckily the necessary functionality already exists in the form of
addPPExplicitToExposeDiff. But it is not cheap, and we should not run this code
when the error message isn’t shown, so we should do this lazily.

We already had MessageData.ofPPFormat to assemble the error message lazily, but it
was restricted to returning FormatWithInfo, a data type that doesn’t admit a nice
API to compose more complex messages (like Format or MessageData has; an attempt to
fix that is in #3926).

Therefore we split the functionality of .ofPPFormat into .ofFormatWithInfo and .ofLazy,
and use .ofLazy to compute the more complex error message of apply.

Fixes #3232.

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc April 16, 2024 17:55 Inactive
@nomeata nomeata marked this pull request as ready for review April 16, 2024 17:57
@github-actions github-actions bot added the toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN label Apr 16, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Apr 16, 2024

Mathlib CI status (docs):

  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 75e68e7565708361e117c5f56b66fdbe013de667 --onto 1c20b5341956fb77fcf2c2601e64075e51d5f858. (2024-04-16 18:01:06)
  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 75e68e7565708361e117c5f56b66fdbe013de667 --onto d3e004932c1f5ac3946850692940512d381c7634. (2024-04-18 20:36:23)
  • ❗ Std/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 22ce2fea9b93b53a338114dde67c27e881aa1b24 --onto 62cdb51ed5b9d8487877d5a4adbcd4659d81fc6a. (2024-04-22 12:31:05)
  • ✅ Mathlib branch lean-pr-testing-3929 has successfully built against this PR. (2024-05-08 11:27:37) View Log
  • ✅ Mathlib branch lean-pr-testing-3929 has successfully built against this PR. (2024-05-08 12:09:06) View Log
  • 🟡 Mathlib branch lean-pr-testing-3929 build against this PR was cancelled. (2024-05-17 09:08:54) View Log
  • 💥 Mathlib branch lean-pr-testing-3929 build failed against this PR. (2024-05-17 09:14:04) View Log
  • 🟡 Mathlib branch lean-pr-testing-3929 build against this PR was cancelled. (2024-05-17 19:34:45) View Log
  • 🟡 Mathlib branch lean-pr-testing-3929 build this PR didn't complete normally. (2024-05-17 20:30:39) View Log

Co-authored-by: David Thrane Christiansen <[email protected]>
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 8, 2024
Copy link
Member

@Vtec234 Vtec234 left a comment

Choose a reason for hiding this comment

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

Hi! I have re-fetched the pretty-printing infrastructure into my mental cache and I think I agree that this is the right-est solution. Just to confirm though, why did you discard #3926? Is it because that still doesn't allow for lazily-produced MessageData, only FormatWithInfos, and furthermore introduced yet another appendable message type?

src/Lean/Message.lean Show resolved Hide resolved
src/Lean/Message.lean Outdated Show resolved Hide resolved
src/Lean/Message.lean Outdated Show resolved Hide resolved
Co-authored-by: Wojciech Nawrocki <[email protected]>
@nomeata
Copy link
Contributor Author

nomeata commented May 17, 2024

Hi! I have re-fetched the pretty-printing infrastructure into my mental cache and I think I agree that this is the right-est solution.

Thanks! It's good to have a second pair of eyes!

Just to confirm though, why did you discard #3926? Is it because that still doesn't allow for lazily-produced MessageData, only FormatWithInfos, and furthermore introduced yet another appendable message type?

Yes! The way FormarWithInfo works makes appending then rather tedious (shifting indices), and it's not just appending but also string interpolation that would have to be duplicated. It didn't seem desirable to have three similar data structures around.

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc May 17, 2024 08:31 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc May 17, 2024 09:08 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan and removed builds-mathlib CI has verified that Mathlib builds against this PR labels May 17, 2024
@Vtec234 Vtec234 requested a review from semorrison as a code owner May 17, 2024 19:16
@Vtec234
Copy link
Member

Vtec234 commented May 17, 2024

Now mathlib fails, and searching for the error message on the phone is annoying. But probably nothing worrying and we can merge?

I have made adaptations to batteries and to mathlib4, and written a changelog. If these build, I think we should be good.

But, it would still be good to benchmark as MessageData is used quite ubiquitously.

@Vtec234
Copy link
Member

Vtec234 commented May 17, 2024

!bench

@github-actions github-actions bot temporarily deployed to lean-lang.org/lean4/doc May 17, 2024 19:19 Inactive
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 17, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 17, 2024
@leanprover-bot
Copy link
Collaborator

Here are the benchmark results for commit 8316658.
There were no significant changes against commit 883a3e7.

@nomeata
Copy link
Contributor Author

nomeata commented May 17, 2024

Mathlib seems to be happy as well, thanks. Any last thoughts before I press the button?

@Vtec234
Copy link
Member

Vtec234 commented May 17, 2024

LGTM!

@nomeata nomeata added this pull request to the merge queue May 18, 2024
Merged via the queue into master with commit 9f6bbfa May 18, 2024
13 checks passed
@nomeata nomeata deleted the joachim/failed-to-unify3 branch May 18, 2024 09:35
@nomeata
Copy link
Contributor Author

nomeata commented May 18, 2024

Thanks for your help getting this over the finishing line.

We should probably now make the omega error lazy. I'm curious if that will show up in mathlib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks-mathlib This is not necessarily a blocker for merging: but there needs to be a plan toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

“failed to unify” message should not print identical types
5 participants