strictFuncs+views: add test that imports stdlib modules

[ee7]

This commit attempts to improve testing of strictFuncs and views, and
prevent regressions like #16873 (resolved by 0b01edd).

We previously only explicitly tested strictFuncs and views with a
smaller number of stdlib modules, mostly in:

Nim/tests/effects/tstrict_funcs.nim

Lines 1 to 16 in 111092e

	discard """
	cmd: "nim c --experimental:strictFuncs --experimental:views $file"
	"""
	import tables, streams, parsecsv
	# We import the below modules to check that they compile with `strictFuncs`.
	# They are otherwise unused in this file.
	import
	complex,
	httpcore,
	math,
	nre,
	rationals,
	sequtils,
	strutils,
	uri

and in

Nim/tests/views/tcan_compile_nim.nim

Lines 1 to 4 in de4f260

	discard """
	cmd: "nim check --hints:on --experimental:strictFuncs --experimental:views compiler/nim.nim"
	action: "compile"
	"""

Note that this commit leaves the pegs module commented out; it
cannot currently be compiled with --experimental:views (see #16892).

Note also that this PR is not sufficient to test strictFuncs and views,
but it does detect a subset of problems. One regression that it does not
detect is when compiling the below with --experimental:strictFuncs

import std/sequtils

type
  Foo = ref object
    x: int

let a = repeat(Foo(x: 2), 3)

which compiles without error in Nim 1.4.2, but currently produces the
error 'repeat' can have side effects in Nim devel.

---

As far as I can tell, we didn't test strictFuncs and views with the stdlib much:

$ git grep --ignore-case --break --heading strictFuncs 
tests/effects/tfuncs_cannot_mutate.nim
9:{.experimental: "strictFuncs".}

tests/effects/tfuncs_cannot_mutate_simple.nim
8:{.experimental: "strictFuncs".}

tests/effects/tstrict_funcs.nim
2:  cmd: "nim c --experimental:strictFuncs --experimental:views $file"

tests/views/tcan_compile_nim.nim
2:  cmd: "nim check --hints:on --experimental:strictFuncs --experimental:views compiler/nim.nim"

[nc-x]

I have also been thinking about the state of several experimental features and how to improve their testing.
Maybe, for devel we can enable a few experimental features by default (features that are supposed to be very close to the finish line, but require a bit of usage to find any major issues).
Three examples would be orc, strictFuncs and strictNotNil. So, in config/nim.cfg we enable these features in devel. Anybody using devel would be testing these features then. Before releasing any new stable version of Nim, these experimental features can be disabled again. If all goes well these features can be promoted to stable.
This is similar to how it works in Rust and has worked well for them.

Anyways, we do need to a way to improve testing and usage of experimental features. Lets see if anybody else has any good ideas.

[saem]

I believe these tests will show that something is wrong, but since the space over which something is wrong is rather large narrowing will take a lot of time. There is still a lot of utility here, so don't want to downplay the value.

Personally, I think something that would not only help here but also compiler testing in general would be a code generator and property based testing. An example I can recall right now (though I'm sure there are others) is in a language called Alpaca (ML like language atop BEAM/Erlang), where they used Erlang and the propEr library, you can see the test code here. Good news is that we already have an RNG (Mersenne), the missing parts to get started are generators to manage the seed and invocation counts for runs, then it's generating strings, numbers, etc... but I'd recommend heading down this road. To help create examples of various combinations of experimental features. Hope that's the type of idea you were soliciting on IRC, @nc-x.

[ee7]

I believe these tests will show that something is wrong, but since the space over which something is wrong is rather large narrowing will take a lot of time. There is still a lot of utility here, so don't want to downplay the value.

Indeed: this PR isn't sufficient to test strictFuncs, but it does detect a subset of problems.

This PR does not detect that compiling something like the below with nim c --experimental:strictFuncs

import std/sequtils

type
  Foo = ref object
    x: int

let a = repeat(1, 3)
doAssert a == @[1, 1, 1] # works with `--experimental: strictFuncs`

let b = repeat(Foo(x: 2), 3) # error with `--experimental:strictFuncs`

currently produces an error on devel:

/nim/lib/pure/collections/sequtils.nim(171, 6) Error: 'repeat' can have side effects
an object reachable from 'x' is potentially mutated
/nim/lib/pure/collections/sequtils.nim(182, 15) the mutation is here
/nim/lib/pure/collections/sequtils.nim(182, 15) is the statement that connected the mutation to the parameter

See #16305 (the above example is similar to example 5).

The implementation of sequtils.repeat in current devel is:

Nim/lib/pure/collections/sequtils.nim

Lines 171 to 182 in c05d1aa

	func repeat*[T](x: T, n: Natural): seq[T] =
	## Returns a new sequence with the item `x` repeated `n` times.
	## `n` must be a non-negative number (zero or more).
	##
	runnableExamples:
	let
	total = repeat(5, 3)
	assert total == @[5, 5, 5]
	result = newSeq[T](n)
	for i in 0 ..< n:
	result[i] = x

[ee7]

Ready for review.

Questions for the reviewer (mostly bikeshedding):

1. Do you prefer another way of importing every stdlib module? I'm fine with the current approach, but ideally we'd remember to add modules to the list as they're added in std/. Generating the module list might be unreliable.
2. Are there other modules that should be added? Note that pegs is commented out as it cannot currently be compiled with --experimental:views (see strictFuncs + views: cannot import pegs #16892). And I think adding --profiler:on and uncommenting nimprof made CI fail on Windows/BSD, but I don't know why. Maybe it would work with action: "compile"?
3. Are there modules that should be removed? Currently prelude is imported, but that's harmless.
4. Should the files be in tests/effects/ or tests/views/? Tell me if you prefer to have a centralized module list that's used by tests in both folders.
5. Testament: Is there a way to use different options for each target? Currently this PR just uses two separate files - I couldn't see how to make it work with e.g. a matrix and when defined(js).
6. Testament: Should we use action: "compile" and/or cmd: "nim check"? Should we use targets differently?
7. Testament: Should we use a matrix to test the combinations like the below?

matrix: "--experimental:strictFuncs; --experimental:views; --experimental:strictFuncs --experimental:views`

If you have suggested changes to the testament use, please use the GitHub "suggested changes" UI or provide the full contents of the discard """ """ for every file.

[Araq]

Three examples would be orc, strictFuncs and strictNotNil. So, in config/nim.cfg we enable these features in devel.

Good idea. We know that --gc:orc doesn't work yet but I know the other options were tested with bootstrapping. And we should ensure it stays this way, yes.

[nc-x]

@Araq opened #16913 for discussion as it is independent of this PR.

[Araq]

Do you prefer another way of importing every stdlib module? I'm fine with the current approach, but ideally we'd remember to add modules to the list as they're added in std/. Generating the module list might be unreliable.

It's ok this way. We can always change it later.

Are there other modules that should be added? Note that pegs is commented out as it cannot currently be compiled with --experimental:views (see #16892). And I think adding --profiler:on and uncommenting nimprof made CI fail on Windows/BSD, but I don't know why. Maybe it would work with action: "compile"?

Action: "compile" is worse than testing things compile completely.

Are there modules that should be removed? Currently prelude is imported, but that's harmless.

Look ok, we can always patch it later.

Should the files be in tests/effects/ or tests/views/? Tell me if you prefer to have a centralized module list that's used by tests in both folders.

In "tests/strictfuncs" I guess but we can move them around later.

Testament: Is there a way to use different options for each target? Currently this PR just uses two separate files - I couldn't see how to make it work with e.g. a matrix and when defined(js).

If there is such a way, it would hardly be better than your approach.

Testament: Should we use action: "compile" and/or cmd: "nim check"? Should we use targets differently?
Testament: Should we use a matrix to test the combinations like the below?

Maybe. Eventually.

[timotheecour]

@ee7 this should use an auto-generated list of modules with a blacklist of unsupported modules instead of a whitelist, otherwise this becomes out of sync and is a maintenance hurdle.

See how to do this in #13221 or (newer) https://github.com/nim-lang/fusion/pull/24/files