Skip to content

Conversation

@Equilibris
Copy link
Collaborator

@Equilibris Equilibris commented Aug 20, 2025

This PR adds an unexpander for the vector literal !![...] notation, to improve readability of goal statements.

@Equilibris Equilibris requested a review from alexkeizer as a code owner August 20, 2025 18:53
Copy link
Owner

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

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

Thanks, having an unexpander would be nice! I'm not sold on changing the separator to ; (especially as your PR mixes the two: a three-element list would be written !![ x; y, z ], which looks very wrong to me), could you revert those changes?

Also, try to write the PR description so that it makes sense without looking at the diff (it becomes part of the git history, after all).

For example:

This PR adds an unexpander for the vector literal !![...] notation, to improve readability of goal statements.

That also helps clarify, for example, if you meant to change the separator, or what your reasoning is for that.

Comment on lines 75 to 77
| `(!![$t; ]) => `($t)
| `(!![$t; $x]) => `(Vec.append1 $t $x)
| `(!![$t; $xs,* , $x]) => `(Vec.append1 !![$t; $xs,*] $x)
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you mix semi-colons and commas here? Did you mean to change the syntax?

Comment on lines 81 to 93
@[app_unexpander Vec.nil]
def nil_uex : Lean.PrettyPrinter.Unexpander
| `($_p) => `(!![])

@[app_unexpander Vec.append1]
def append1_uex : Lean.PrettyPrinter.Unexpander
| `($_p !![$x,*] $r) => `(!![$(x.push r),* ])
| `($_p !![$t; $x,*] $r) => `(!![$t; $(x.push r),* ])
| _ => throw () -- unhandled

/-- info: !![ℤ, ℕ, Prop] : Vec Type (Nat.succ 0).succ.succ -/
#guard_msgs in
#check !![ℤ, ℕ, Prop]
Copy link
Owner

Choose a reason for hiding this comment

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

Nice! Adding an unexpander for this syntax is a really good idea!

Note that standard Lean/Mathlib style does not indent after a section or namespace.
QPFTypes largely does not follow this style, but we probably should for new code so that eventually we are consistent.

That is, please remove the indentation here; but don't worry about the rest of the file (unless you feel particularly inspired to make a separate style cleanup PR)

@Equilibris
Copy link
Collaborator Author

Equilibris commented Aug 20, 2025

Sorry the [x; a,b] means that you concat a and b onto x, three element lists still have the same syntax. I can revert this though I just added it so it would apply in more situations, so the type of x would be a Vec

@alexkeizer
Copy link
Owner

alexkeizer commented Aug 20, 2025

Sorry the [x; a,b] means that you concat a and b onto x, three element lists still have the same syntax. I can revert this though I just added it so it would apply in more situations, so the type of x would be a Vec

At first glance I'm not the biggest fan, as it looks misleadingly similar, why not write x ++ !![a, b] to follow standard syntax? It's a bit larger, sure, but definitely not large enough to warrant custom magic syntax. In any case, it's orthogonal to the unexpander. Could you take it out, and if you feel strongly about this let's discuss it in a separate PR?

@Equilibris
Copy link
Collaborator Author

Sounds good! I realise that it meant to be a | as in Prolog for [x|a,b]

Copy link
Owner

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge after accepting the suggestion!

Thanks for the PR

Co-authored-by: Alex Keizer <alex@keizer.dev>
@Equilibris Equilibris enabled auto-merge (squash) August 21, 2025 07:39
@Equilibris Equilibris merged commit 2513863 into main Aug 21, 2025
2 checks passed
@Equilibris Equilibris deleted the add-vec-delab branch August 21, 2025 07: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.

3 participants