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: remove the coercion from String to Name #3589

Merged
merged 9 commits into from
Mar 22, 2024

Conversation

david-christiansen
Copy link
Contributor

This coercion caused difficult-to-diagnose bugs sometimes. Because there are some situations where converting a string to a name should be done by parsing the string, and others where it should not, an explicit choice seems better here.

@david-christiansen
Copy link
Contributor Author

This coercion has led to a number of subtle bugs and frustrating situations for me in the past. It seems to me that coercions make sense when there's a single canonical mapping from one type to another, where the target of the coercion is in some sense intuitively "the same". But in this case, there are two mappings, both of which make sense in different situations, and if I forget to use the correct one, I'd rather get a type error to remind me than a program that silently computes the wrong answer.

@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 Mar 4, 2024
@leanprover-community-mathlib4-bot
Copy link
Collaborator

leanprover-community-mathlib4-bot commented Mar 4, 2024

Mathlib CI status (docs):

  • ❗ Std CI can not be attempted yet, as the nightly-testing-2024-03-04 tag does not exist there yet. We will retry when you push more commits. If you rebase your branch onto nightly-with-mathlib, Std CI should run now. (2024-03-04 10:15:42)
  • 🟡 Mathlib branch lean-pr-testing-3589 build this PR didn't complete normally. (2024-03-04 11:09:18) View Log
  • 🟡 Mathlib branch lean-pr-testing-3589 build this PR didn't complete normally. (2024-03-11 12:53:41) View Log
  • 🟡 Mathlib branch lean-pr-testing-3589 build this PR didn't complete normally. (2024-03-18 12:45:21) View Log
  • 🟡 Mathlib branch lean-pr-testing-3589 build against this PR was cancelled. (2024-03-21 01:10:29) View Log
  • 🟡 Mathlib branch lean-pr-testing-3589 build this PR didn't complete normally. (2024-03-21 02:08:05) View Log

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Mar 4, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Mar 4, 2024
Copy link
Member

@tydeu tydeu left a comment

Choose a reason for hiding this comment

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

I added a bunch of stylistic suggestions for the changed code along with some tweaks for the Lake fixes.

src/Lean/Elab/App.lean Outdated Show resolved Hide resolved
src/Lean/Elab/Syntax.lean Outdated Show resolved Hide resolved
src/lake/Lake/Load/Materialize.lean Outdated Show resolved Hide resolved
src/Lean/Hygiene.lean Outdated Show resolved Hide resolved
src/Lean/Meta/IndPredBelow.lean Outdated Show resolved Hide resolved
src/Lean/Compiler/IR/EmitLLVM.lean Outdated Show resolved Hide resolved
src/Lean/Parser.lean Outdated Show resolved Hide resolved
src/Lean/Parser.lean Outdated Show resolved Hide resolved
src/Lean/Parser.lean Outdated Show resolved Hide resolved
src/Lean/Parser/Extra.lean Outdated Show resolved Hide resolved
@kim-em kim-em added the full-ci label Mar 5, 2024
@kim-em
Copy link
Collaborator

kim-em commented Mar 5, 2024

@david-christiansen, if you'd be able to rebase onto nightly-with-mathlib, that would be great. We have a recent compatible Mathlib build, and so I would like to see the scope of the downstream fallout (not necessarily immediately dealing with!) before we merge.

david-christiansen and others added 6 commits March 8, 2024 21:51
This coercion caused difficult-to-diagnose bugs sometimes. Because
there are some situations where converting a string to a name should
be done by parsing the string, and others where it should not, an
explicit choice seems better here.
@david-christiansen
Copy link
Contributor Author

OK, rebased on nightly-with-mathlib

@david-christiansen
Copy link
Contributor Author

Thanks for the great feedback, @tydeu!

@tydeu
Copy link
Member

tydeu commented Mar 11, 2024

@david-christiansen The speedcenter is failing because the Lake benchmark is using this coercion in its lakefile:

lean_lib Inundation where
srcDir := "test"
roots := #[test]

This can be changed to roots := #[.mkSimple test].

EDIT: Applied it myself to hopefully save you some time. 😄

@david-christiansen
Copy link
Contributor Author

Applied it myself to hopefully save you some time. 😄

What a wonderful surprise, as I show up to do it! Thank you!

leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Mar 11, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Mar 11, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request Mar 18, 2024
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request Mar 18, 2024
@kim-em kim-em added this pull request to the merge queue Mar 21, 2024
Merged via the queue into leanprover:master with commit 966fa80 Mar 22, 2024
20 checks passed
@david-christiansen david-christiansen deleted the coe-String-Name branch April 2, 2024 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants