import strtabs fails with Error: '[]=' can have side effects when strictFuncs is active

[sschwarzer]

If the --experimental:strictFuncs option is used, an import of strtabs causes a compile time error.

Example

# in strtabs_error.nim
import strtabs

Current Output

$ nim --experimental:strictFuncs c strtabs_error.nim 
Hint: used config file '/.../.choosenim/toolchains/nim-#devel/config/nim.cfg' [Conf]
Hint: used config file '/.../.choosenim/toolchains/nim-#devel/config/config.nims' [Conf]
Hint: used config file '/../config.nims' [Conf]
....................
/.../.choosenim/toolchains/nim-#devel/lib/pure/strtabs.nim(234, 6) Error: '[]=' can have side effects

Expected Output

The above snippet should compile without error.

Possible Solution

The proc referenced by the compiler is

proc `[]=`*(t: StringTableRef, key, val: string) {.                             
  rtlFunc, extern: "nstPut", noSideEffect.} =                                   
  ## Inserts a `(key, value)` pair into `t`.                                    
  ##                                                                            
  ## See also:                                                                  
  ## * `[] proc <#[],StringTableRef,string>`_ for retrieving a value of a key   
  ## * `del proc <#del,StringTableRef,string>`_ for removing a key from the table
  runnableExamples:                                                             
    var t = {"name": "John", "city": "Monaco"}.newStringTable                   
    t["occupation"] = "teacher"                                                 
    doAssert t.hasKey("occupation")                                             
                                                                                
  var index = rawGet(t, key)                                                    
  if index >= 0:                                                                
    t.data[index].val = val                                                     
  else:                                                                         
    if mustRehash(len(t.data), t.counter): enlarge(t)                           
    rawInsert(t, t.data, key, val)                                              
    inc(t.counter)

Note the noSideEffect pragma, which doesn't seem to make sense since the point of the proc is to modify the table. So the noSideEffect pragma should be removed. (I wonder why it was added to begin with.)

Additional Information

$ nim -v
Nim Compiler Version 1.3.5 [Linux: amd64]
Compiled at 2020-08-01
Copyright (c) 2006-2020 by Andreas Rumpf

active boot switches: -d:release

[sschwarzer]

I found a few other errors:

pegs.nim(164, 6) Error: 'charSet' can have side effects
an object reachable from 's' is potentially mutated

nre.nim(374, 6) Error: '[]' can have side effects
an object reachable from 'pattern' is potentially mutated

After removing the noSeideEffect pragma from strtabs, I get:

xmltree.nim(337, 6) Error: 'delete' can have side effects
an object reachable from 'n' is potentially mutated

mimetypes.nim(1885, 6) Error: 'newMimetypes' can have side effects

After removing noSideEffect from xmltree.delete, I see only errors from pegs.nim, nre.nim and mimetypes.nim, where it isn't obvious to me whether these are supposed to have side effects or not.

Edit: I tried to attach my script (created with find and some Vim macros) that found the errors to this comment, but Github only gives me "Something went really wrong, and we can't process that file." :-(

Edit 2: I uploaded the script to https://pastebin.com/BZVYpTQ3 .

[Yardanico]

@sschwarzer is this fixed by #15143 ?

[sschwarzer]

@sschwarzer is this fixed by #15143 ?

Yes, and a similar bug in the xmltree module.

As mentioned, my script (linked above) tells me about more problems in module imports, but I don't know if the pragmas there are wrong or the compiler. Therefore I didn't make any changes to these other modules (mimetypes.nim, nre.nim and pegs.nim).

Note that if you fix one of the modules, you may see errors with other modules that were originally imported. For example, xmtree imports strtabs and I saw the problem with xmltree only after fixing strtabs.

[Araq]

Well this is not very actionable. Please create PRs that remove the .noSideEffect annotation and then we can discuss it in detail.

[sschwarzer]

I have made the following commits:

devel...sschwarzer:strictfuncs_errors2

With these commits, my script (linked above) no longer reports side effect errors with experimental: "strictFuncs" enabled.

Should I create a single pull request, one per file or something else? Please let me know.

[Araq]

Single pull request suffices. However, it looks like most of these indicate "strictFuncs" bugs, not stdlib problems.

[sschwarzer]

Single pull request suffices. However, it looks like most of these indicate "strictFuncs" bugs, not stdlib problems.

Indeed I was wondering if just the side effect analysis for strictFuncs didn't work correctly. That was also a reason why I didn't include these changes in my first pull request. I can't tell whether the analysis is ok because I don't know what the intended semantics for the analysis are. :-)

If the annotations are correct - once the side effect analysis is improved - wouldn't it be better not to apply the pull request and instead fix the side effect analysis for strictFuncs? This would avoid double work because it would make sense to readd the annotations after the side effect analysis is fixed.

It may be that I misunderstood something; in this case please clarify. :-)

[Araq]

If the annotations are correct - once the side effect analysis is improved - wouldn't it be better not to apply the pull request and instead fix the side effect analysis for strictFuncs?

Yes, exactly.

[sschwarzer]

Ok, what do we do with this ticket now? Strictly speaking, the wrong annotations (in strtabs and xmltree) have been fixed, so we can close this ticket.

However, it might still be worth to act on this ticket by adding test cases to Nim to be triggered by the faulty strictFuncs analysis. But maybe this should go into a new ticket.

Anyway, make up your mind and feel free to close this ticket if you want. :-)

By the way, welcome back! :-)

[Araq]

I'm leaving this open since I'm working on a fix.

[Araq]

Fixed as far as I can tell.