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

Add complex recursion AST check functionality #370

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sarpsahinalp
Copy link

@sarpsahinalp sarpsahinalp commented May 3, 2024

Added new JavaParser methods to create a method call graph and detect recursion via cycles in the graph

  • Added UnknownRecursionAssert with hasRecursion(), hasNoRecursion() methods
  • MethodCallGraph class representing the Graph that is produced
    - VoidVisitorAdapter to traverse the nodes in the CompilationUnit of AST.
  • Added excludeMethods(Method... methods) to exclude wished nodes from the MethodCallGraph
  • Added startingNode(Method method) to determine a starting node, when checking for Recursion within a method, via DFS traverse the tree.
  • Added tests for the correctness of the implementation

Copy link
Collaborator

@Strohgelaender Strohgelaender left a comment

Choose a reason for hiding this comment

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

Your PR contains many error messages annotated with "$NON-NLS-1$". I'm not sure about the localization state of the javaparser parts in Ares, but I think these user-facing error messages should also offer translations.

pom.xml Show resolved Hide resolved
Copy link
Collaborator

@MaisiKoleni MaisiKoleni left a comment

Choose a reason for hiding this comment

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

Doing call graphs correctly is really difficult and close to impossible. I think the design and strictness should depend if the intended purpose. The default intended purpose of Ares functionality is grading (not just calculating a number for practice exercises in Artemis, but final exam grades in the curriculum), which is why security is so often important.
This security does not need to be absolute, incredibly difficult to circumvent is enough.

For my taste, this is currently too easy. I will have a closer look at the code, but my time is limited, probably someone else from ls1intum needs to do that. My thoughts:

  • what about method reference expressions?
  • what about virtual method invocation? (interfaces, superclasses)

The last one is particularly hard. I had a similar problem not too long ago and had only partial success. Tools (SAST) that do this well are sold for good money, there is not much else.

In the end, you need to know what you want. I just want to warn you that it is either easy to circumvent or very difficult to implement. So again, think about your use case and what you need. If you are willing to make compromises, because the feature is intended for practice exercises / low stake assessment only, you can do so but should clearly document that scope.

@@ -0,0 +1,18 @@
#!/bin/bash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those two are not needed right? At least I could not see what for.

@sarpsahinalp
Copy link
Author

Your PR contains many error messages annotated with "$NON-NLS-1$". I'm not sure about the localization state of the javaparser parts in Ares, but I think these user-facing error messages should also offer translations.

The overall feedback will be improved via my thesis, therefore this PR was a quick solution for the AST Recursion check.

@sarpsahinalp
Copy link
Author

sarpsahinalp commented May 23, 2024

Doing call graphs correctly is really difficult and close to impossible. I think the design and strictness should depend if the intended purpose. The default intended purpose of Ares functionality is grading (not just calculating a number for practice exercises in Artemis, but final exam grades in the curriculum), which is why security is so often important. This security does not need to be absolute, incredibly difficult to circumvent is enough.

For my taste, this is currently too easy. I will have a closer look at the code, but my time is limited, probably someone else from ls1intum needs to do that. My thoughts:

  • what about method reference expressions?
  • what about virtual method invocation? (interfaces, superclasses)

The last one is particularly hard. I had a similar problem not too long ago and had only partial success. Tools (SAST) that do this well are sold for good money, there is not much else.

In the end, you need to know what you want. I just want to warn you that it is either easy to circumvent or very difficult to implement. So again, think about your use case and what you need. If you are willing to make compromises, because the feature is intended for practice exercises / low stake assessment only, you can do so but should clearly document that scope.

Via Java Symbol Resolver, we can handle most cases of interfaces, superclasses, and most expressions. However, as you mentioned, this functionality is currently intended for the introductory programming course, so with the exception of anonymous classes, most cases are covered. I will improve the documentation further!

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.

3 participants