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

ASTBuilder: Generate sealed class hierarchy #707

Merged
merged 4 commits into from
Feb 10, 2025
Merged

Conversation

maul-esel
Copy link
Contributor

This PR changes the ASTBuilder such that it marks the generated AST classes as sealed (for abstract supertypes such as Expression) resp. final (for concrete classes such as IdentifierExpression).

The intent is to have a precise, complete and closed description of the elements that can make up an AST. As a major consequence, this enables exhaustiveness checks for pattern matching! Code can now switch over all types of e.g. Boogie expressions like this:

switch (expr) {
case IdentifierExpression idExpr -> ...
case BooleanLiteral boolLit -> ...
... (more cases) ...
}

without needing to include a default case, as the compiler can be sure whether or not all cases are handled. This doesn't just get rid of an annoying line of code, it will also have the benefit that if new AST classes are added, the compiler immediately reports all the case distinctions where the new classes must be handled -- rather than silently going to the default and crashing (or worse, behaving incorrectly) at runtime.

My hope is that such pattern matching can be particularly useful when (back-)translating between C, ACSL, Boogie and SMT. I was fiddling around in the IcfgBuilder code when I had this idea; there are several places there that could benefit from pattern matching.

Other Changes

To make this change work, I had to get rid of two hand-written classes that extend the Boogie resp. ACSL expressions and were used in backtranslation. One of them is now in the official ACSL AST (as the backtranslation creates instances that are passed on to other code), the other no longer extends the Boogie AST.

Limitations

I stopped short of even more pattern-matching support by making the classes records -- this would be a more complex change:

  • Records cannot have superclasses (other than java.lang.Record). So we would have to replace abstract base classes like Expression with (sealed) interfaces. Some functionality could be implemented in those interfaces with default methods, but all fields would have to be generated in each concrete implementation.
  • Records can only have immutable fields, and our AST classes have some write-once fields particularly for the ILocation. These cannot easily be made final because the object graph is cyclic -- the node points to the location, and the location points to the node.

As there seem to be some plans to support better pattern matching for (non-record) classes in future Java versions, perhaps the goal of better AST pattern matching can then be achieved in an easier way.

@danieldietsch
Copy link
Member

This is a really nice change!

Copy link
Contributor

@schuessf schuessf left a comment

Choose a reason for hiding this comment

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

Nice change, I have just one open question regarding the change in the AST.

- Replace class TemporaryPointerExpression (extends Boogie's Expression)
  with a sealed interface ExpressionOrPointer and two implementations.

  TemporaryPointerExpression was not visible to any code outside of
  CACSL2BoogieBacktranslator, and it did not need to extend Expression.

- Make FakeAcslPointerExpression (extends ACSL's Expression)
  an official class in the AST description.

  Instances of this class are part of the returned backtranslated
  program executions, so code outside of this class should know the type
  in order to handle these instances.

These changes are in preparation for modifying ASTBuilder such that it
generates sealed class hierarchies, which will be more amenable to pattern
matching (in particular, enabling exhaustiveness checks).
This check is broken, in the sense that it is always fals and does not achieve
the intended effect: an ACSL Contract instance never implements the interface
IASTFunctionDefinition. The intent seems to have been to check whether the
annotated element of the C AST is an IASTFunctionDefinition.

As contracts are now supported, the check is also obsolete and can be removed.
This allows pattern-matching switches over AST nodes to be recognized as
exhaustive by the Java compiler, such that a "default" case can be omitted.

The intent is to increase maintainability. A newly added type in the AST
will cause compile errors immediately (for previously-exhaustive switches
without default clause), rather than silently going into default clauses
and thereby causing potentially incorrect behaviour at runtime.
@schuessf
Copy link
Contributor

I just started the Nightly build to be sure that there is no issue with this change 🙂

@maul-esel
Copy link
Contributor Author

👍 Thanks. I always forget that it has to be re-enabled after creating the PR. The previous build for the branch looks good though.

@maul-esel
Copy link
Contributor Author

If there are no further objections and the CI does not show new problems, I will merge this tonight.

@schuessf
Copy link
Contributor

👍 Thanks. I always forget that it has to be re-enabled after creating the PR. The previous build for the branch looks good though.

Oh, if you ran it on the branch before, there are probably to crucial changes. I just did not find any reference in this PR to a succesful Nightly. But it does not hurt to wait for another Nightly run either.

@maul-esel maul-esel merged commit 77843cb into dev Feb 10, 2025
3 of 4 checks passed
@maul-esel maul-esel deleted the wip/dk/sealed-ast branch February 10, 2025 19:50
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