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

Simplify the semantics of Lambda.free_variables and Lambda.subst #1606

Merged
merged 7 commits into from Mar 15, 2018

Conversation

Projects
None yet
3 participants
@chambart
Copy link
Contributor

chambart commented Feb 13, 2018

Lambda.free_variables assumes that no variables are bound multiple times, and Lambda.subst_lambda assumes that the substitution do not clash with any bound variable.

The patch is larger than can be expected for this kind of change because I changed Lambda.subst_lambda from using Ident.tbl to using Ident.Map.t. The remove function on the map is required and is not implemented on tbl. I think it is better to change the type (and all the uses of subst_lambda) rather than implementing remove on tbl.

The main reason for the change is that it is useful (and the cleanest way) for a patch about let-rec compilation.

I'd like to highlight a particular case about Lifused in free_variables. The argument variable is not considered a free variable (which was the case before). But it's not obvious whether this is correct. I don't think that a situation could arise where this matters but I am still a bit puzzled by that.

Also this code might be slightly slower, but I couldn't notice the difference.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 13, 2018

The patch seems correct from a distance, but I have several naive questions:

  • There seems to be subtle differences between Ident.tbl and Ident.Map.t that I am not sure I understand. Is there a documentation somewhere of what .tbl does, when it should, and when it should not be used?

  • The Lambda.iter function could be described as a half-assed attempt at genericity. When is it the right thing to do to write your own recursive boilerplate again (as you did here), to change the generic traversal (as you could have done) or to add new traversals? Is there a generic traversal that would improve things here?

Now to more precise comments:

  • free_methods does something very strange and its only call-site seems to be very hackish; it is not clear that maintaining free_ids and free_methods in Lambda is worth it if you stop using free_ids for free_variables, they could move to Translclass.

  • It is in fact possible to implement free* and subst without remove, by maintaining the set of bound variables as you move inside terms -- then on non-binding variable occurrences you only act if the variable is not in that bound set. (I am not claiming that it would be better than your approach, I don't know.)

  • Would it make sense to bite the bullet and implement environment-passing open-recursion iter, map and fold for Lambda? This should let you implement free_variables, subst{,_lambda} and map, but also large parts of Simplif

@chambart

This comment has been minimized.

Copy link
Contributor Author

chambart commented Feb 13, 2018

As long as you use only find_same map and tbl are equivalent. But tbl have some functions to search by name only (and not stamp). Also adding twice a binding for a given ident won't replace the old one, it will still be returned by find_all. In general, I would advise to use Ident.Map.t instead of tbl almost everywhere, except in the Env module.

Usually, when the type is not too large, I find it less error-prone to rewrite the whole recursion. If you want the function to be properly maintainable, you often need to list all the cases, and there is a very small gain from using a generic iterator. Especially if you want the iterator to be generic enough to provide both a way to propagate information down and up, which is needed for instance for the substitution.

My conclusion for that would be that there are not enough traversals similar enough to benefit from introduction of specific generic function. But there are a few functions in Simplif that could probably be rewritten with iter and may benefit from it. I can take a look into it.

For the specific questions:

  • I'm going to get rid of free_ids and move that to translclass.
  • I considered doing subst with a 'removed' set. But I find it a bit ugly. And I consider moving things from tbl to map to be a benefit.
  • As explained before, I wouldn't consider it an improvement.
@gasche

This comment has been minimized.

Copy link
Member

gasche commented Feb 13, 2018

Thanks, that clarifies things, and I think your choices are reasonable.

chambart added some commits Feb 15, 2018

Simplify free_methods and move to translclass
It does not considers assigned variables as free methods anymore.
@chambart

This comment has been minimized.

Copy link
Contributor Author

chambart commented Feb 15, 2018

I removed free_ids, inlined it into free_methods and moved it to translclass. It used to consider v as a free method in Lassign(v, _) which, even if I have absolutely no idea how this is going to be used, clearly is incorrect.

I took the opportunity to rename Lambda.iter into something that doesn't suggest that the function recursively traverse the expression. It might not be the best name, but it clearly prevent the confusion.

@gasche

gasche approved these changes Feb 25, 2018

Copy link
Member

gasche left a comment

I reviewed the PR again and I believe it is correct.

I don't know whether we should be careful about breaking the interface lambda.mli -- it may affect users of compiler-libs -- but in absence of a recognized need to preserve it, I think that maintaining the old interfaces for hamper maintainability more than they would help.

@chambart, please take care of merging (and rebasing if necessary, etc.). I think that Changes entry detailing the API changes could make sense, in the Internals category, but that's your choice.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Mar 15, 2018

I have taken the liberty of merging this with trunk and adding a Changes entry; I will merge assuming this passes CI.

@mshinwell mshinwell merged commit 96394c3 into ocaml:trunk Mar 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.