-
Notifications
You must be signed in to change notification settings - Fork 465
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
RFC: termination_by syntax: less wiggle-room with variables #3081
Comments
Not necessarily contradicting you here but note that this is the same behavior as |
Yeah, I'm somewhat on the fence over this change overall. Also, omitting the |
Another alternative for "less wiggle room" would be to require naming all the variables! I think more important than the actual requirement is what the error messages are when it is not fulfilled. |
Yes, that's a possible alternative. I think “all variables” can be confusing, though, or at least counter intuitive, as it would involve (The last bit makes me wonder what happens if only the termination_by clause in a local |
I started to implement that The change I have to make to user code “feel right”, changes like @@ -468,7 +468,7 @@ theorem State.join_le_left (σ₁ σ₂ : State) : σ₁.join σ₂ ≼ σ₁ :=
next => apply cons_le_cons; apply le_trans ih (erase_le _)
next => apply le_trans ih (erase_le_cons (le_refl _))
next h => apply le_trans ih (erase_le_cons (le_refl _))
-termination_by σ₁ _ => σ₁.length
+termination_by σ₁.length So worth persuing. I am hitting a slight snag in the interaction with GuessLex, which infers a termination order, and wants to use parameters that come before the At least currently, GuessLex goes through the same interface at the user. The new check is applied before, so only applies to user-written code, but it still means we can’t have all of these:
A pragmatic way forward may be to suggest Ah, or I can pass them on with the full array to rename everything to the next stage, but print it with the trimmed list of variables. This should be pretty and working in most cases, and be hopefully only mildly annoying in other cases. |
Done as per #3040 |
Status quo
Currently (assuming #2921), the
termination_by
syntax expect a list, possibly empty, of identifiers (or_
), then=>
, and then the body.The semantics is that the names in the list override any variable names from the back. Examples:
works but equivalently
or
This is a bit silly: Why would the user not want to use the variable name given in the header? Is there ever a compelling reason?
Of course, the list of variables do have their justification: When the function arguments are not in the header, but bound by a lambda or by match equations:
or
If there are variables both in the header and in the lambda/matches, the list is applied from the right:
and you can wildly mix overriding variable names and referring to the original name using
_
:Oddities
This works, and knowing the implementation it’s not surprised it behaves like this, but it is a bit odd:
termination_by => n
looks odd.:
” lean considers, but maybe not always.a
tox
here.Proposed change
I wonder if our users are not better served with a more rigid syntax:
:=
or discriminants in matches equations). No fewer and not more.=>
must be omitted.So above the valid variants would be:
If the system requires the user to get the number of variables right, the effect will more likely match the user’s intention than the more liberal “override from left to right”.
Impact
As we are changing the syntax anyways in #3040, the pain of having to touch this code would be mostly alleviated.
The implementation would probably have to carry around a bit of extra book keeping to remember which variables were originally bound before the
:
and which were afterwards. (This is maybe the reason it’s not already like this). But maybe worth for the extra guard rails.Maybe there is an good compelling reason to allow users to rename variables? I don't see one yet.
Add 👍 to issues you consider important. If others benefit from the changes in this proposal being added, please ask them to add 👍 to it.
The text was updated successfully, but these errors were encountered: