Skip to content

Conversation

@plp127
Copy link
Contributor

@plp127 plp127 commented Jun 12, 2025

This PR fixes a bug where the single-quote character Char.ofNat 39 would delaborate as ''', which causes a parse error if pasted back in to the source code.

@plp127 plp127 requested a review from kim-em as a code owner June 12, 2025 14:20
@nomeata nomeata changed the title fix: fix char delay fix: delaboration of ''' Jun 12, 2025
@nomeata nomeata changed the title fix: delaboration of ''' fix: quoting single quote Char (''') Jun 12, 2025
nomeata
nomeata previously approved these changes Jun 12, 2025
Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll let someone from the library team press the button.

@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 Jun 12, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented Jun 12, 2025

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 0002ea8a3702a0c2191b03e1274aefc05b53ca8c --onto 0a9c24649767dea857a388e5385b7ae94bfd0185. You can force Mathlib CI using the force-mathlib-ci label. (2025-06-12 15:39:59)
  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 0002ea8a3702a0c2191b03e1274aefc05b53ca8c --onto faffe863343211d03d07d4ef19d90a2aa9d6fc8c. You can force Mathlib CI using the force-mathlib-ci label. (2025-06-13 15:51:31)

@Rob23oba
Copy link
Contributor

I feel like single quotes within double quotes or the other way round shouldn't need to escaped. Also (unrelated) the way it currently works is that it allocates a new string for every character when trying to quote, maybe we could instead use a function String.pushQuotedChar (s : String) (c : Char) (outerQuote : Char).

@Rob23oba
Copy link
Contributor

Rob23oba commented Jun 12, 2025

So something like

def String.pushQuotedChar (s : String) (c : Char) (quote : Char := '"') : String :=
  if      c = '\n' then s.push '\\' |>.push 'n'
  else if c = '\t' then s.push '\\' |>.push 't'
  else if c = '\\' ∨ c = quote then s.push '\\' |>.push c
  else if c.toNat <= 31 ∨ c = '\x7f' then smallCharToHex s c
  else s.push c
where
  smallCharToHex (s : String) (c : Char) : String :=
    let n  := Char.toNat c
    let d2 := n / 16
    let d1 := n % 16
    s.push '\\' |>.push 'x' |>.push (Nat.digitChar d2) |>.push (Nat.digitChar d1)

@nomeata nomeata dismissed their stale review June 12, 2025 16:46

As Rob says, the change to string literals is probably not great.

Co-authored-by: Kyle Miller <[email protected]>
@nomeata nomeata added the changelog-library Library label Jun 13, 2025
@nomeata nomeata enabled auto-merge June 13, 2025 15:25
@nomeata nomeata added this pull request to the merge queue Jun 13, 2025
Merged via the queue into leanprover:master with commit cceabbb Jun 13, 2025
12 checks passed
@plp127 plp127 deleted the aliu/char-bug branch June 13, 2025 16:24
algebraic-dev pushed a commit to algebraic-dev/lean4 that referenced this pull request Jun 18, 2025
This PR fixes a bug where the single-quote character `Char.ofNat 39`
would delaborate as `'''`, which causes a parse error if pasted back in
to the source code.

---------

Co-authored-by: Kyle Miller <[email protected]>
wkrozowski pushed a commit to wkrozowski/lean4 that referenced this pull request Jun 24, 2025
This PR fixes a bug where the single-quote character `Char.ofNat 39`
would delaborate as `'''`, which causes a parse error if pasted back in
to the source code.

---------

Co-authored-by: Kyle Miller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-library Library 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.

5 participants