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

Improve corres rules whileLoop #650

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

michaelmcinerney
Copy link
Contributor

Please see the commit message for a brief explanation

@michaelmcinerney
Copy link
Contributor Author

Sorry, forced pushed to correct the commit message

lib/ExtraCorres.thy Outdated Show resolved Hide resolved
Copy link
Member

@Xaphiosis Xaphiosis left a comment

Choose a reason for hiding this comment

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

Having spent a bit of time poking that compare time while loop, this improvement makes a lot of sense!

lib/ExtraCorres.thy Outdated Show resolved Hide resolved
lib/ExtraCorres.thy Outdated Show resolved Hide resolved
lib/ExtraCorres.thy Outdated Show resolved Hide resolved
lib/ExtraCorres.thy Outdated Show resolved Hide resolved
lib/Monads/No_Fail.thy Outdated Show resolved Hide resolved
Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

Cool, much nicer rules! Minor style nitpicks only.

lib/Monads/No_Fail.thy Outdated Show resolved Hide resolved
Comment on lines 245 to 246
and body_inv: "\<And>r. \<lbrace>P r and C r\<rbrace> B r \<lbrace>\<lambda>r. P r\<rbrace>"
"\<And>r'. \<lbrace>P' r' and C' r'\<rbrace> B' r' \<lbrace>\<lambda>r'. P' r'\<rbrace>"
Copy link
Member

Choose a reason for hiding this comment

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

I'm undecided myself, but could using rv in these postconditions to distinguish from the bound r improve readability? I guess a potential concern is that it could also be confused with the final return value of the whole loop.

Suggested change
and body_inv: "\<And>r. \<lbrace>P r and C r\<rbrace> B r \<lbrace>\<lambda>r. P r\<rbrace>"
"\<And>r'. \<lbrace>P' r' and C' r'\<rbrace> B' r' \<lbrace>\<lambda>r'. P' r'\<rbrace>"
and body_inv: "\<And>r. \<lbrace>P r and C r\<rbrace> B r \<lbrace>\<lambda>rv. P rv\<rbrace>"
"\<And>r'. \<lbrace>P' r' and C' r'\<rbrace> B' r' \<lbrace>\<lambda>rv'. P' rv'\<rbrace>"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The situation is quite tricky with these whileLoops. There is the (blue) initial value given to the whileLoop (you can see an example of this in the conclusion of the statement of corres_whileLoop_ret). Then there is the (green) return value given by the loop body. I think it would be nicer to stick with the r for here, to reinforce that connection. But happy to discuss it some more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other alternative for these valid bits is that I could just eta contract the postcondition. I left it written out to emphasise that these guards do talk about the return value, but maybe that's clear enough

Copy link
Member

Choose a reason for hiding this comment

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

I would usually prefer the eta-contracted version unless we're trying to do the bound-name preservation trick where Isabelle would replace rv with whatever name was in the goal (which is not the case here).

@michaelmcinerney
Copy link
Contributor Author

Also, most of these lemmas use the assumes... shows and I don't think they're properly indented. The autoindenter will indent will leave the assumes indented by 2, indent the subsequent ands by a further 2 spaces, and then have the shows indented by 2. Should I go with that?

@Xaphiosis
Copy link
Member

Also, most of these lemmas use the assumes... shows and I don't think they're properly indented. The autoindenter will indent will leave the assumes indented by 2, indent the subsequent ands by a further 2 spaces, and then have the shows indented by 2. Should I go with that?

I thought it would do this:

assumes a: blah
    and dsad: blah2

hmm.

@lsf37
Copy link
Member

lsf37 commented Oct 3, 2023

Also, most of these lemmas use the assumes... shows and I don't think they're properly indented. The autoindenter will indent will leave the assumes indented by 2, indent the subsequent ands by a further 2 spaces, and then have the shows indented by 2. Should I go with that?

I usually avoid the and and just repeat the assumes, which is also more stable against moving things around. The correct indent of that is:

lemma name:
  assumes x: "..."
  assumes y: "..."
  shows "..."

The and is mostly useful if you really need two propositions to be parsed in a single assumes for typing reasons or similar, which is very rare. The other times I use it, if it's just very short statements like assumes a: "A" and b: "B".

@michaelmcinerney michaelmcinerney force-pushed the michaelm-whileLoop_improvements branch from 164fe2d to 78ba96f Compare October 5, 2023 07:02
@michaelmcinerney michaelmcinerney force-pushed the michaelm-whileLoop_improvements branch from 78ba96f to 7170ef1 Compare October 5, 2023 12:26
@michaelmcinerney michaelmcinerney merged commit e7cca6a into master Oct 5, 2023
14 checks passed
@michaelmcinerney michaelmcinerney deleted the michaelm-whileLoop_improvements branch October 5, 2023 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants