Skip to content

Fix issue #841#919

Draft
jcp19 wants to merge 4 commits intomasterfrom
fix-841
Draft

Fix issue #841#919
jcp19 wants to merge 4 commits intomasterfrom
fix-841

Conversation

@jcp19
Copy link
Contributor

@jcp19 jcp19 commented Apr 22, 2025

This PR fixes issue #841 and applies the following changes:

  • unifies the logic to check the signature/contract of pure functions between in interface declarations and outside of it
  • it moves the checks of well foundedness of termination measures from MemberTyping.scala to GhostMemberTyping.scala
  • it disallows conditional termination measures in ghost or pure functions/methods. We currently do not check that they are exhaustive, and given that this feature is pretty much never used, I went with the simple syntactic check instead of a sematnic one, which is more efficient

@jcp19 jcp19 requested a review from ArquintL April 22, 2025 14:42
@jcp19 jcp19 linked an issue Apr 22, 2025 that may be closed by this pull request
Copy link
Member

@ArquintL ArquintL left a comment

Choose a reason for hiding this comment

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

Looks overall good to me but I've left some requests for small changes

Comment on lines +47 to +48
// (João) Conditional termination measures are a very rarely used feature. They allow defining termination measures
// case-per-case. However, for pure or ghost functions and methods, we need to show that all conditions provided
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// (João) Conditional termination measures are a very rarely used feature. They allow defining termination measures
// case-per-case. However, for pure or ghost functions and methods, we need to show that all conditions provided
// (João) Conditional termination measures are a very rarely used feature. They allow defining termination measures
// case-by-case. However, for pure or ghost functions and methods, we need to show that all conditions provided

val hasConditionalTerminationMeasureErrors = {
val conditionalMeasure = spec.terminationMeasures.find(_.isConditional)
conditionalMeasure match {
case Some(n) => error(n, "Conditional termination measures are not allowed in pure members.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case Some(n) => error(n, "Conditional termination measures are not allowed in pure members.")
case Some(n) => error(n, "Conditional termination measures are not allowed for pure or ghost functions and methods.")

}

private[typing] def wellDefIfPureMethod(member: PMethodDecl): Messages = {
if (member.spec.isPure) {
Copy link
Member

Choose a reason for hiding this comment

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

this if condition is redundant

@@ -61,15 +74,23 @@ trait GhostMemberTyping extends BaseTyping { this: TypeInfoImpl =>

private[typing] def wellDefIfPureFunction(member: PFunctionDecl): Messages = {
if (member.spec.isPure) {
Copy link
Member

Choose a reason for hiding this comment

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

this if condition is redundant

Comment on lines +34 to +35
// spec must come from a ghost or pure function
private[typing] def wellFoundedIfNeeded(spec: PFunctionSpec): Messages = {
Copy link
Member

Choose a reason for hiding this comment

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

three comments:

  • can we change the behavior such that this function can be called for any spec and internally checks whether it's a spec for a ghost or pure function / method? That's the intuition I have for these wellFoundedIf... functions. Alternatively, please add a require stmt that checks this assumption
  • Maybe I'm missing something but isn't this function only called for explicitly ghost functions and methods? To me, it seems like we do not call this function for actual pure functions and methods
  • can we rename this function? Without looking at its implementation, I do not have any clue what Needed is supposed to do. What about wellFoundedIfGhostOrPure?

Comment on lines +16 to +17
//:: ExpectedOutput(type_error)
decreases _ if b
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//:: ExpectedOutput(type_error)
decreases _ if b
//:: ExpectedOutput(type_error)
decreases _ if b // we syntactically disallow conditional termination measures for members that must terminate

Comment on lines +7 to +12
// Type error, pure function does not have termination measures.
//:: ExpectedOutput(type_error)
pure
// Type error, pure function cannot have variadic parameters.
//:: ExpectedOutput(type_error)
M(a ...int) int
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 split the errors into separate interface functions. Furthermore, I find it a bit funny that one error occurs at the pure keyword whereas the other errors occur at the parameter declaration site but that's just an artifact of how we type-check. We could consider making the following declarations one-liners such that the testcases are not overly restrictive (should we ever indicate missing termination measures, e.g., at the function name

Suggested change
// Type error, pure function does not have termination measures.
//:: ExpectedOutput(type_error)
pure
// Type error, pure function cannot have variadic parameters.
//:: ExpectedOutput(type_error)
M(a ...int) int
// Type error, pure function does not have termination measures.
//:: ExpectedOutput(type_error)
pure
M1(a int) int
// Type error, we do not permit conditional termination measures for members that must terminate
//:: ExpectedOutput(type_error)
decreases if a == 42
pure
M2(a int) int
decreases
pure
// Type error, pure function cannot have variadic parameters.
//:: ExpectedOutput(type_error)
M3(a ...int) int
decreases
pure
// Type error, pure function must have exactly one return parameter
//:: ExpectedOutput(type_error)
M4(a int)

@jcp19 jcp19 mentioned this pull request Apr 23, 2025
@jcp19 jcp19 marked this pull request as draft June 3, 2025 10:14
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.

Requiring termination measures for pure interface methods

2 participants