Fix issue where iterating over an empty TSet or TOrderedSet results in a SIGSEVG

[santegoeds]

To reproduce bug:

import sets
var t: TSet[string]  # or TOrderedSet[string]
echo(t)

$ nimrod c -r b.nim
config/nimrod.cfg(37, 2) Hint: added path: '/home/tjerk/.babel/pkgs/' [Path]
Hint: used config file '/home/tjerk/apps/Nimrod/config/nimrod.cfg' [Conf]
Hint: system [Processing]
Hint: b [Processing]
Hint: sets [Processing]
Hint: os [Processing]
Hint: strutils [Processing]
Hint: parseutils [Processing]
Hint: times [Processing]
Hint: posix [Processing]
Hint: hashes [Processing]
Hint: math [Processing]
gcc -o /home/tjerk/dev/nimrod/bfair/b /home/tjerk/dev/nimrod/bfair/nimcache/pure_math.o /home/tjerk/dev/nimrod/bfair/nimcache/pure_hashes.o /home/tjerk/dev/nimrod/bfair/nimcache/posix_posix.o /home/tjerk/dev/nimrod/bfair/nimcache/pure_times.o /home/tjerk/dev/nimrod/bfair/nimcache/pure_parseutils.o /home/tjerk/dev/nimrod/bfair/nimcache/pure_strutils.o /home/tjerk/dev/nimrod/bfair/nimcache/pure_os.o /home/tjerk/dev/nimrod/bfair/nimcache/collections_sets.o /home/tjerk/dev/nimrod/bfair/nimcache/Nimrod_system.o /home/tjerk/dev/nimrod/bfair/nimcache/bfair_b.o -ldl -lm
Hint: operation successful (15733 lines compiled; 0.250 sec total; 22.225MB) [SuccessX]
/home/tjerk/dev/nimrod/bfair/b
Traceback (most recent call last)
b.nim(5) b
sets.nim(40) $
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Error: execution of an external program failed

[reactormonk]

Looks legit. Could you add some tests to tests/collections/tsets.nim?

[Araq]

Meh, I don't consider this a bug at all.

[santegoeds]

I think it improves robustness. Should I bother adding tests?

[dom96]

Sequences and strings would behave the same way. Why should sets be any different?

[santegoeds]

Mind you that I'm very new to Nimrod but I have been interpreting Sequences and strings as "internal" types with special behaviour (similar to some of the Go-lang internal types).

Because it's a TSet I expected it to be created on the stack and therefore in a valid state without explicit initialisation. If the type has been a PSet I would have looked at initialising it using either initSet or new(t).

[reactormonk]

@santegoeds has a point. The problem here is that TSet is indeed on the stack, but the seq containing the data is on the heap.

[dom96]

indeed. That's a valid point.

[santegoeds]

I've updated the pull request so operations on uninitialised TSets and TOrderedSets are supported and added more tests.

There is obviously an impact wrt runtime performance so I guess it's a policy decision whether it's suitable for inclusion.

[reactormonk]

Your goodwill is appreciated, but the problem lies deeper. How should nimrod deal with stack-allocated data structures that reference stuff on the heap? If we'd accept your pull request, we have to change all the code of all collections that have something allocated on the heap. Also, it's required that every new collection behaves like this with null values. Araq will decide on the course of action for the problem.

[santegoeds]

I completely understand; I only updated the pull request because it was a job half-done. Now that I understand the issue, this approach is probably the wrong way to fix it anyway.

To give you and Araq some context of why I thought this was a bug... I was writing code as below and it crashed when I tried to iterate through the uninitialised set.

import json
import sets

type
  TMarketFilter* = object 
    textQuery: string
    exchangeIds: TSet[string]

proc `%`[T](s: TSet[T]): PJsonNode =
  new(result)
  result.kind = JArray
  newSeq(result.elems, len(s))
  var i = 0
  for p in s:
    result.elems[i] = % p
    inc(i)

proc `%`(filter: TMarketFilter): PJsonNode = 
  result = % {"textQuery": % filter.textQuery, "exchangeIds": % filter.exchangeIds}

proc listMarkets(session: PSession, filter: TMarketFilter): PJsonNode =
  PJsonNode params = % {"filter": % filter}
  # Make HTTP request....

var filter = TMarketFilter(textQuery: "A Query")
session.listMarkets(filter)

[Araq]

WIll be mitigated with the planned handling of 'nil' in 'len' and 'add'.

[Varriount]

What would these plans be?