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

patched_stdlib(parsejson): resolve CStringConv warnings #445

Merged
merged 1 commit into from Aug 20, 2022

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Oct 22, 2021

After the upgrade to Nim 1.6.0 in commit 761936b, a regular
nimble build would produce these warnings:

/foo/configlet/src/patched_stdlib/parsejson.nim(237, 37) Warning: implicit conversion to 'cstring' from a non-const location: my.buf; this will become a compile time error in the future [CStringConv]

/foo/configlet/src/patched_stdlib/parsejson.nim(247, 39) Warning: implicit conversion to 'cstring' from a non-const location: my.buf; this will become a compile time error in the future [CStringConv]

I authored a commit in the nim-lang/Nim repo that resolves the
most visible CStringConv warnings, including these in parsejson, by
making the cstring conversion explicit. Let's update configlet's copy of
parsejson without waiting for a Nim version bump, so that we no longer
see these CStringConv warnings while compiling configlet.

As a reminder, we maintain our own copy of std/json and std/parsejson in
the configlet repo so that we can forbid comments and trailing commas in
JSON (see 15c8403 for details).

As background, the Nim manual says that an implicit conversion to
cstring will eventually not be allowed:

A Nim string is implicitly convertible to cstring for convenience.

[...]

Even though the conversion is implicit, it is not safe: The garbage collector
does not consider a cstring to be a root and may collect the underlying
memory. For this reason, the implicit conversion will be removed in future
releases of the Nim compiler. Certain idioms like conversion of a const string
to cstring are safe and will remain to be allowed.

And from Nim 1.6.0, such a conversion triggers a warning:

A dangerous implicit conversion to cstring now triggers a [CStringConv] warning.
This warning will become an error in future versions! Use an explicit conversion
like cstring(x) in order to silence the warning.


To-do:

  • Resolve this upstream, and then I can update the hash in our parsejson.nim file.

@ErikSchierboom
Copy link
Member

@ee7 Can this be merged/updated? Those warnings are annoying :)

@ee7 ee7 force-pushed the patched-stdlib-CStringConv branch from 6cc0244 to 43a237c Compare May 7, 2022 14:54
@ee7
Copy link
Member Author

ee7 commented May 11, 2022

I'll try to finally finish nim-lang/Nim#19488 soon. I have a minor preference for merging this PR after that, so that we can either:

  1. Update the hash below and keep the diff from upstream as small as possible:

# This file is minimally adapted from this version in the Nim standard library:
# https://github.com/nim-lang/Nim/blob/c17baaefbcff/lib/pure/json.nim
# The standard library version is lenient: it silently allows a trailing comma.
# The below version is stricter: it raises a `JsonParsingError` for a trailing
# comma.

  1. or at least reference the upstream commit in our commit message, if we'll base our patched file on version-1-6 rather than Nim devel.

@ee7
Copy link
Member Author

ee7 commented Aug 18, 2022

I finally got around to finishing the upstream PR. So hopefully that'll be merged soon, and I'll finally merge this PR.

Sorry for the wait.

After the upgrade to Nim 1.6.0 in commit 761936b, a regular
`nimble build` would produce these warnings:

    /foo/configlet/src/patched_stdlib/parsejson.nim(237, 37) Warning:
    implicit conversion to 'cstring' from a non-const location: my.buf;
    this will become a compile time error in the future [CStringConv]

    /foo/configlet/src/patched_stdlib/parsejson.nim(247, 39) Warning:
    implicit conversion to 'cstring' from a non-const location: my.buf;
    this will become a compile time error in the future [CStringConv]

I authored a commit [1] in the `nim-lang/Nim` repo that resolves the
most visible CStringConv warnings, including these in parsejson, by
making the cstring conversion explicit. Let's update configlet's copy of
parsejson without waiting for a Nim version bump, so that we no longer
see these CStringConv warnings while compiling configlet.

As a reminder, we maintain our own copy of std/json and std/parsejson in
the configlet repo so that we can forbid comments and trailing commas in
JSON (see 15c8403 for details).

As background, the Nim manual says that an implicit conversion to
cstring will eventually not be allowed [2]:

    A Nim `string` is implicitly convertible to `cstring` for convenience.

    [...]

    Even though the conversion is implicit, it is not *safe*: The garbage collector
    does not consider a `cstring` to be a root and may collect the underlying
    memory. For this reason, the implicit conversion will be removed in future
    releases of the Nim compiler. Certain idioms like conversion of a `const` string
    to `cstring` are safe and will remain to be allowed.

And from Nim 1.6.0, such a conversion triggers a warning [3]:

    A dangerous implicit conversion to `cstring` now triggers a `[CStringConv]` warning.
    This warning will become an error in future versions! Use an explicit conversion
    like `cstring(x)` in order to silence the warning.

[1] nim-lang/Nim@e8657c710776
[2] https://github.com/nim-lang/Nim/blob/e8657c710776/doc/manual.md#cstring-type
[3] https://github.com/nim-lang/Nim/blob/e8657c710776/changelogs/changelog_1_6_0.md#type-system
@ee7 ee7 force-pushed the patched-stdlib-CStringConv branch from 43a237c to d9c3efe Compare August 20, 2022 10:01
@ee7 ee7 marked this pull request as ready for review August 20, 2022 10:12
@ee7 ee7 changed the title patched_stdlib: resolve CStringConv warnings patched_stdlib(parsejson): resolve CStringConv warnings Aug 20, 2022
@ee7 ee7 merged commit b558ee5 into exercism:main Aug 20, 2022
@ee7 ee7 deleted the patched-stdlib-CStringConv branch August 20, 2022 10:18
@ee7
Copy link
Member Author

ee7 commented Aug 20, 2022

I fixed these warnings in the repo for the Nim compiler, and rebased our patched parsejson on Nim devel.

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