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

Prevent assignment to for bindings #3388

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LiberalArtist
Copy link
Contributor

This is an attempt at resolving #3378 by making it a syntax error to attempt to set! any of the identifiers bound by for-like forms. All of the existing tests pass (except those added in 8ca4977 that test the behavior I'm trying to change), but this needs more tests and, if it works out, a corresponding change to the documentation from 8ca4977. It may also need some more syntax-protects.

Here’s an example that breaks:
```
> (for/lists (lst) ([i (in-range 2)])
      (define lst 'd)
      #:break #f
      (cons 'a lst))
; readline-input:5:12: define-values: duplicate binding name
;   at: lst
;   in: (define-values (lst) (quote d))
```
@mflatt
Copy link
Member

mflatt commented Sep 15, 2020

Sorry for the delay here.

While I appreciate this experiment, it's a kind of backward-incompatible change to the semantics that we normally refrain from. Sometimes, these kinds of changes are necessary to keep the language safe or to close a security hole. I don't think that's the case here, though. For example, it's reasonable for for/lists to check that it has a list to reverse. (Also, shadowing can cause the same kinds of problems as assignment — unless your changes also cover shadowing, but then it's a further change to the semantics.) Assigning to a for variable is a bad idea, but I think it's well defined and safe, so a programmer who misuses assignment that way just makes their own code bad.

If I'm not misunderstanding the safety issues, maybe this change is better to take up in Rhombus?

@LiberalArtist
Copy link
Contributor Author

I don't have a very firm opinion on this. I have never seen this be an issue in practice—certainly I have never wanted to set! a for variable—which probably suggests leaving well enough alone.

The aspect that seems most concerning is that users of define-sequence-syntax, in particular, are sort of encouraged to use unsafe operations for performance after an initial check. If a sequence implementer hadn't considered the possibility of set! (which I hadn't, until #3378), the breakage could be worse than a bad error message. On the other hand, it's encouraging that only in-directory actually seemed vulnerable to assignment, and it used safe functions.

(Also, shadowing can cause the same kinds of problems as assignment — unless your changes also cover shadowing, but then it's a further change to the semantics.)

I'm not sure I'm thinking of what you have in mind: do you mean something like this?

#lang racket
(for/fold ([x 'a])
          ([x '(b)])
  (define x 'c)
  #:break #f
  (define x 'd)
  x)

Compatibility aside, what I was aiming for was roughly to be able to imagine that for/fold desugared to an application of sequence-fold to a lambda expression. ("Roughly" because that doesn't account for things like #:break, #:final, and nested iteration.) The for/fold implementation itself, each sequence transformer, and the user code would have their own identifiers, as though they were functions accepting arguments and returning results, rather than bits of code sliced up and inlined around each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants