Setting subscript index of cstring is allowed and generates bad code

[metagn]

Low priority, disallowing this makes more sense than making it work.

Example

var x = cstring"abcd"
x[0] = 'x'

Current Output

x_43003[0].charCodeAt(0) = 97;
^

ReferenceError: Invalid left-hand side in assignment

Expected Output

compile error

Possible Solution

• undefine []= for cstring in JS. Or leave it alone and let people get the error on their own. relevant definition:

Nim/lib/system.nim

Lines 437 to 438 in dc3919b

	proc `[]=`*[I: Ordinal;T,S](a: T; i: I;
	x: sink S) {.noSideEffect, magic: "ArrPut".}

$ nim -v
Nim Compiler Version 1.3.1

[ghost]

Just FYI - this is not specific to the JS backend :)

[ringabout]

bad code gen at both C and JS banckend.
Works in VM at both C and JS banckend.

I think we should disable it in code gen phase

[timotheecour]

For c, see another issue (search sigbus), it leads to bad code only in specific cases so disabling it for c in all cases is not good. It's a complicated issue, but theres a way to solve it.

[ringabout]

Could you give me an example?

cstring is char * in c backend, it seems not to be modified?
https://stackoverflow.com/questions/1011455/is-it-possible-to-modify-a-string-of-char-in-c

At least it fails in the simple example above.

Edited:
foud the sigbus issue and link it:
#8463

[ringabout]

Anyway, we should document this situation. Whether this is already documented?

[ringabout]

Let's suppose that c part won't be fixed like #8463 said or we could document c part. Because c part is only specific to cstring literals modification.

This works in c backend not in JS backend.

var x = "123456"
var s: cstring = x
s[0] = 'u'

I will label this issue as JS.

[timotheecour]

for js it should be statically disallowed, that's simple.

for c, there are 2 solutions:

• the same analysis as done escape analysis: enables memory safe views (cf 1st class openarray) and optimizations #14976 would work, to distinguish whether the cstring originates from ROM or from a regular location.
• alternatively, we'd need a way to encode that some memory is immutable in the typesystem (see [WIP] [RFC] Nim should have immutable type qualifier (let is more like C++'s const); it would enable a new string class design, etc #8370)

[Araq]

There should be no []= accessor for cstring at all.

[ringabout]

I try to undefine []= for cstring, but it doesn't work. I'm not sure it caused by magic.

proc `[]=`*[I: Ordinal;T: not cstring,S](a: T; i: I; 
   x: sink S) {.noSideEffect, magic: "ArrPut".} 

Overload [] doesn't work too

import system except `[]=`


proc `[]=`(x: cstring, index: int, c: string) =
  echo "Hui"

var wind = cstring"abcd"
wind[0] = "1234"

[timotheecour]

There should be no []= accessor for cstring at all.

that'd be a large breaking change:

proc main()=
  var a = "foo"
  var b: cstring = a # or getting `b` from C API that returns a `char*` (not a `const char*`)
  b[0] = 'F'
  echo a
main()

fusion/pointers can add an overload

  proc toUncheckedArray*(a: cstring): ptr UncheckedArray[char] {.inline.} =
    cast[ptr UncheckedArray[char]](a)

to mitigate (and keep this 0 cost) but still.

note

even if we add this toUncheckedArray overload (which seems useful regardless), cstring.[]= can be made safer than UncheckedArray.[]= using a new cstringChecks as follows:

proc `[]=`(a: cstring, b: index) =
  when compileOption("cstringChecks):
    doAssert b < a.len
  #rest of code

ditto with [].

Whereas this wouldn't be possible with UncheckedArray, which shouldn't be concerned with 0-terminated C strings.

So cstring.[]= is more typesafe than UncheckedArray[char].[]=

[timotheecour]

@xflywind IMO #15877 is not the right fix not sufficient, see here:

proc main()=
  var a = "abc".cstring
  template fn() =
    a[0] = 'x'
  # fn() # ok: correctly gives: Error: cstring doesn't support `[]=` operator
  doAssert not compiles(fn()) # bug: assert fails
main()

nim r -b:js main fails (eef2948 1.5.1)

IMO the right fix should happen "earlier", we need to look deeper into #15877 (comment)

see also: #15911

[ringabout]

It seems to compile

static:
  var a = "abc".cstring
  template fn() =
    a[0] = 'x'
  fn()

[timotheecour]

[]= should be legal in VM+js but not in RT+js; it's a similar problem as the cross compiling issue in #16702 (comment) .

I don't know if this is solvable or not TBH.

proc main1() =
  var a = "abc"
  var b = a.cstring
  b[0] = 'x'
  template fn() = b[0] = 'x'
  doAssert compiles(fn())

proc main2()=
  var a = "abc"
  var b = a.cstring
  # b[0] = 'x' # correctly gives: Error: cstring doesn't support `[]=` operator
  doAssert not compiles(fn()) # should succeed, but right now fails
  when compiles(fn()):
    echo "foo" # should this print or not?

static: main1()
main2()

[ringabout]

bad code gen at both C and JS banckend.
Works in VM at both C and JS banckend.

I think we should disable it in code gen phase

My PR just fix the JS code gen.

[timotheecour]

I'm thinking of the following general approach to solve this kind of problem (refs timotheecour#564):

when defined(js):
  proc `[]=`*(a: cstring, b: int, c: char) {.nimvm.} = ... 
else:
  proc `[]=`*(a: cstring, b: int, c: char) = ... 

in this example, you'd have this behavior:

• in c backend, []= would be defined (for rt or vm)
• in js backend, []= would be defined if we're in vm, but not if we're in js

there are other use cases of {.nimvm.} which adds some notion of namespacing to distinguish whether we're running in vm or rt.