Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

func doesn't check for side effects in proc parameters #16303

Closed
ee7 opened this issue Dec 9, 2020 · 13 comments · Fixed by #16781
Closed

func doesn't check for side effects in proc parameters #16303

ee7 opened this issue Dec 9, 2020 · 13 comments · Fixed by #16781
Labels
Documentation Content Related to documentation content (not generation). Easy Hacktoberfest (Open) Issues suitable for Hacktoberfest, open for working on.

Comments

@ee7
Copy link
Contributor

ee7 commented Dec 9, 2020

What's the intended behaviour here?

Example

proc echoInt(n: int) =
  echo n

func foo(n: int, bar: proc(x: int)) =
  bar(n) 

foo(2, echoInt)

See: https://play.nim-lang.org/#ix=2HiM

Current Output

2

which also occurs if you compile with --experimental:strictFuncs.

Expected Output

Either

2

in which case, I think the manual could be a little clearer, or:

foo.nim(4, 6) Error: 'foo' can have side effects

which is what happens if you call echoInt inside the func more directly:

 proc echoInt(n: int) =
   echo n

 func foo(n: int, bar: proc(x: int)) =
-  bar(n)
+  echoInt(n)

 foo(2, echoInt)

An error is produced if we write:

 proc echoInt(n: int) =
   echo n

-func foo(n: int, bar: proc(x: int)) =
+func foo(n: int, bar: proc(x: int) {.noSideEffect.}) =
  bar(n)

 foo(2, echoInt)

Additional Information

This behaviour occurs with devel and all Nim versions going back to v0.18.0, which introduced func.

The manual contains:

Nim/doc/manual.rst

Lines 6155 to 6161 in 9ce2f87

The ``noSideEffect`` pragma is used to mark a proc/iterator to have no side
effects. This means that the proc/iterator only changes locations that are
reachable from its parameters and the return value only depends on the
arguments. If none of its parameters have the type ``var T`` or ``out T``
or ``ref T`` or ``ptr T`` this means no locations are modified. It is a static
error to mark a proc/iterator to have no side effect if the compiler cannot
verify this.

The procedural type is internally a pointer to a procedure, but I don't know if it's supposed to be covered by "var T or out T
or ref T or ptr T".

The experimental manual says this about strictFuncs:

Since version 1.4 a stricter definition of "side effect" is available. In addition
to the existing rule that a side effect is calling a function with side effects
the following rule is also enforced:
Any mutation to an object does count as a side effect if that object is reachable
via a parameter that is not declared as a ``var`` parameter.

@timotheecour
Copy link
Member

timotheecour commented Dec 9, 2020

IMO the current behavior is both the correct and the useful one, the side effect only happens via a parameter which is declared proc.

The manual just needs to be clarify that side effects are allowed via proc parameters.

@timotheecour timotheecour added the Documentation Content Related to documentation content (not generation). label Dec 9, 2020
@ee7
Copy link
Contributor Author

ee7 commented Dec 9, 2020

--experimental:strictFuncs enforces that a parameter that mutates must be a var parameter.

Could it also enforce that a proc parameter does not have side effects?

@timotheecour
Copy link
Member

timotheecour commented Dec 9, 2020

Could it also enforce that a proc parameter does not have side effects?

