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

Lean: add support for range types #869

Open
wants to merge 1 commit into
base: sail2
Choose a base branch
from

Conversation

ineol
Copy link
Collaborator

@ineol ineol commented Jan 10, 2025

No description provided.

@ineol ineol requested a review from javra January 10, 2025 18:33
Copy link

github-actions bot commented Jan 10, 2025

Test Results

   12 files  ±0     24 suites  ±0   0s ⏱️ ±0s
  749 tests ±0    749 ✅ ±0  0 💤 ±0  0 ❌ ±0 
2 493 runs  +1  2 493 ✅ +1  0 💤 ±0  0 ❌ ±0 

Results for commit d89514c. ± Comparison against base commit f87c292.

♻️ This comment has been updated with latest results.

| Typ_app (Id_aux (Id "range", _), [A_aux (A_nexp low, _); A_aux (A_nexp high, _)]) ->
(* The heuristics is that 99% of rages are positive, so we try to
translate them to Nat. *)
if clearly_negative low then string "Int" else string "Nat"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to keep two kinds, @Alasdair implemented the Nat kind specifically to avoid having heuricstics. Not sure how to solve that for ranges, though.

I think your heuristic doensn't have a one-sided error? Nexp_neg can be a double negation, for example

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I grepped all the uses of range in the models and my understanding is that for the overwhelming majority of them they are clearly non-negative, and a few clearly include negative elements, so this is an optimistic assumption.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way to do this would be to take an env representing the context when converting the type, then you can call Type_check.prove env (nc_gteq low (nint 0)). That avoids any syntactic-restrictions, but Lean might need more explicit conversions to be inserted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule for conversions is generally that Int to Nat would have to be made explicit while Nat to Int would be implicit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@javra what do you think is the best thing to do here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried using @Alasdair 's suggestion for implementing clearly_negative? Otherwise we could just leave it as it is and add a comment to improve the heuristics later on.

But I'd call it provably_positive and only return Nat if the lower bound can be proven to be positive, otherwise we'd have to use Int in order not to run into problems.

@ineol ineol force-pushed the lean-ranges branch 3 times, most recently from dc8a1bd to 9e9d5f3 Compare January 13, 2025 18:24
@ineol ineol requested a review from javra January 15, 2025 15:08
@ineol
Copy link
Collaborator Author

ineol commented Jan 16, 2025

I implemented Alasdair's strategy and it seems to be working. I put the env in the context we already had.

Now, the ranges in parameters are transformed into atom in the enviroment somehow, and so are translated to Int. I guess for consistency we should translate them in the same way. Do you know what is going on @Alasdair ?

x

/-- Type quantifiers: x : Int, k_n : Int, 0 ≤ x ∧ x ≤ k_n -/
def f_nnegvar (x : Int) : Nat :=
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't go through because it would need an explicit conversion here. I don't immediatly get why the return type is translated as Nat but the argument type as Int 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my reply below

@Alasdair
Copy link
Collaborator

I implemented Alasdair's strategy and it seems to be working. I put the env in the context we already had.

Now, the ranges in parameters are transformed into atom in the enviroment somehow, and so are translated to Int. I guess for consistency we should translate them in the same way. Do you know what is going on @Alasdair ?

In general the type range(X, Y) will be converted into int('n) with the constraint X <= 'n <= Y. For historical reasons the int type with a single parameter is refered to as atom internally, but that isn't really visible to users. We do this canonicalisation step mostly just to ensure that different ways of writing a function type (with a range, explicit constraint, etc) all get mapped to the same thing and therefore get handled consistently.

@ineol
Copy link
Collaborator Author

ineol commented Jan 17, 2025

I updated the PR to also try to prove that parameters/atoms are non-negative. What do you think?

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.

3 participants