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 AST functionality with excludeMain() #369

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sarpsahinalp
Copy link

@sarpsahinalp sarpsahinalp commented Apr 30, 2024

Improved the current AST functionality to allow instructors to exclude the Main method from AST checks.

  • Added a method, excludeMain(), to avoid AST checking of the Main method, as it is removed from the AST.

@sarpsahinalp sarpsahinalp changed the title Add the excludeMain() method to bypass AST checks for the main method Improve AST functionality with excludeMain() May 3, 2024
@MaisiKoleni
Copy link
Collaborator

MaisiKoleni commented May 17, 2024

Why exclude main? Could you describe a use case here? Why not any other method? Or just just specific issues?

This potentially weakens security (or maybe better called reliability, accuracy here). What are the thoughts on that and the rationale?

@Strohgelaender
Copy link
Collaborator

Strohgelaender commented May 17, 2024

Why exclude main? Could you describe a use case here?

I believe the idea here is to allow students restriction-free local testing of their own solution using the main method.

But I'm not sure if just excluding the main method is the best approach for this. Consider a student submission like this:

public static void main(String[] args) {
   testCase1();
   testCase2();
}

Even though the testCasex() methods are clearly only for local testing and have nothing to do with the exercise, they would be included in the analysis. We probably need to implement some kind of dependency tree to distinguish methods (only) used by the entry point(s) and methods used in the solution of the exercise.

Edit: I just noticed that your other PR implements a MethodCallGraph class. I did not find time to look at this PR in detail, but maybe you can combine these approaches.

.filter(method -> method.isStatic() && method.getNameAsString().equals("main")
&& method.getParameters().size() == 1 && method.getType().isVoidType()
&& method.getParameter(0).getTypeAsString().equals("String[]"))
.forEach(Node::remove);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We modify the AST here, it seems. Are we absolutely sure this has no unintended side effects?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or put differently: what does the removal do? Both according to the documentation and the actual implementation?

Copy link
Author

Choose a reason for hiding this comment

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

Removal removes the node from the tree and it's child nodes. This seems to be working as intented in the documentation and the actual implementation.

@MaisiKoleni
Copy link
Collaborator

If we are going in that direction, a boolean flag is not a sustainable solution (this is quite common).
Consider a design that is more flexible and sustainable. We can still offer a sensible selection of implementations in some way. Think of collect and Collectors, Charset and StandardCharsets, and so on, or similar APIs.

@sarpsahinalp
Copy link
Author

If we are going in that direction, a boolean flag is not a sustainable solution (this is quite common). Consider a design that is more flexible and sustainable. We can still offer a sensible selection of implementations in some way. Think of collect and Collectors, Charset and StandardCharsets, and so on, or similar APIs.

I will take a look in to those APIs thank you for your review!

@sarpsahinalp
Copy link
Author

Why exclude main? Could you describe a use case here?

I believe the idea here is to allow students restriction-free local testing of their own solution using the main method.

But I'm not sure if just excluding the main method is the best approach for this. Consider a student submission like this:

public static void main(String[] args) {
   testCase1();
   testCase2();
}

Even though the testCasex() methods are clearly only for local testing and have nothing to do with the exercise, they would be included in the analysis. We probably need to implement some kind of dependency tree to distinguish methods (only) used by the entry point(s) and methods used in the solution of the exercise.

Edit: I just noticed that your other PR implements a MethodCallGraph class. I did not find time to look at this PR in detail, but maybe you can combine these approaches.

For this we need to use the MethodCallGraph which is again not in scope of this PR. Will take this to Markus 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