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 detection of loop headers #1247

Merged
merged 3 commits into from
Nov 29, 2024
Merged

Fix detection of loop headers #1247

merged 3 commits into from
Nov 29, 2024

Conversation

Lysxia
Copy link
Collaborator

@Lysxia Lysxia commented Nov 20, 2024

Fixes #164

We desugar all loops using loop, we put a noop marker right before the loop keywords, and the invariants right after. We search for the loop header backwards from the invariants and stop when we hit a marker, in which case the invariants become assertions.

In hindsight this should work just as well with the invariants before loop and the marker after it, but the implementation turns out to be slightly more convenient this way because it lets us search for the marker block by block instead of statement by statement.

@jhjourdan
Copy link
Collaborator

What is the plan for handling for loop? In the case of for loops, the environment can be different from the environment before the loop (and hence shadow variables), and some mutation happen just before the place where the invariant is inserted. But still, the invariant should be interpreted in the outer scope.

@Lysxia Lysxia force-pushed the move-loop-invariants branch from 8dc1795 to 7d24daa Compare November 22, 2024 22:18
@Lysxia
Copy link
Collaborator Author

Lysxia commented Nov 22, 2024

I had to change the invariant_moves test case because the curly braces in the RHS of while let Some(_) = { ... } are not allowed in let .... else ......

@Lysxia Lysxia changed the title Insert loop invariants inside loop bodies Fix detection of loop headers Nov 22, 2024
@Lysxia Lysxia force-pushed the move-loop-invariants branch 3 times, most recently from e23322f to 26b897a Compare November 23, 2024 00:18
@jhjourdan
Copy link
Collaborator

I had to change the invariant_moves test case because the curly braces in the RHS of while let Some(_) = { ... } are not allowed in let .... else ......

That's not very satisfying. Why isn't this syntax allowed? What if we add a pair of parentheses ?

Why didn't you use if let ... { ... } else { break } instead?

@Lysxia
Copy link
Collaborator Author

Lysxia commented Nov 23, 2024

if let works better indeed!

@Lysxia Lysxia force-pushed the move-loop-invariants branch from 26b897a to 277ef5f Compare November 23, 2024 13:21
@jhjourdan
Copy link
Collaborator

if let works better indeed!

Great!

It's weird, though, that "let else" does not support this.

What's happening if the pattern is irrefutable? Does rustc complain about dead code?

@Lysxia Lysxia force-pushed the move-loop-invariants branch 3 times, most recently from adbe5a5 to ad56307 Compare November 25, 2024 14:56
@Lysxia
Copy link
Collaborator Author

Lysxia commented Nov 25, 2024

What's happening if the pattern is irrefutable? Does rustc complain about dead code?

Yes it does. Good catch. I added an attribute to disable the warning.

I fixed the desugaring of variants, which was the last change to pass tests. I noticed that loop variants don't seem to do anything at the moment (unlike variants for recursive functions), there is just one not very meaningful test case for it. To handle that test case I had to merge the desugaring of variants and invariants; the result is not super pretty but it will be easier to clean this code when loop variants are actually implemented to know what it's meant to do.

@Lysxia Lysxia force-pushed the move-loop-invariants branch from ad56307 to 0535910 Compare November 25, 2024 18:04
@Lysxia Lysxia requested a review from jhjourdan November 26, 2024 14:11
@Lysxia Lysxia force-pushed the move-loop-invariants branch from 0535910 to e5eea9d Compare November 29, 2024 21:25
@Lysxia Lysxia force-pushed the move-loop-invariants branch from e5eea9d to 06ce828 Compare November 29, 2024 21:30
@Lysxia Lysxia merged commit 6bebc34 into master Nov 29, 2024
4 checks passed
@Lysxia Lysxia deleted the move-loop-invariants branch November 29, 2024 21:35
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.

Could not find loop header
2 participants