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

Expand docs for the first arg to syntax-local-make-definition-context #2043

Conversation

lexi-lambda
Copy link
Member

This expands some of the documentation for syntax-local-make-definition-context to clarify the extent to which supplying a parent definition context has an effect. I’m not sure this is the best way of phrasing things, so I’m happy if you’d prefer a different wording; I just spent a lot of time trying to hunt down a bug that stemmed from misunderstanding the existing documentation, so I wanted to try and add some clarification that would have helped me.

@lexi-lambda lexi-lambda requested a review from mflatt April 13, 2018 18:23
@mflatt
Copy link
Member

mflatt commented Apr 18, 2018

As far as I can tell, this is correct and a great improvement.

@lexi-lambda
Copy link
Member Author

I’m pretty certain it’s wrong. The reason I’m bugging you with so many questions on slack is figuring out exactly what the right thing to say is. :)

This program demonstrates why this PR is incorrect:

#lang racket

(begin-for-syntax
  (define x-id #'x)
  (define y-id #'y)

  (define intdef-ctx-a (syntax-local-make-definition-context))
  (syntax-local-bind-syntaxes (list x-id) #'(make-rename-transformer #'values) intdef-ctx-a)

  (define intdef-ctx-b (syntax-local-make-definition-context intdef-ctx-a))
  (syntax-local-bind-syntaxes (list y-id) #`(make-rename-transformer #'#,x-id) intdef-ctx-b)

  (println (local-expand y-id 'expression '() intdef-ctx-b)))
bad.rkt:5:17: x: unbound identifier
  in: x
  location...:
   bad.rkt:5:17

This modified program does not produce any errors:

#lang racket

(begin-for-syntax
  (define x-id #'x)
  (define y-id #'y)

  (define intdef-ctx-a (syntax-local-make-definition-context))
  (syntax-local-bind-syntaxes (list x-id) #'(make-rename-transformer #'values) intdef-ctx-a)

  (define intdef-ctx-b (syntax-local-make-definition-context))
  (define x_a-id (internal-definition-context-introduce intdef-ctx-a x-id))
  (syntax-local-bind-syntaxes (list y-id) #`(make-rename-transformer #'#,x_a-id) intdef-ctx-b)

  (println (local-expand y-id 'expression '() intdef-ctx-b)))

However, it prints #<syntax x>, not #<syntax values>, since x is out of context when expanded in this program. If the fourth argument to local-expand is changed to (list intdef-ctx-b intdef-ctx-a), the program prints #<syntax values>.

This would imply that the parent-ctx argument does absolutely nothing when it comes to adding scopes, and indeed, the code confirms that behavior! All parent-ctx seems to do is change the frame-id that is used, so I’m trying to understand what that does so I can update the documentation properly.

@mflatt
Copy link
Member

mflatt commented Apr 18, 2018

I see – I focused on the part of what the other functions don't do, and not on what syntax-local-make-definition-context does. And, yes, now I understand why you're asking about frame-id. It seems clear that I've forgotten what frame-id is about.

Next try:

I believe frame IDs are about detecting the situation where a use-site scope needs to be added, where not adding a use-site scope is a useful optimization. A use-site scope is needed when a macro is being used in the same context where the macro is defined. That correspondence is detected by comparing the frame ID of a macro's binding to the current expansion context's frame ID. A definition context needs to carry its frame ID for binding via syntax-local-bind-syntaxes as well as for local-expand to set the current expansion context.

Now, what happens when you have a definition context immediately within some other definition context? The expander wants to conservatively treat those as the same definition context for the purposes of triggering a use-site scope. If you create a definition context during expansion within an existing definition context, that sameness can be detected automatically; the current context's frame ID is captured as the new context's frame ID. But if a programmer is creating multiple definition contexts and mixing them together at the end (by passing them both to local-expand), then you have to tell the expander that the definition contexts should be the same. That turns out to be what it means to provide an existing definition context to syntax-local-make-definition-context.

If you don't tell syntax-local-make-definition-context that you plan to combine two definitions contexts together in local-expand, the consequence is that use-site scopes might not be added often enough, and an expansion might break. In fact, local-expand should check that they're consistent in that way and with the current expansion context, instead of just going wrong.

The v6 expander performed that check, but in a different way than using a frame-id field. It had a stronger constraint than the new implementation, where a definition context could only be used in the same context where it is created. I lifted that restriction, because I saw no reason for it, but I effectively lost the consistency check on frame IDs (which was part of the reason for the old constraint, it seems).

If all that is right, then the documentation could say that you need to provide a definition-context argument to syntax-local-make-definition-context to enable combining that one and the result in a list for local-expand. More generally, if C1 R C2 means that C2 was created by passing C1 to syntax-local-make-definition-context, then N definition contexts can be passed to local-expand as long as the transitive closure of R starting from one of them reaches the whole set.

@lexi-lambda
Copy link
Member Author

Closing in favor of #2051.

@lexi-lambda lexi-lambda deleted the clarify-syntax-local-make-definition-context-docs branch April 19, 2018 22:40
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