ls1intum / Ares

The Artemis Java Test Sandbox. A JUnit 5 Extension for Easy and Secure Artemis Java Testing
https://ls1intum.github.io/Ares/
MIT License
18 stars 7 forks source link

Add complex recursion AST check functionality #370

Open sarpsahinalp opened 6 months ago

sarpsahinalp commented 6 months ago

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

sarpsahinalp commented 6 months ago

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 commented 6 months ago

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!