that would make func much less useful; the fact that proc param can mutate is already encoded in the type of the proc being proc instead of func; if you want to ensure no mutation happens, use func for the proc parameter. The current func design (EDIT: excluding strictFuncs/views, refs #16305) is sane and gives API strictly more control

@ee7
Copy link
Contributor Author

ee7 commented Dec 9, 2020

if you want to ensure no mutation happens, use func for the proc parameter.

Do you mean, "add {.noSideEffect.} to the proc parameter"? Like:
func foo(bar: proc(n: int) {.noSideEffect.}) =

Because if you write:
func foo(bar: func(n: int)) =

it produces:

Error: func keyword is not allowed in type descriptions, use proc with {.noSideEffect.} pragma instead

@timotheecour
Copy link
Member

timotheecour commented Dec 10, 2020

Do you mean, "add {.noSideEffect.} to the proc parameter"? Like:

@timotheecour
Copy link
Member

timotheecour commented Dec 10, 2020

And here's an example explaining why it's safe to have a func that contains a proc param:

when true:
  func callCb(cb: proc(): int): int = result = cb()
  proc mycb1(): int = 1
  proc mycb2(): int = (echo "ok"; 1)
  func bar(): int =
    # result = callCb(mycb1) # ok: no error generated here
    result = callCb(mycb2) # ok: correctly reports Error: 'bar' can have side effects
  echo (bar(),)

callCb is legally a func, and compiler guarantees side effects can only happen through cb.
But calling callCb itself does correctly generate a side effect error if used inside a func and its param cb has side effects, as in result = callCb(mycb2)

ee7 added a commit to ee7/Nim that referenced this issue Dec 10, 2020
This commit changes the funcs that take a `proc` parameter back to
procs.

This reverts some of commit 6f57eba:
  sequtils.nim: Use `func` (nim-lang#16293)

See also:
- nim-lang#16303
- nim-lang#16304
@Araq
Copy link
Member

Araq commented Dec 10, 2020

Works as the spec says it does, the spec is overly brief though.

See for example:

proc echoInt(n: int) =
  echo n

func foo(n: int, bar: proc(x: int)) =
  bar(n)

func more =
  foo(2, echoInt) # error: 'more' can have side effects

more()

@Araq Araq closed this as completed Dec 10, 2020
@timotheecour
Copy link
Member

timotheecour commented Dec 10, 2020

Works as the spec says it does, the spec is overly brief though.

the spec doesn't match the implementation, as I explained above.
https://nim-lang.github.io/Nim/manual.html#pragmas-nosideeffect-pragma says:

If none of its parameters have the type var T or out T or ref T or ptr T this means no locations are modified.

That's incorrect, see below example:
bar is neither ptr|ref nor a var (and out T shouldn't be there) and yet count is modified:

when true:
  func foo(bar: proc(): int): int = bar()
  var count = 0
  proc fn1(): int = 1
  proc fn2(): int = (count.inc; count)
  func fun1() = discard foo(fn1) # ok
  # func fun2() = discard foo(fn2) # Error: 'fun2' can have side effects

  # with callbacks, the compiler is conservative, ie that bar will have side effects
  var foo1: type(foo) = foo
  func main() =
    discard foo(fn1) # ok
    # discard foo1(fn1) # now this errors
  main()

This time, the implementation is the correct, sane, most useful behavior (see #16303 (comment)), the spec is overly restrictive.

It's easy to correct and I can volunteer a PR to fix it, along those lines:


Side effects in a {.noSideEffect.} proc foo, are only allowed through proc params, and when all such params are statically determined to be {.noSideEffect.}, foo will indeed be {.noSideEffect.}, in other words {.noSideEffect.}` is determined at call site, see example below:

< insert above example >.

If none of its parameters have the type var T or out T or ref T or ptr T or proc without {.noSideEffect.} this means no locations are modified.


In the new spec, {.noSideEffect.} tells you that side effects only happen through proc params, which is still a strong guarantee, and make these procs useful in more contexts:

  • it allows uing foo inside a func (when all its params are statically {.nosideeffect.})
  • but it also allows passing params with side effects (and then resulting foo won't itself be usable inside a func, and the compiler checks for this)

@timotheecour timotheecour reopened this Dec 10, 2020
@Araq
Copy link
Member

Araq commented Dec 10, 2020

I'm refering to this section of the spec:

  1. Every indirect call via some proc type T is assumed to
    raise system.Exception (the base type of the exception hierarchy) and
    thus any exception unless T has an explicit raises list.
    However, if the call is of the form f(...) where f is a parameter of the currently analyzed routine it is ignored. The call is optimistically assumed to have no effect. Rule 2 compensates for this case.
  2. Every expression of some proc type within a call that is not a call
    itself (and not nil) is assumed to be called indirectly somehow and thus
    its raises list is added to p's raises list.

Every effect is subject to these rules, including the write tracking effect. Unfortunately the spec doesn't say that. So yeah, PRs are welcome.

@juancarlospaco
Copy link
Collaborator

juancarlospaco commented Dec 10, 2020

A decision must be made, this is becoming edit war via pull requests proc ↔️ func

@ee7
Copy link
Contributor Author

ee7 commented Dec 10, 2020

A decision must be made soon, this is becoming edit war

I think that's a bit too strong.

There's just 2 open PRs, they're both mine, and they're simply:

There's no rush, except that people on devel right now could see some regressions when using strictFuncs with a small number of the newly changed funcs. I don't consider myself in a war, and there's no competing pull requests.

But at this point I'd like to make a PR that does a safe first pass that changes all procs tagged with {.noSideEffect.} to func. I just started with a module-by-module approach attempting to make it easier to review, easier to diagnose CI failures, and to see what breaks. At least it created some discussion.

For procs that aren't tagged with noSideEffect, if there is actually a question about which we should change, we can always handle that in a later pass.

@juancarlospaco
Copy link
Collaborator

is not about the amounts, is about wasted effort, that can be put into fixing actual bugs.
The spec should cover this whatsoever. I agree with you, I did not say theres a "rush".

@ee7
Copy link
Contributor Author

ee7 commented Dec 10, 2020

A decision must be made

Well, in any case it's easy to just keep a proc that takes a proc parameter as a proc for now.

wasted effort, that can be put into fixing actual bugs.

Sure.

But this discussion would've happened eventually, and we can always just revert any questionable commit. So hopefully most of the wasted effort is mine :)

ee7 added a commit to ee7/Nim that referenced this issue Dec 14, 2020
This commit changes the funcs that take a `proc` parameter back to
procs.

This reverts some of commit 6f57eba:
  sequtils.nim: Use `func` (nim-lang#16293)

See also:
- nim-lang#16303
- nim-lang#16304
Araq pushed a commit that referenced this issue Dec 14, 2020
This commit changes the funcs that take a `proc` parameter back to
procs.

This reverts some of commit 6f57eba:
  sequtils.nim: Use `func` (#16293)

See also:
- #16303
- #16304
@ringabout ringabout added Easy Hacktoberfest (Open) Issues suitable for Hacktoberfest, open for working on. labels Jan 11, 2021
mildred pushed a commit to mildred/Nim that referenced this issue Jan 11, 2021
This commit changes the funcs that take a `proc` parameter back to
procs.

This reverts some of commit 6f57eba:
  sequtils.nim: Use `func` (nim-lang#16293)

See also:
- nim-lang#16303
- nim-lang#16304
ardek66 pushed a commit to ardek66/Nim that referenced this issue Mar 26, 2021
This commit changes the funcs that take a `proc` parameter back to
procs.

This reverts some of commit 6f57eba:
  sequtils.nim: Use `func` (nim-lang#16293)

See also:
- nim-lang#16303
- nim-lang#16304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Content Related to documentation content (not generation). Easy Hacktoberfest (Open) Issues suitable for Hacktoberfest, open for working on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants