Nim icon indicating copy to clipboard operation
Nim copied to clipboard

Compiler internalError when using operations on empty untyped built-in set

Open n1kolasM opened this issue 1 year ago • 2 comments

Description

Operations on built-in set literal {} cause compiler internal error. Code examples:

assert card({}) == 0
assert {} <= {}

Also, according to manual

The empty set is type compatible with any concrete set type.

So, binary operations with empty set literal and typed set should also work:

assert {} <= {1'u8}

Nim Version

Since nim 0.19.0 to devel, introduced in #7558 Directly tested version:

Nim Compiler Version 1.0.0 [Linux: amd64]
Compiled at 2024-04-11
Copyright (c) 2006-2019 by Andreas Rumpf

git hash: f7a8fc46c0012033917582eb740dc0343c093e35
active boot switches: -d:release
Nim Compiler Version 1.6.14 [Linux: amd64]
Compiled at 2023-06-27
Copyright (c) 2006-2023 by Andreas Rumpf

active boot switches: -d:release

Current Output

Error: internal error: invalid kind for firstOrd(tyEmpty)

Expected Output

# Code compiles, all asserts pass

Possible Solution

For operations only on empty set literal it is enough to add early return in toBitSet in compiler/nimsets.nim

proc toBitSet*(conf: ConfigRef; s: PNode): TBitSet =
  result = @[]
  # Added early return
  if s.len == 0:
    return
  # Below is implementation from devel
  var first: Int128 = Zero
  var j: Int128 = Zero
  first = firstOrd(conf, s.typ.elementType)
  bitSetInit(result, int(getSize(conf, s.typ)))
  for i in 0..<s.len:
    if s[i].kind == nkRange:
      j = getOrdValue(s[i][0], first)
      while j <= getOrdValue(s[i][1], first):
        bitSetIncl(result, toInt64(j - first))
        inc(j)
    else:
      bitSetIncl(result, toInt64(getOrdValue(s[i]) - first))

Additional Information

No response

n1kolasM avatar Apr 11 '24 16:04 n1kolasM

Then the right place for fixing this problem would be GAP's PackageManager package.

ThomasBreuer avatar Nov 24 '20 11:11 ThomasBreuer

Possibly, but I'm not sure how easy it would be to do file locking in PackageManager. Correct me if I'm wrong, but I don't think GAP currently supports file locking primitives? If not, then doing it in GAP.jl might be the pragmatic approach for the time being.

rbehrends avatar Nov 24 '20 12:11 rbehrends

I thought about adding some kind of preprocessing to the installation: Before starting to download, unpack, and compile the package in question, the process could check for a file with a reserved name in the package directory; if such a file is there then someone else is currently installing this package; if the file is not there then write it, and afterwards check whether the file is there and contains what it should be in, in order to make sure that the file was not written by someone else; if the file is o.k. then install the package and finally remove the file in question. (Of course one has to deal with the case that the process crashes before the final step, for example via a reasonable timestamp heuristic.) Is this perhaps too naive?

ThomasBreuer avatar Nov 24 '20 12:11 ThomasBreuer

There are a number of issues that make this tricky:

  1. If the process crashes during installation, the lock file will be left behind, preventing further installations; I don't think a timestamp heuristic can fix that reliably.
  2. You can mitigate it by storing the PID in the lockfile and testing whether the process is alive, but doing this atomically without file locking is not easy.

It's not impossible (see Pidfile.jl), but it's much easier with fcntl().

rbehrends avatar Nov 24 '20 13:11 rbehrends