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

fix: prefer longer parse even if unsuccessful #1658

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

@@ -59,6 +59,10 @@ instance : Ord USize where
instance : Ord Char where
compare x y := compareOfLessAndEq x y

instance lexOrd [Ord α] [Ord β] : Ord (α × β) where
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we didn't make this a global instance. It conflicts with the general pointwise order instance which works more generally for preorders as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I thought we had discussed this instance before and was quite confused that it did not exist at all (but I may have confused it with the general Ord/LT diamond discussion).

@Kha Kha force-pushed the longest-parse-prio branch from 55e1f5b to cb9e35f Compare September 28, 2022 09:12
@Kha Kha marked this pull request as ready for review September 28, 2022 09:13
@@ -1424,7 +1424,7 @@ def longestMatchStep (left? : Option Syntax) (startSize startLhsPrec : Nat) (sta
let prevLhsPrec := s.lhsPrec
let s := s.restore prevSize startPos
let s := runLongestMatchParser left? startLhsPrec p c s
match compare previousScore (score s prio) with
match (let _ := @lexOrd; compare previousScore (score s prio)) with
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this the correct idiom then :) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also do local attribute [instance] lexOrd before the function, or have a type alias Lex A B on which to unconditionally put the instance. But this way is fine too, or (inst := lexOrd) in compare. If this program were subject to proofs I would generally prefer haveI here to avoid creating the additional let binding in the generated term, but for this code that's not really a concern and the compiler will normalize the two variants the same anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I will merge as-is. I think the haveI/letI remark is important, we should discuss it in the next dev-meeting. We also get back to the let vs let dep discussion. It would be great to use the same style for letI (let inl?), and have it as part of the let family: let, let rec, let dep, let mut, ...

@leodemoura leodemoura merged commit 71e6470 into leanprover:master Sep 28, 2022
@Kha Kha deleted the longest-parse-prio branch November 14, 2022 13:29
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.

4 participants